Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Respond to keepalive messages within a TCP socket #1742

Merged
merged 3 commits into from
Feb 18, 2019
Merged

Conversation

clemahieu
Copy link
Contributor

No description provided.

@argakiig argakiig added the functionality quality improvements This item indicates the need for or supplies a better way to implement existing functionality label Feb 18, 2019
@zhyatt zhyatt added this to the V18.0 milestone Feb 18, 2019
std::unique_ptr<nano::keepalive> request (new nano::keepalive (error, stream, header_a));
if (!error)
{
add_request (std::unique_ptr<nano::message> (request.release ()));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can't this just be: add_request (std::move (request)); ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, a lot of these need to be fixed to use c++13 semantics.

});

auto output (keepalive.to_bytes ());
bool done (false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably should be atomic ?

{
BOOST_LOG (connection->node->log) << boost::str (boost::format ("Keepalive req sent to %1%") % connection->socket->remote_endpoint ());
}
connection->socket->async_write (bytes, [connection = connection](boost::system::error_code const & ec, size_t size_a) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

connection = connection (?)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

connection is a member variable, so if you want to pass it in without passing this this is the way to do it (and [&var = var] for reference)

Copy link
Contributor

@cryptocode cryptocode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM + test works locally

@argakiig argakiig merged commit 685ffc9 into master Feb 18, 2019
argakiig pushed a commit that referenced this pull request Feb 18, 2019
* Respond to keepalive messages within a TCP socket so we can determine version in both directions.

* Fixing logging copy-pasta from frontier_req body.

* Fixing double-counting incoming keepalives.
@clemahieu clemahieu deleted the keepalive_tcp branch March 20, 2019 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
functionality quality improvements This item indicates the need for or supplies a better way to implement existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants