Conversation
|
rustbot has assigned @Mark-Simulacrum. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
I think that the doc comments need editing for grammar and also to allow for cases where the Additionally, the API requires passing a Duration, but the documentation says
In practice I believe that the special value |
|
It's intentionally |
|
Another problem: the number of seconds passed also isn't a maximum either, it's a hint. For example, if you pass 8 seconds to this API, that will get calculated as 4 retransmits, which will wait up to 15 seconds worth of retransmits. (as of linux kernel tag v6.18 (2025-11-30) which is what I have easily available at the moment) This means that the duration can be significantly higher than the passed duration (up to 120 seconds longer in the most extreme cases I believe) or significantly lower (the duration is capped at 255 retransmits, which is about 29880 seconds or so if i'm doing the math correctly) |
| /// A socket listener will be awakened solely when data arrives. | ||
| /// | ||
| /// The `accept` argument set the maximum delay until the | ||
| /// data is available to read, reducing the number of short lived | ||
| /// connections without data to process. | ||
| /// Contrary to other platforms `SO_ACCEPTFILTER` feature equivalent, there is | ||
| /// no necessity to set it after the `listen` call. | ||
| /// Note that the delay is expressed as Duration from user's perspective | ||
| /// the call rounds it down to the nearest second expressible as a `c_int`. |
There was a problem hiding this comment.
| /// Configures `TCP_DEFER_ACCEPT` on the socket. | |
| /// | |
| /// This defers `accept()` yielding an incoming TcpStream that has not received any data yet by | |
| /// `accept` Duration. | |
| /// | |
| /// Contrary to other platforms `SO_ACCEPTFILTER` feature equivalent, there is | |
| /// no necessity to set it after the `listen` call. | |
| /// | |
| /// Note that the delay is expressed as a `Duration`, but the underlying syscall requires | |
| /// seconds. Currently we round down to the nearest second expressible as a `c_int`. This may | |
| /// change in the future if the underlying API becomes more expressive. |
Proposal for updating the wording a bit to avoid some of the grammar issues (may need editing locally to adjust indentation etc).
|
I'll also nominate just to do a last check with libs-api on the comments from @asquared31415 above and some more thoughts as I look at this.
|
|
We discussed this during today's libs-api meeting. We want the
We'd like to see this addressed too - it was in fact news to us since it's not mentioned on the manpage. Since the logic first rounds down and then up again and since this is a performance optimization and not some hard timing guarantee we didn't think it'd be worthwhile to spend too wording on the exact details and instead we could say something like these times are approximate and subject to both rounding by the standard library and the kernel.
We didn't discuss this during the meeting, but it seems reasonable to me . |
|
@asquared31415 I interpret this sentence on the manpage
as saying that this also caps the maximum number of retransmits. Does that mean that low values may make handshakes less reliable than the default system configuration? |
|
Correct, it bounds the maximum number of retransmits using an undocumented formula based on the input integer. I think that having this value set may cause the handshake to be "less reliable" in that under extreme conditions when many retransmits are needed, the connection may be dropped instead (if the maximum is reached). But that's the explicit tradeoff you opt in to when you use this option. |
Close #119639
FCP: #119639 (comment)