search

kazu-yamamoto / http2

HTTP/2.0 library including HPACK
BSD 3-Clause "New" or "Revised" License
86 stars 23 forks source link

Properly close streams #104

Closed edsko closed 10 months ago

edsko commented 11 months ago

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 commented 10 months ago

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.

alt-romes commented 10 months ago

@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 commented 10 months ago

Yes. I tweeted about the blog post already.

kazu-yamamoto commented 10 months ago

Rebased and merged. Thank you for your contribution!