Improve UdpSocket documentation

[Patiga]

I tried working with UdpSocket and ran into EINVAL errors with no clear indication of what causes the error. Also, it was uncharacteristically hard to figure this module out, compared to other Rust std modules.

1. send and send_to return a usize This one is just clarity. Usually, returned usizes indicate that the buffer might have only been sent partially. This is not the case with UDP. Since that usize must always be buffer.len(), I have documented that.

2. bind limits connect and send_to When you bind to a limited address space like localhost, you can only connect to addresses in that same address space. Error kind: AddrNotAvailable.

3. connecting to localhost locks you to localhost On Linux, if you first connect to localhost, subsequent connects to
   non-localhost addresses fail. Error kind: InvalidInput.

For debugging the third one, it was really hard to find someone else who already had that problem. I only managed to find this thread: https://www.mail-archive.com/[email protected]/msg159519.html

[rustbot]

r? @joshtriplett

(rustbot has picked a reviewer for you, use r? to override)

[rustbot]

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

• Stabilizing library features
• Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
• Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
• Changing public documentation in ways that create new stability guarantees
• Changing observable runtime behavior of library APIs

[workingjubilee]

Thank you for your PR and sorry for the delay on review!

r? @workingjubilee

[bors]

☔ The latest upstream changes (presumably #111710) made this pull request unmergeable. Please resolve the merge conflicts.

[Patiga]

@workingjubilee Sorry for the delay on my part now. To be honest, I mostly lost my attachment/motivation on the pull request after it was idle for half a year. Finally found some motivation to just finish this up :)
Would it have been possible to make the bot assign me another reviewer? After one month and due to the reviewer's inactiveness in the past, I mostly lost hope on getting a response. Maybe a reassignment after a significant amount of time could be done automatically?
Thanks a lot for the response and feedback you gave, though, I really appreciated that! (although I failed to follow up on it)

[workingjubilee]

Hm? My intention was to finish this as the reviewer in lieu of the originally assigned reviewer (though I'm hardly perfect on my response times, either), though I'm happy to reroll for another reviewer from the library team if you would prefer. Reviewers can get rerolled from a team by asking for r? and then the name of a team. As I understand it, libs is currently working on improving processes for faster turnaround times on things, onboarding new members, etc.

[rustbot]

Failed to set assignee to and: invalid assignee

Note: Only org members, users with write permissions, or people who have commented on the PR may be assigned.

[workingjubilee]

To be honest, I mostly lost my attachment/motivation on the pull request after it was idle for half a year. Finally found some motivation to just finish this up :)

Also completely understand this, a little too well sometimes. ^^;

Hm, maybe I misread what you said and we were discussing hypotheticals instead of practicals, to answer that more directly:

Would it have been possible to make the bot assign me another reviewer?

Yes, I believe you essentially always had that power via the mechanism I just described.

Maybe a reassignment after a significant amount of time could be done automatically?

There's a slight risk at the moment, due to the kind of struggle to improve processes I just mentioned, that automatically rerolling PRs just gets you another reviewer who also is kinda overloaded. So I personally would hesitate to implement that in triagebot. When things are well and truly fucked, often human intervention is what's really needed. But it's not an awful idea.

[workingjubilee]

Almost, I dug a little deeper into one of the additions and discovered several horrifying truths about networking libraries and platforms. My apologies for not discovering them sooner.

[workingjubilee]

I doublechecked some stuff, and this needs to be corrected against the reality that we only will perform one send_to call and that the first thing we do is limit the buffer size sent to the maximum accepted by the platform: no reasonably-sized buffers will partially write. However, some platforms have a buffer limit smaller than usize, and in that case Rust will partially write.

This may be bad behavior on our part, feel free to open an issue if you think so!

https://github.com/rust-lang/rust/blob/b51bf3b880f05ff4a2554da5713091f988d32611/library/std/src/sys_common/net.rs#L548-L562

On Linux, the limit is size_t, and Linux will EMSGSIZE (which I don't think we have an ErrorKind for... maybe I should raise an issue about that) if the packet is too big to be sent "at once" according to MTU, so this is correct for Linux. It may be more useful to clarify that transmissions that exceed the regular datagram size of 65507 may be refused and that calling send_to with a size that may be higher than that requires being attentive to the Result, but that it won't be a partial write up to around i32::MAX.

[workingjubilee]

And it may seem unreasonable to hinge this on some pedantry: who the fuck is trying to push 2GiB buffers over UDP? But a network stack can do packet-splitting for UDP and thus accept and try to transmit huge buffers like that, and if someone is relying on "no partial writes" but they send, say, 3GiB, well... yeah.

And nowadays we have like 10Gb Ethernet, even for (higher-end) client hardware, and it's creeping up on 25Gb being common enough on dedicated NICs...

[Patiga]

This is fascinating and weird. Without data to back it up, I wouldn't expect any software to actually send packages large enough to be split up and handle the returned usize.

According to RFC 791 page 12, the size of a UDP packet is a 16-bit number, suggesting that packets larger than 2^16 (minus one) are impossible. There doesn't seem to be an extension to solve this. In IPv6, there are apparently jumbograms, but that seems to me like a whole other topic.

So I'm not sure if it is actually possible to send a UDP packet larger than 2^16, which then in turn would make partial write impossible.

[Patiga]

I am definitely not sure about anything currently, but maybe this part of the documentation (about the maximum package size, partial writes) should be at the top of the page with a link to the RFC, not mentioned at each send function variant.
Does that sound good?

[workingjubilee]

I think jumbograms de facto allow sending larger UDP packets? People do a lot of slightly-off-spec stuff in networking yet somehow it works out.

But yes, I am happy to have us properly link stuff instead. That makes it less likely this documentation falls afoul of my main concern: that a later spec could make it formally untrue, but no one can figure out it's wrong, so it remains taken as an authority.

[workingjubilee]

As send_to remarks, see:

https://github.com/rust-lang/rust/blob/b51bf3b880f05ff4a2554da5713091f988d32611/library/std/src/sys_common/net.rs#L686-L693

[Patiga]

Sorry for the holdup this time, I was afk for 2 weeks.

[workingjubilee]

Whee! Sorry about that. Thank you!
@bors r+

[bors]

📌 Commit adb22b9 has been approved by workingjubilee

It is now in the queue for this repository.

[bors]

⌛ Testing commit adb22b9 with merge 187b813...

[bors]

☀️ Test successful - checks-actions
Approved by: workingjubilee
Pushing 187b813 to master...

[rust-timer]

Finished benchmarking commit (187b813): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	1.4%	[0.7%, 2.0%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.0%	[-1.0%, -1.0%]	1
All ❌✅ (primary)	1.4%	[0.7%, 2.0%]	2

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 621.563s -> 622.162s (0.10%)
Artifact size: 272.01 MiB -> 272.03 MiB (0.01%)