Make sure connection preface is always sent first

kazu-yamamoto / http2

HTTP/2.0 library including HPACK

BSD 3-Clause "New" or "Revised" License

86 stars 22 forks source link

Make sure connection preface is always sent first #33

Closed pcapriotti closed 2 years ago

pcapriotti commented 3 years ago

This fixes a race condition in the client between the frameSender thread and the main thread, in the case where a SETTINGS frame is sent in response to a server SETTINGS frame before the connection preface has had a chance to be sent.

This PR also includes a commit to prevent client threads from waiting forever on stream input when the client frameSender thread dies. This is useful in its own right, I think, but in particular it is needed to make the test for the above fix work.

pcapriotti commented 3 years ago

Would you fix the race issue only in this PR? Based on the experience of quic, I think that the waiting-forever issue should be fixed by the thread tree feature provided by async.

I could do that, but then I'm not sure how to set up a test for it. Could you elaborate on how you want the waiting-forever issue to be fixed? I can try writing a patch for that first.

pcapriotti commented 2 years ago

I rebased this branch so that it only contains the preface race fix. I wrote a patch employing async to propagate exceptions and thread termination to the main client thread, based on this one: https://github.com/wireapp/http2/commit/617c685b1c7727b77a006c4f5d4d65f3cc52fded, and the preface test is now based on both of these patches: https://github.com/wireapp/http2/commit/fb4afa7aa16641f7642b797d55af3465e6a103fa.

Should I submit these as separate PRs or include them here?

kazu-yamamoto commented 2 years ago

Thank you for brushing up this PR. What I did so far is reordering these commits:

fb4afa7 should come first. If you do so, you can demonstrate that the bug exists. I confirmed that the test gets never-finished.
Then 78c68d8 and 617c685 should follow. With two patches, the test gets finished.

If you kindly make this PR in this order, I'd love to merge.

pcapriotti commented 2 years ago

If you kindly make this PR in this order, I'd love to merge.

Done.

kazu-yamamoto commented 2 years ago

Merged. Thank you for your contribution! If you wish, I will release a new version.

pcapriotti commented 2 years ago

Thanks for merging!

If you wish, I will release a new version.

We are fine depending on a git version, so up to you. Also, I will probably have more PRs coming in the next few days.

kazu-yamamoto commented 2 years ago

So, I wait for the PRs.