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

Avoid prematurely unlinking streams in send_reset, in some cases. #319

Merged
merged 2 commits into from
Oct 16, 2018

Conversation

goffrie
Copy link
Contributor

@goffrie goffrie commented Oct 11, 2018

Because send_reset called recv_err, which calls reclaim_all_capacity,
which eventually calls transition(stream, ..) -- all of which happens before
the RESET frame is enqueued -- it was possible for the stream to get unlinked
from the store (if there was any connection-level capacity to reassign). This
could then cause the stream to get "leaked" on drop/EOF since it would no longer
be iterated.

Fix this by delaying the call to reclaim_all_capacity after enqueueing the
RESET frame.

A test demonstrating the issue is included.

Because `send_reset` called `recv_err`, which calls `reclaim_all_capacity`,
which eventually calls `transition(stream, ..)` -- all of which happens _before_
the RESET frame is enqueued -- it was possible for the stream to get unlinked
from the store (if there was any connection-level capacity to reassign). This
could then cause the stream to get "leaked" on drop/EOF since it would no longer
be iterated.

Fix this by delaying the call to `reclaim_all_capacity` _after_ enqueueing the
RESET frame.

A test demonstrating the issue is included.
Copy link
Collaborator

@carllerche carllerche left a comment

Choose a reason for hiding this comment

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

Makes sense. Thanks for the explanation and the test 👍 .

@carllerche
Copy link
Collaborator

Unfortunately, the build is broken due to an unrelated problem. I will work on fixing that.

@carllerche
Copy link
Collaborator

I submitted #321 to fix the build.

@carllerche carllerche merged commit ea8b8ac into hyperium:master Oct 16, 2018
goffrie added a commit to goffrie/h2 that referenced this pull request Oct 26, 2018
carllerche pushed a commit that referenced this pull request Oct 29, 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