search

quinn-rs / quinn

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

Abstract runtime support #1364

Closed Ralith closed 2 years ago

Ralith commented 2 years ago

Fixes #502. Draws very heavily on prior work in https://github.com/quinn-rs/quinn/pull/1346. I haven't intensively reviewed all of the inherited code yet, and perhaps there's opportunity to break things up further.

@yu-re-ka I'd like your review on this, if you don't mind? Especially the async-std stuff which I was mostly guessing on.

yu-re-ka commented 2 years ago

LGTM. Only comment I want to make is that all errors are assumed to be transient errors. This was already the case before and is probably fine, but in the async-std case it may be possible to explicitly handle transient errors and bubble other errors upwards.

Ralith commented 2 years ago

Thanks! Propagating UDP errors is possible, but we're not aware of any cases where it's actually appropriate to do so, and in some cases it actually exposes you to denial-of-service attacks.

DemiMarie commented 2 years ago

Thanks! Propagating UDP errors is possible, but we're not aware of any cases where it's actually appropriate to do so, and in some cases it actually exposes you to denial-of-service attacks.

Is that true in all cases? At the very least I would like to be able to log these errors, as otherwise a firewall blocking traffic is indistinguishable from a timeout. Also, some errors (such as EPERM on Linux) are a result of the kernel blocking the traffic deliberately and so are not temporary.

Ralith commented 2 years ago

I have personally observed EPERM on Linux manifesting transiently. This is also beyond the scope of this PR.

yu-re-ka commented 2 years ago

fe93a681081952bcb2515c791bdf3204d0061139

  Time (mean ± σ):      1.387 s ±  0.065 s    [User: 1.689 s, System: 0.767 s]
  Range (min … max):    1.236 s …  1.532 s    50 runs

59285257dfe3161d9c04930d9db2fb3a6a444463

  Time (mean ± σ):      1.384 s ±  0.048 s    [User: 1.683 s, System: 0.772 s]
  Range (min … max):    1.242 s …  1.495 s    50 runs
elenaf9 commented 2 years ago

Hi, would you mind releasing a new version of quinn-udp that includes this change? We are currently using quinn-proto with UdpSocket from std::net. Would love to switch to quinn-udp sockets for the ECN support, however since we support multiple runtimes (async-std and tokio) we depend on the changes of this PR.

Ralith commented 2 years ago

Sure! I've published quinn-udp 0.2.

elenaf9 commented 2 years ago

Thanks!