Skip to content

std::net: update tcp deferaccept delay type to Duration. #140482

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
Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

std::net: update tcp deferaccept delay type to Duration. #140482

wants to merge 2 commits into from
+26 −17

Conversation

devnexen
Copy link
Contributor

See comment here.

@rustbot
Copy link
Collaborator

rustbot commented Apr 29, 2025

r? @ibraheemdev

rustbot has assigned @ibraheemdev.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Apr 29, 2025
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@devnexen devnexen force-pushed the tcp_deferaccept_toduration branch from b1c90c8 to 82a0b2a Compare April 29, 2025 18:28
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@devnexen devnexen force-pushed the tcp_deferaccept_toduration branch from 82a0b2a to 7a84725 Compare April 29, 2025 19:00
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@devnexen devnexen force-pushed the tcp_deferaccept_toduration branch from 7a84725 to 7cea902 Compare April 29, 2025 19:29
@ibraheemdev
Copy link
Member

r? libs for the (unstable) API change.

@rustbot rustbot assigned thomcc and unassigned ibraheemdev May 2, 2025
@thomcc
Copy link
Member

thomcc commented May 30, 2025

r? libs-api

@rustbot rustbot added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label May 30, 2025
@rustbot rustbot assigned joshtriplett and unassigned thomcc May 30, 2025
joboet
joboet reviewed Aug 29, 2025
View reviewed changes
joboet
joboet reviewed Aug 29, 2025
View reviewed changes
@devnexen devnexen force-pushed the tcp_deferaccept_toduration branch from 7cea902 to dafdcfa Compare August 30, 2025 05:32
@rustbot
Copy link
Collaborator

rustbot commented Aug 30, 2025

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@devnexen devnexen requested a review from joboet August 30, 2025 06:49
joboet
joboet reviewed Aug 30, 2025
View reviewed changes
@joboet joboet assigned joboet and unassigned joshtriplett Aug 30, 2025
joboet
joboet reviewed Aug 30, 2025
View reviewed changes
@devnexen devnexen force-pushed the tcp_deferaccept_toduration branch from dafdcfa to c0c9afe Compare August 30, 2025 08:34
joboet
joboet reviewed Aug 30, 2025
View reviewed changes
Copy link
Member

@joboet joboet left a comment •
edited by rustbot
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from the nit the implementation looks good to me. I'm nominating this for libs-api to discuss the overflow behaviour. It might arguably be better to use a saturating conversion on the seconds – deferring the ACK for less time than given is not really an error.

@rustbot label +I-libs-api-nominated

View changes since this review

@rustbot rustbot added the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Aug 30, 2025
@devnexen devnexen force-pushed the tcp_deferaccept_toduration branch from c0c9afe to a1096c6 Compare August 30, 2025 10:30
@dtolnay dtolnay mentioned this pull request Sep 16, 2025
Open
@Amanieu
Copy link
Member

Amanieu commented Sep 16, 2025

We discussed this in the @rust-lang/libs-api meeting again. The duration should be explicitly documented as the maximum duration to defer. This allows us to round it down as needed to address OS limitations. Therefore we shouldn't trigger an error for overflows and should instead just use INT_MAX as the time.

Amanieu
Amanieu reviewed Sep 16, 2025
View reviewed changes
library/std/src/os/net/linux_ext/tcp.rs
///
/// The `accept` argument set the delay in seconds until the
/// data is available to read, reducing the number of short lived
/// connections without data to process.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still says "the delay in seconds".

@Amanieu Amanieu removed the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Sep 16, 2025
@joboet
Copy link
Member

joboet commented Sep 16, 2025

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 16, 2025
@rustbot
Copy link
Collaborator

rustbot commented Sep 16, 2025

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@devnexen
Copy link
Contributor Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 16, 2025
Amanieu
Amanieu reviewed Sep 17, 2025
View reviewed changes
library/std/src/os/net/linux_ext/tcp.rs Outdated
fn set_deferaccept(&self, accept: u32) -> io::Result<()>;
fn set_deferaccept(&self, accept: Duration) -> io::Result<()>;

/// Gets the accept delay value (in seconds) of the `TCP_DEFER_ACCEPT` option.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Gets the accept delay value (in seconds) of the `TCP_DEFER_ACCEPT` option.
/// Gets the accept delay value of the `TCP_DEFER_ACCEPT` option.

@devnexen devnexen force-pushed the tcp_deferaccept_toduration branch from 13f8c1d to 05fecb3 Compare September 17, 2025 05:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Amanieu Amanieu Amanieu left review comments

@joboet joboet joboet left review comments

Assignees

@joboet joboet

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@devnexen @rustbot @rust-log-analyzer @ibraheemdev @thomcc @Amanieu @joboet @joshtriplett