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

Properly close streams #104

Conversation

edsko
Copy link
Contributor

@edsko edsko commented Jan 4, 2024

This avoids another "blocked indefinitely" in client code, following on from #97. I suspect that there are probably others, still.

@kazu-yamamoto I'm not entirely sure which exception to use when the stream is closed; if ConnectionIsClosed is not the right one, let me know. Also, I think the first Open/Open case probably never arises in practice, but this way the code doesn't make any assumptions; but in principle I think we can just leave out this case (but I'm not 100% sure).

Incidentally, I am writing a blog post about deadlock detection in STM, which also illustrates why fixes like this are important; I'll post a link once I have a preview in case you're curious.

@edsko
Copy link
Contributor Author

edsko commented Jan 6, 2024

Incidentally, without this PR, a call to getRequestBodyChunk may either throw a "blocked indefinitely on STM" exception if one is lucky, or simply stall forever, depending on when GC runs exactly (and whether or not in runs), and whether or not it is therefore able to detect the fact that the STM transaction is blocked indefinitely. For this reason, without this PR my grapesy test suite occasionally hangs in tests with unclean client termination.

edsko added a commit to well-typed/grapesy that referenced this pull request Jan 6, 2024
We still depend on a fix to `http2`; without it, depending on whether or not
the GC is able to detect an STM deadlock, the tests can hang. See
kazu-yamamoto/http2#104 for details.
@kazu-yamamoto kazu-yamamoto self-requested a review January 9, 2024 03:15
edsko added a commit to well-typed/grapesy that referenced this pull request Jan 11, 2024
We still depend on a fix to `http2`; without it, depending on whether or not
the GC is able to detect an STM deadlock, the tests can hang. See
kazu-yamamoto/http2#104 for details.
edsko added a commit to well-typed/grapesy that referenced this pull request Jan 11, 2024
We still depend on a fix to `http2`; without it, depending on whether or not
the GC is able to detect an STM deadlock, the tests can hang. See
kazu-yamamoto/http2#104 for details.
@alt-romes
Copy link

@kazu-yamamoto FWIW the aforementioned @edsko's blogpost has been published at https://well-typed.com/blog/2024/01/when-blocked-indefinitely-is-not-indefinite/

@kazu-yamamoto
Copy link
Owner

Yes. I tweeted about the blog post already.

Copy link
Owner

@kazu-yamamoto kazu-yamamoto left a comment

Choose a reason for hiding this comment

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

LGTM

kazu-yamamoto added a commit that referenced this pull request Jan 16, 2024
@kazu-yamamoto
Copy link
Owner

Rebased and merged.
Thank you for your contribution!

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.

None yet

3 participants