Remove race between ID, Push & Delta #907
Conversation
Stebalien
left a comment
Stebalien
left a comment
There was a problem hiding this comment.
Partial review. I'm going to allocate some time tomorrow to do a more thorough review.
Stebalien
left a comment
Stebalien
left a comment
There was a problem hiding this comment.
Nice! I was not expecting something this clean for a problem this gnarly.
❤️
p2p/protocol/identify/id.go
Outdated
| const transientTTL = 10 * time.Second | ||
|
|
||
| type addPeerHandlerReq struct { | ||
| s network.Stream |
There was a problem hiding this comment.
Having to send this stream is unfortunate but I see why we're doing it.
What if, instead of preparing/recording the entire message inside the loop, we just recorded our protocols/addresses? Then, we'd change the populateMessage signature to take the set of protocols/addresses that should be sent.
As is, we may end up sending the peer the wrong observed address (we'll send them the address observed on the initial connection).
There was a problem hiding this comment.
@Stebalien I do understand the intuition behind not wanting to send streams to channels but can you explain with a bit more detail why we wouldn't to pass streams across channels.
I am making the change.
There was a problem hiding this comment.
Streams on channels is fine. The issue is really:
- The identify loop doesn't actually care about the stream, we're handling the response outside of the loop. Really, we're just sending the stream so we can pull off the remote address so we can initialize an identify message that we're not even sending from the loop.
- The observed address will always be the addressed observed during the first identify request until we finally disconnect.
There was a problem hiding this comment.
@Stebalien I see what you mean. Have made the change.
|
@Stebalien Have addresses your review comments with some counter questions. Please take a look when you can ! |
|
Hey @Stebalien Have made all the changes. This PR is ready for one last look. Please take a look when you can ! |
Stebalien
force-pushed
the
feat/identify-races
branch
from
be84156 to
b964c9f
Compare
|
(rebased) |
|
This PR was merged and released, but it points to a commit of go-libp2p-blankhost instead of a release: |
|
Looks like it wasn't released, so we have time to fix this on master. @aarshkshah1992? |
4 participants
For #823 & #903 .
Blankhost PR at libp2p/go-libp2p-blankhost#42.