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

Revise and add additional 0-rtt doc comments #1826

Merged
merged 2 commits into from
Jul 1, 2024

Conversation

gretchenfrage
Copy link
Contributor

I had a hard time getting 0-RTT actually working in practice. It escalated to me forking rustls so I could put in a bunch of print statements. Although I did find the 0-rtt tests, there were some subtle enough pitfalls that I didn't notice the relevant ways my attempts were diverging.

In hopes of improving understandability and discoverability of this, this PR adds notes about these pitfalls to some of the doc comments, and also largely rewrites the doc comment for Connecting::into_0rtt to make its behavior more clear, especially across various edge cases.

This PR does not change any code or APIs.

@gretchenfrage gretchenfrage marked this pull request as ready for review April 16, 2024 05:01
Copy link
Member

@djc djc left a comment

Choose a reason for hiding this comment

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

Thanks for fleshing out these docs, great idea!

I have lots of little feedback about how we should document this stuff, but would be happy to get something like this merged.

quinn-proto/src/config.rs Outdated Show resolved Hide resolved
quinn-proto/src/config.rs Outdated Show resolved Hide resolved
quinn-proto/src/config.rs Outdated Show resolved Hide resolved
quinn/src/connection.rs Outdated Show resolved Hide resolved
quinn/src/connection.rs Outdated Show resolved Hide resolved
quinn/src/connection.rs Outdated Show resolved Hide resolved
quinn/src/connection.rs Outdated Show resolved Hide resolved
quinn/src/send_stream.rs Outdated Show resolved Hide resolved
@gretchenfrage
Copy link
Contributor Author

Thanks for the feedback! Happy to workshop this into something we both think is good.

@gretchenfrage
Copy link
Contributor Author

Thanks to some other changes that have been made to the code base since this was last reviewed, some of the "where should this be documented" questions sort of solved themselves. I've shuffled things around, changed some things, and tried to make sure the current revision reflects the feedback given.

Current change set:

  • Rustls-specific documentation on crypto::rustls::ServerConfig and crypto::rustls::ClientConfig
  • Non-Rustls-specific documentation Connection::into_0rtt
  • A warning regarding latency on SendStream::stopped

Thanks!

@gretchenfrage gretchenfrage requested a review from djc June 14, 2024 23:01
Copy link
Member

@djc djc left a comment

Choose a reason for hiding this comment

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

This looks great, thanks for spending time on this!

Copy link
Collaborator

@Ralith Ralith left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

quinn-proto/src/crypto/rustls.rs Outdated Show resolved Hide resolved
quinn/src/connection.rs Outdated Show resolved Hide resolved
quinn/src/connection.rs Outdated Show resolved Hide resolved
quinn/src/connection.rs Outdated Show resolved Hide resolved
quinn/src/connection.rs Outdated Show resolved Hide resolved
@gretchenfrage
Copy link
Contributor Author

Thanks for the feedback! All comments should be addressed.

quinn-proto/src/crypto/rustls.rs Outdated Show resolved Hide resolved
@gretchenfrage gretchenfrage force-pushed the 0rtt-docs-changes branch 3 times, most recently from ea662e0 to 62e1db0 Compare July 1, 2024 04:53
quinn-proto/src/crypto/rustls.rs Show resolved Hide resolved
@Ralith Ralith merged commit 02ed621 into quinn-rs:main Jul 1, 2024
8 checks passed
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.

4 participants