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

Named futures + replace NewConnection with Connection methods #1357

Merged
merged 5 commits into from
Sep 27, 2022
Merged

Conversation

Ralith
Copy link
Collaborator

@Ralith Ralith commented May 12, 2022

Experimenting with a return to named 'static futures, motivated by downstream use cases. See e.g. hyperium/h3#80.

Open questions:

  • Which interfaces specifically need to be named?
  • Is 'static needed, or just names?
  • Are GATs + TAIT close enough that none of this matters?

Also included is an idea to drop NewConnection in favor of unobtrusive async methods on Connection, bundled because they would have to be rewritten to match this PR's approach if it move forwards. I'd like feedback on this change before I invest the effort to actually remove NewConnection and update all the examples/docs.

@Ralith Ralith changed the title Named futures Named futures + replace NewConnection with Connection methods May 12, 2022
@Ralith Ralith mentioned this pull request May 22, 2022
quinn/src/connection.rs Show resolved Hide resolved
quinn/src/notify.rs Outdated Show resolved Hide resolved
quinn/src/notify.rs Outdated Show resolved Hide resolved
quinn/src/notify.rs Outdated Show resolved Hide resolved
quinn/src/notify.rs Outdated Show resolved Hide resolved
@djc
Copy link
Member

djc commented May 23, 2022

Sorry for the slow review...

@Ralith
Copy link
Collaborator Author

Ralith commented May 23, 2022

No worries; it's not particularly pressing, nor a rebase hazard.

@Ralith Ralith marked this pull request as ready for review September 18, 2022 19:37
@Ralith
Copy link
Collaborator Author

Ralith commented Sep 18, 2022

Still TODO: Actually getting rid of NewConnection, unless there's a compelling reason to keep it around.

@Ralith
Copy link
Collaborator Author

Ralith commented Sep 18, 2022

That seems to have panned out to a pleasant simplification in the tests/examples. Less unwrap, less nesting, and fewer intermediate variables all 'round.

@Ralith Ralith force-pushed the named-futures branch 5 times, most recently from 199c724 to 6f4a7be Compare September 18, 2022 20:26
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.

Nice work, and actually -100 lines!

quinn/src/connection.rs Outdated Show resolved Hide resolved
quinn/src/connection.rs Outdated Show resolved Hide resolved
quinn/src/connection.rs Show resolved Hide resolved
quinn/src/connection.rs Show resolved Hide resolved
quinn/src/connection.rs Show resolved Hide resolved
quinn/src/connection.rs 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 Show resolved Hide resolved
quinn/src/connection.rs Show resolved Hide resolved
@Ralith Ralith force-pushed the named-futures branch 2 times, most recently from f5fe239 to 2ee0aa4 Compare September 21, 2022 00:22
Reduces indirection and allows us to avoid cloning an `Arc` each time
one of the relevant events is waited on, and opens the door to
handwritten futures built on them.
@djc djc merged commit 6e4bcbb into main Sep 27, 2022
@djc djc deleted the named-futures branch September 27, 2022 09:08
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