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

Upgrade to Quinn 0.11 #238

Merged
merged 5 commits into from
May 20, 2024
Merged

Upgrade to Quinn 0.11 #238

merged 5 commits into from
May 20, 2024

Conversation

djc
Copy link
Contributor

@djc djc commented May 3, 2024

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 quinn-rs/quinn#1737.

@djc djc marked this pull request as draft May 3, 2024 15:39
@Ruben2424
Copy link
Contributor

i probably can take a look at the tests this weekend

@inflation
Copy link
Contributor

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

@djc
Copy link
Contributor Author

djc commented May 4, 2024

Yeah, I was testing on macOS.

Cargo.toml Outdated Show resolved Hide resolved
@Ruben2424
Copy link
Contributor

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
Copy link
Contributor Author

djc commented May 7, 2024

Yes, I only have client_close_only_on_last_sender_drop failing now.

@djc
Copy link
Contributor Author

djc commented May 7, 2024

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?

@djc djc marked this pull request as ready for review May 7, 2024 15:55
h3/Cargo.toml Show resolved Hide resolved
@seanmonstar
Copy link
Member

I'm not entirely sure, git blame says it was added in 64bea29.

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

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
Copy link
Contributor Author

djc commented May 16, 2024

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

@seanmonstar
Copy link
Member

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

@Ruben2424
Copy link
Contributor

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

@djc
Copy link
Contributor Author

djc commented May 16, 2024

@Ruben2424 thanks!

@Ruben2424
Copy link
Contributor

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

};

let server_fut = async {
let conn = server.next().await;
//= https://www.rfc-editor.org/rfc/rfc9114#section-4.2.2
//= type=test
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why remove this comment? It's used for RFC coverage.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if I am correct this does not test the server limit. It tests that our client does not send any requests that extend the server limit.

@Ruben2424 Ruben2424 merged commit 0561867 into hyperium:master May 20, 2024
10 checks passed
@djc
Copy link
Contributor Author

djc commented May 20, 2024

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

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.

None yet

5 participants