connectd: Update connection list with new address #4192
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustyrussell
approved these changes
Nov 11, 2020
Contributor
rustyrussell
left a comment
rustyrussell
left a comment
There was a problem hiding this comment.
Needs some pycheck fixes, and Travis is grumpy...
tests/plugins/slow_start.py
Outdated
| @plugin.async_method('waitconn') | ||
| def wait_connection(request, plugin): | ||
| sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM) | ||
| addr = ('localhost', 6669) |
Contributor
There was a problem hiding this comment.
Use port number 0, which will assign an unused ephemeral port? Then use getsockname and print out what port you got.
This avoids conflicts on port 6669.
cdecker
reviewed
Nov 12, 2020
Member
cdecker
left a comment
cdecker
left a comment
There was a problem hiding this comment.
The Travis failure might just be a timeout that is a bit optimistic in the .result(1) calls below. I'm wondering though why the decoy port is managed in a plugin at all, since the creation of the listening socket and acceptance of the connection could just as well be done in the test itself.
Collaborator
Author
Needs signaling between the listening socket and the main test thread. By using a plugin we can take advantage of Open to different ideas for how to do inter-thread signaling on python. /me eyes Golang channels. |
niftynei
force-pushed
the
nifty/fix-4154
branch
2 times, most recently
from
13792a4 to
a8b1a38
Compare
Member
Ah yes, inter-thread / inter-process communication :-) But no need to amend, I was just curious as to why the big hammer was used in this case :-) |
Collaborator
Author
I can't take credit for the idea, @rustyrussell suggested it first lol. |
Member
|
Seems Travis doesn't like the change just yet. Failing tests are:
The stresstest one seems to be a crash here: So likely just a null-pointer somewhere in |
If we're already attempting to connect to a peer, we would ignore new connection requests. This is problematic if your node has bad connection details for the node -- you can't update it while inflight. This patch appends new connection suggestions to the list of connections to try. Fixes ElementsProject#4154
Collaborator
Author
|
So I'm a bit confused by this crash. Line 288 of Except that, we shouldn't be calling Update: the connection associated with the |
Coauthored-By: Rusty Russell @rustyrussell
niftynei
force-pushed
the
nifty/fix-4154
branch
from
a8b1a38 to
6491961
Compare
rustyrussell
approved these changes
Nov 17, 2020
Contributor
rustyrussell
left a comment
rustyrussell
left a comment
There was a problem hiding this comment.
Ack 6491961
Nice catch of long-outstanding race between incoming and outgoing conns!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
If we're already attempting to connect to a peer, we would ignore
new connection requests. This is problematic if your node has bad
connection details for the node -- you can't update it while inflight.
This patch appends new connection suggestions to the list of connections
to try.
Fixes #4154
Changelog-None