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

Replace Broadcast helper with tokio::sync::Notify #1264

Merged
merged 4 commits into from Dec 23, 2021
Merged

Replace Broadcast helper with tokio::sync::Notify #1264

merged 4 commits into from Dec 23, 2021

Conversation

Ralith
Copy link
Collaborator

@Ralith Ralith commented Dec 19, 2021

Drops a chunk of complex and under-audited synchronization code in favor of a similar, better-tested and more efficient primitive from tokio.

@Ralith Ralith changed the title Notify Replace Broadcast helper with tokio::sync::Notify Dec 19, 2021
@Ralith
Copy link
Collaborator Author

Ralith commented Dec 20, 2021

One subtler consequence of this is that we're going from returning a named, 'static future to an anonymous, self-borrowing one. I think that's okay, and more forwards-compatible in any case. It does make it more difficult to use with hand-written futures, however. If that's a concern long-term, we might consider exposing poll_foo(&self, cx: &mut Context) variants of everything; this is in any case not the first async method in quinn.

@daxpedda
Copy link
Contributor

I was hoping that eventually quinn will become executor independent, especially now that quinn-udp is external. I couldn't find a Notify equivalent in futures though. I guess this can also be dealt with later.

See #502.

@Ralith
Copy link
Collaborator Author

Ralith commented Dec 22, 2021

@daxpedda Not to worry, this would be utterly trivial to replace with futures_intrusive::ManualResetEvent or similar should the larger challenges of executor-independence be addressed. The only reason I didn't go with that immediately is to avoid the extra dependency, and due to some lurking soundness concerns in futures-intrusive.

quinn/src/connection.rs Outdated Show resolved Hide resolved
Drops a chunk of complex and under-audited synchronization code in
favor of a similar, better-tested and more efficient primitive from
tokio.
@djc djc merged commit e39f3f6 into main Dec 23, 2021
@djc djc deleted the notify branch December 23, 2021 09:56
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

3 participants