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.

Sign up for GitHub

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

Jump to bottom

Abstract runtime support #1364

Merged
merged 4 commits into from
Jul 5, 2022
Merged

Abstract runtime support #1364

merged 4 commits into from
Jul 5, 2022

Conversation

Ralith
Copy link
Collaborator

@Ralith Ralith commented Jun 3, 2022 •
edited
Loading

Fixes #502. Draws very heavily on prior work in #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.

@Ralith Ralith mentioned this pull request Jun 3, 2022
Closed
@Ralith Ralith force-pushed the abstract-runtime branch 2 times, most recently from af79dbe to 1c37e94 Compare June 3, 2022 04:37
@Ralith Ralith mentioned this pull request Jun 3, 2022
Closed
@Ralith Ralith force-pushed the abstract-runtime branch 2 times, most recently from 2bb0e37 to 006108e Compare June 3, 2022 04:48
@Ralith Ralith requested a review from djc June 15, 2022 20:27
@yu-re-ka
Copy link

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
Copy link
Collaborator Author

Ralith commented Jun 16, 2022

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.

yu-re-ka
yu-re-ka previously approved these changes Jun 16, 2022
View reviewed changes
@djc djc mentioned this pull request Jun 20, 2022
Closed
@Ralith Ralith mentioned this pull request Jun 27, 2022
Open
@DemiMarie
Copy link

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
Copy link
Collaborator Author

Ralith commented Jun 28, 2022

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

This was referenced Jul 1, 2022
Closed
Merged
@Ralith Ralith force-pushed the abstract-runtime branch 5 times, most recently from 79156d9 to f30eb43 Compare July 3, 2022 17:54
@Ralith Ralith mentioned this pull request Jul 3, 2022
Closed
@Ralith Ralith force-pushed the abstract-runtime branch from f30eb43 to 6d7b420 Compare July 3, 2022 21:17
djc
djc reviewed Jul 4, 2022
View reviewed changes
quinn/src/endpoint.rs Outdated Show resolved Hide resolved
quinn/src/endpoint.rs Outdated Show resolved Hide resolved
quinn/src/endpoint.rs Outdated Show resolved Hide resolved
quinn/src/endpoint.rs Outdated Show resolved Hide resolved
@Ralith Ralith force-pushed the abstract-runtime branch from 6d7b420 to 5928525 Compare July 5, 2022 00:41
djc
djc approved these changes Jul 5, 2022
View reviewed changes
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.

I forget, did we review benchmark impact for these changes?

@yu-re-ka
Copy link

yu-re-ka commented Jul 5, 2022

fe93a68

  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

5928525

  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

@Ralith Ralith merged commit 18f00de into main Jul 5, 2022
@Ralith Ralith deleted the abstract-runtime branch July 5, 2022 16:06
@elenaf9
Copy link

elenaf9 commented Sep 22, 2022

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
Copy link
Collaborator Author

Ralith commented Sep 22, 2022

Sure! I've published quinn-udp 0.2.

@elenaf9
Copy link

elenaf9 commented Sep 22, 2022

Thanks!

@djc djc mentioned this pull request Oct 26, 2022
Closed
mxinden added a commit to mxinden/quinn that referenced this pull request Feb 24, 2023
@mxinden
docs(quinn): remove restriction to tokio
7df26f3 
With quinn-rs#1364 `quinn` is no longer restricted
to `tokio`. Commit updates the crate heading accordingly.
@mxinden mxinden mentioned this pull request Feb 24, 2023
Merged
Ralith pushed a commit that referenced this pull request Feb 24, 2023
@mxinden @Ralith
docs(quinn): remove restriction to tokio
c17315f 
With #1364 `quinn` is no longer restricted
to `tokio`. Commit updates the crate heading accordingly.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@djc djc djc approved these changes

@yu-re-ka yu-re-ka yu-re-ka left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

async_std support
5 participants
@Ralith @yu-re-ka @DemiMarie @elenaf9 @djc