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

deprecate NegotiateLazy #85

Merged
merged 1 commit into from
Apr 21, 2022
Merged

deprecate NegotiateLazy #85

merged 1 commit into from
Apr 21, 2022

Conversation

marten-seemann
Copy link
Contributor

Negotiate and NegotiateLazy both

  1. block until they've read the first "/multistream/1.0.0" from rwc
  2. write the "/multistream/1.0.0" response
  3. read tokens from rwc in a loop, echoing the the token if the protocol is supported, or send "na" if the protocol is not supported

The only difference between these two function is that Negotiate performs writes synchronously, whereas NegotiateLazy spawns a separate Go routine to perform these writes, and returns before these writes have finishes.

That means that NegotiateLazy is only faster than Negotiate if we assume that any of these writes would block / take significant time. I would argue that this is not the case, as we're either writing to a network.SecureConn or a network.Stream, both of which probably are buffered, or wouldn't block under normal circumstances.

Deprecating NegotiateLazy would allow us to easily fix the race condition (#83) by just deleting the faulty code path :)

Copy link
Contributor

@vyzo vyzo left a comment

Choose a reason for hiding this comment

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

do it!

@Stebalien
Copy link
Member

Let me page in the context here. IIRC, there was a very good reason not to do that.

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.

So, we can merge this:

  1. It probably won't affect performance (like, at all). It might actually improve performance by removing a goroutine.
  2. It has nothing to do with race condition in NegotiateLazy #83 because I apparently suck at writing issues.

#83 was about the client side, this is the server side. We can't remove the client-side of this because that would make every multistream negotiation take 1rt (before the actual protocol can start).

This, honestly, shouldn't even be called lazy. It's just some strange async buffering thing.

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.

3 participants