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

always reset ping streams when done #380

Merged
merged 1 commit into from
Aug 21, 2018
Merged

Conversation

Stebalien
Copy link
Member

Currently, we leak a stream. We could use FullClose but, IMO, it's not worth it. We might as well just reset and walk away.

Currently, we leak a stream. We could use FullClose but, IMO, it's not worth it.
We might as well just reset and walk away.
@ghost ghost assigned Stebalien Jul 27, 2018
@ghost ghost added the status/in-progress In progress label Jul 27, 2018
@Stebalien Stebalien requested a review from marten-seemann July 27, 2018 17:55
@Stebalien
Copy link
Member Author

@marten-seemann thoughts?

@marten-seemann
Copy link
Contributor

I guess it works this way, but we're kind of misusing Reset here, don't we? Reset is intended for abnormal stream termination (e.g. a peer sends me stuff that I don't need).

@Stebalien
Copy link
Member Author

Yeah, that's why I pulled you into the conversation. In this case, I think reset may be the correct solution because we only bail when:

  1. The other side closes the stream (they shouldn't do that).
  2. The other side sends something we don't want (they shouldn't do that).
  3. The user interrupts us (we should terminate immediately).

@marten-seemann
Copy link
Contributor

Ok, that means that in Ping we have to close the stream on ctx.Done(), and reset under all other conditions.

Thinking about this, pings should be independent of each other, so we really should open a new stream for every ping. I opened an issue (#391) for that.

@Stebalien
Copy link
Member Author

Ok, that means that in Ping we have to close the stream on ctx.Done(), and reset under all other conditions.

Well, the context can still be canceled at any time. I guess we could close nicely if the context is canceled between pings but I don't really see any reason not to reset then. The reason to reset is that:

  1. That way, we don't have to wait around for an EOF.
  2. The other side doesn't care either way.

@Stebalien Stebalien merged commit 7d6f952 into master Aug 21, 2018
@ghost ghost removed the status/in-progress In progress label Aug 21, 2018
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.

2 participants