search

quinn-rs / quinn

Async-friendly QUIC implementation in Rust
Apache License 2.0
3.85k stars 394 forks source link

set may_fragment based on whether setting IP_DONTFRAG fails with ENOPROTOOPT. #1626

Closed jieliangma closed 1 year ago

jieliangma commented 1 year ago

fix issue

Ralith commented 1 year ago

The reason we hesitated to add support for older macOS/iOS versions up front in #1547 is that platforms which may fragment packets must report this accurately in may_fragment(), so that PMTU discovery is correctly disabled. Because PMTU discovery is a desirable performance improvement in general, we must ensure that we do so only on these legacy platforms, without interfering with performance on modern Unixes.

In short, this PR can't be merged without adding dynamic iOS version detection logic to may_fragment(). For robustness, we should also only ignore the specific expected error, and only in environments specifically expected to produce it.

djc commented 1 year ago

In short, this PR can't be merged without adding dynamic iOS version detection logic to may_fragment(). For robustness, we should also only ignore the specific expected error, and only in environments specifically expected to produce it.

If we try to set IP_DONTFRAG in init(), I suppose we can store the may_fragment bit as state after that, so that we don't need to interrogate the socket for every call to may_fragment()?

Ralith commented 1 year ago

Good point, that's also a lot easier and less fragile than version checks. may_fragment should then probably be a method on UdpSocketState.

Ralith commented 1 year ago

Thanks for the revision! Please split the cosmetic refactoring commits out from the semantic changes so they can be easily reviewed.

jieliangma commented 1 year ago

Thanks for the revision! Please split the cosmetic refactoring commits out from the semantic changes so they can be easily reviewed.

done

djc commented 1 year ago

In short, this PR can't be merged without adding dynamic iOS version detection logic to may_fragment(). For robustness, we should also only ignore the specific expected error, and only in environments specifically expected to produce it.

It seems okay to me to set may_fragment based on whether setting IP_DONTFRAG fails with NOPROTOOPT, without separate implementations for FreeBSD or newer versions of the Apple OSes, do you see any reason?

Ralith commented 1 year ago

Yeah, that sounds good to me, since we're no longer relying on an independent judgement in may_fragment to line up.

djc commented 1 year ago

@jieliangma #1629 refactored part of this code a bit, should make the changes here easier but will require some rebasing or rework.

djc commented 1 year ago

Going to merge this and fix it up a little. Thanks @jieliangma!