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

Client sync threads #89

Merged
merged 2 commits into from
Sep 14, 2023
Merged

Client sync threads #89

merged 2 commits into from
Sep 14, 2023

Conversation

kazu-yamamoto
Copy link
Owner

This should fix #87.

@kazu-yamamoto
Copy link
Owner Author

@edsko Gentle ping. Do you settle down now?

Can we release a new major version after #89 and #90 are merged?

@kazu-yamamoto
Copy link
Owner Author

Background: http2 blocks the release of new http2-tls, http3 and h3spec now.

@edsko
Copy link
Contributor

edsko commented Sep 13, 2023

Yes, I'm back as of Monday evening (Wednesday morning now). I should have time to look at these today or tomorrow! You'll hear from me very soon :) Thanks :)

@edsko
Copy link
Contributor

edsko commented Sep 13, 2023

@kazu-yamamoto It seems that this doesn't solve the problem, when I remove my threadDelay workaround, I still get test failures. I will check wireshark to see if it's the same thing, will report back soon.

@edsko
Copy link
Contributor

edsko commented Sep 13, 2023

Ok, I looked at the Wireshark output. If I do not use my threadDelay workaround, then before this PR I see the final DATA frame after the GOAWAY frame. It is true that with this PR the GOAWAY frame is the final one, but that's because the final DATA frame is now gone entirely (rather than being output before the GOAWAY frame).

@kazu-yamamoto
Copy link
Owner Author

Uhhm.
I don't understand why the last DATA is missing.
waitCounter0 waits until all streaming threads are done.
At this time, the last DATA should be queued.
Then GOAWAY is queued.
I have no idea at this moment.

@edsko
Copy link
Contributor

edsko commented Sep 14, 2023

Ok, perhaps that's my side then. I will dig.

@kazu-yamamoto
Copy link
Owner Author

OK. I understand.
Each stream has a TBQueue.
GOAWAY can come even when the TBQueue is not empty.
So, the sender should call decCounter when it receives StreamingFinished.

@kazu-yamamoto
Copy link
Owner Author

I hope that 287e8f6 fixes your problem.

@edsko
Copy link
Contributor

edsko commented Sep 14, 2023

@kazu-yamamoto Well, good sir, I think you got it! The problem is non-deterministic so I can't be 100% sure, but I have one regression test which for whatever reason seemed to trigger it quite reliably; I ran it over and over again for about 10 minutes and every single run passed; then I ran my full QuickCheck-based test suite for 50 minutes and it also passed. Nice work!

@kazu-yamamoto kazu-yamamoto merged commit 6d7636f into master Sep 14, 2023
10 checks passed
@kazu-yamamoto kazu-yamamoto deleted the client-sync-threads branch September 14, 2023 22:08
@kazu-yamamoto
Copy link
Owner Author

Merged.
Thank you for your thorough testing!

Are there any issues before the next major release?

@edsko
Copy link
Contributor

edsko commented Sep 15, 2023

Are there any issues before the next major release?

None that I am currently aware of! There might be some memory leaks, I haven't gotten to the stress testing part of my library yet, but there are, they should not affect the API.

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.

GOAWAY frame can overtake final DATA frame
2 participants