-
Notifications
You must be signed in to change notification settings - Fork 11
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
[1.0.2] P2P: Resolve on reconnect - Take 2 #853
Conversation
{ | ||
std::shared_lock g( connections_mtx ); | ||
con = find_connection_i( host ); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we keep the lock until we are done using con
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. get_status
grabs a different mutex. Safer to not hold both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So should we do the same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did do it there.
plugins/net_plugin/net_plugin.cpp
Outdated
if (colon == std::string::npos || colon == 0) { | ||
fc_elog( logger, "Invalid peer address. must be \"host:port[:<blk>|<trx>]\": ${p}", ("p", peer_address()) ); | ||
return false; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There seems to be a little duplication between this block and the split_host_port_type()
below. I'm not sure it's worth trying to fix up though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is a bit of a mess. I'll fix it up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Resolve address on reconnect. (Take 2, See #825)
Instead of:
Now you get:
This takes a different approach than #825. This approach is closer to Leap 4.0 approach to connection lifetime although it does keep the
connections_manager
added in Leap 5.0.Plan on backporting to Leap 5.0 once this is merged.
Resolves #525