hyperium / h3

MIT License
575 stars 75 forks source link

Upgrade to Quinn 0.11 #238

Closed djc closed 1 month ago

djc commented 2 months ago

This is still failing some tests -- would appreciate some help with addressing these.

control_close_send_error fails for me on master, too.

We plan to release 0.11 soon, see https://github.com/quinn-rs/quinn/issues/1737.

Ruben2424 commented 2 months ago

i probably can take a look at the tests this weekend

inflation commented 2 months ago

The same error used to happen in particularly macOS a lot.

djc commented 2 months ago

Yeah, I was testing on macOS.

Ruben2424 commented 2 months ago

I fixed some race conditions in the failing tests. Does control_close_send_error now work on macOS? The test tests::connection::client_close_only_on_last_sender_drop still fails. I couldn't fix it without understanding what exactly it tests. Does it test anything? Can we remove it?

djc commented 2 months ago

Yes, I only have client_close_only_on_last_sender_drop failing now.

djc commented 2 months ago

I squashed some intermediate commits and switched to the published versions of Quinn crates.

The test tests::connection::client_close_only_on_last_sender_drop still fails. I couldn't fix it without understanding what exactly it tests. Does it test anything? Can we remove it?

@seanmonstar are you the person who has more context on this?

seanmonstar commented 2 months ago

I'm not entirely sure, git blame says it was added in https://github.com/hyperium/h3/commit/64bea294977716774ac2800d1a26a71149ad4398.

One option is to comment it out with a note, and if we notice something wrong related to closure, then we can try to figure out what this one was checking specifically.

Ruben2424 commented 2 months ago

One option is to comment it out with a note, and if we notice something wrong related to closure, then we can try to figure out what this one was checking specifically.

I think the failure has nothing todo with the purpose of this test. It is also a race condition. The client future finishes so fast, that the server cannot initialize the connection fully before the client closes the connection. Then this unwrap() panics let mut incoming = server::Connection::new(conn).await.unwrap();.

We could also do a recv_response().await on the client request streams. But for me it seams that maybe they were intentionally omitted? idk?

djc commented 2 months ago

@seanmonstar friendly ping? This is blocking upgrading hickory-dns to latest rustls/quinn etc, so would be nice to get it released.

seanmonstar commented 2 months ago

I don't have anything else. You could comment out the test, or explore what Ruben said.

Ruben2424 commented 2 months ago

I can try to finnish the test (or comment it out) this weekend. After a review i can prepare releases.

djc commented 2 months ago

@Ruben2424 thanks!

Ruben2424 commented 2 months ago

The tests are now successful. @seanmonstar do you mind doing a review?

djc commented 1 month ago

Thanks for the reviews/fixes, looking forward to the release!