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

Remove race between ID, Push & Delta #907

Merged
merged 11 commits into from
May 13, 2020
Merged

Conversation

aarshkshah1992
Copy link
Contributor

@aarshkshah1992 aarshkshah1992 commented May 4, 2020

For #823 & #903 .

Blankhost PR at libp2p/go-libp2p-blankhost#42.

p2p/host/basic/basic_host.go Show resolved Hide resolved
p2p/protocol/identify/id.go Show resolved Hide resolved
p2p/protocol/identify/peer_loop.go Show resolved Hide resolved
p2p/protocol/identify/peer_loop.go Outdated Show resolved Hide resolved
Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

Partial review. I'm going to allocate some time tomorrow to do a more thorough review.

p2p/protocol/identify/id_push.go Outdated Show resolved Hide resolved
p2p/host/basic/basic_host.go Show resolved Hide resolved
p2p/host/basic/basic_host.go Outdated Show resolved Hide resolved
p2p/host/basic/basic_host.go Outdated Show resolved Hide resolved
p2p/protocol/identify/id.go Show resolved Hide resolved
p2p/protocol/identify/id.go Show resolved Hide resolved
@Stebalien Stebalien mentioned this pull request May 7, 2020
Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

Nice! I was not expecting something this clean for a problem this gnarly.

❤️

p2p/protocol/identify/id.go Outdated Show resolved Hide resolved
p2p/protocol/identify/peer_loop.go Show resolved Hide resolved
p2p/protocol/identify/id.go Show resolved Hide resolved
p2p/protocol/identify/peer_loop.go Outdated Show resolved Hide resolved
p2p/protocol/identify/id.go Outdated Show resolved Hide resolved
@@ -63,6 +59,15 @@ func init() {
// transientTTL is a short ttl for invalidated previously connected addrs
const transientTTL = 10 * time.Second

type addPeerHandlerReq struct {
s network.Stream
Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Stebalien I see what you mean. Have made the change.

p2p/protocol/identify/id.go Outdated Show resolved Hide resolved
p2p/protocol/identify/peer_loop.go Outdated Show resolved Hide resolved
p2p/protocol/identify/peer_loop.go Outdated Show resolved Hide resolved
@aarshkshah1992
Copy link
Contributor Author

@Stebalien Have addresses your review comments with some counter questions. Please take a look when you can !

@aarshkshah1992
Copy link
Contributor Author

Hey @Stebalien

Have made all the changes. This PR is ready for one last look. Please take a look when you can !

@Stebalien
Copy link
Member

(rebased)

Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

Suggested changes in #922 but otherwise LGTM.

@aarshkshah1992 aarshkshah1992 merged commit 973933a into master May 13, 2020
@aarshkshah1992 aarshkshah1992 deleted the feat/identify-races branch May 14, 2020 11:33
@raulk
Copy link
Member

raulk commented May 15, 2020

This PR was merged and released, but it points to a commit of go-libp2p-blankhost instead of a release:

973933a#diff-37aff102a57d3d7b797f152915a6dc16R14

@raulk
Copy link
Member

raulk commented May 15, 2020

Looks like it wasn't released, so we have time to fix this on master. @aarshkshah1992?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants