search

kazu-yamamoto / http2

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

Client sync threads #89

Closed kazu-yamamoto closed 1 year ago

kazu-yamamoto commented 1 year ago

This should fix #87.

kazu-yamamoto commented 1 year ago

@edsko Gentle ping. Do you settle down now?

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

kazu-yamamoto commented 1 year ago

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

edsko commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

kazu-yamamoto commented 1 year ago

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 commented 1 year ago

I hope that 287e8f6 fixes your problem.

edsko commented 1 year ago

@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 commented 1 year ago

Merged. Thank you for your thorough testing!

Are there any issues before the next major release?

edsko commented 1 year ago

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.