quinn-rs / quinn

Async-friendly QUIC implementation in Rust
Apache License 2.0
3.76k stars 380 forks source link

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

Closed Ralith closed 2 years ago

Ralith commented 2 years ago

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

Ralith commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@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.