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

Make sure connection preface is always sent first #33

Merged
merged 3 commits into from
Dec 10, 2021

Conversation

pcapriotti
Copy link
Contributor

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.

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.

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.

@pcapriotti
Copy link
Contributor Author

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 added a commit to wireapp/http2 that referenced this pull request Dec 10, 2021
This tests that a client always sends the connection preface before any
other frame (see PR kazu-yamamoto#33).
@pcapriotti
Copy link
Contributor Author

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: wireapp@617c685, and the preface test is now based on both of these patches: wireapp@fb4afa7.

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

@kazu-yamamoto
Copy link
Owner

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.

This tests that a client always sends the connection preface before any
other frame (see PR kazu-yamamoto#33).
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.
Also throw an exception if one of the worker threads terminates before
the client has received a response.
@pcapriotti
Copy link
Contributor Author

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

Done.

@kazu-yamamoto kazu-yamamoto merged commit b5fd8cc into kazu-yamamoto:master Dec 10, 2021
@kazu-yamamoto
Copy link
Owner

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

@pcapriotti
Copy link
Contributor Author

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
Copy link
Owner

So, I wait for the PRs.

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Feb 12, 2022
3.0.3
* Return correct status messages in HTTP2 client
  (#31)[kazu-yamamoto/http2#31]
* Follow changes in Aeson 2
  (#32)[kazu-yamamoto/http2#32]
* Make sure connection preface is always sent first
  (#33)[kazu-yamamoto/http2#33]
* Avoid empty data (#34)[kazu-yamamoto/http2#34]

3.0.2
* Skip inserting entries that do not fit in the encoding table
  (#28)[kazu-yamamoto/http2#28]
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