Add setter and getter for TCP_QUICKACK on TcpStream for Linux #96324

berendjan · 2022-04-22T19:42:51Z

Reference issue #96256

Setting TCP_QUICKACK on TcpStream for Linux

rust-highfive · 2022-04-22T19:42:54Z

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 r? rust-lang/libs-api @rustbot label +T-libs-api -T-libs to request review from a libs-api team reviewer. 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

rust-highfive · 2022-04-22T19:42:55Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @kennytm (or someone else) soon.

Please see the contribution instructions for more information.

berendjan · 2022-04-22T19:45:39Z

r? rust-lang/libs-api @rustbot label +T-libs-api -T-libs

the8472 · 2022-04-23T12:28:28Z

Since this is linux-specific I think it should be an extension trait, similar to os::linux::fs::MetadataExt, albeit as a sealed trait and under os::linux::net instead.

berendjan · 2022-04-23T14:23:53Z

Agreed, however I tried that approach but ran into the problem that the Socket in TcpStream is private. How do i solve this without creating 2 methods (get/set) for TcpStream?

the8472 · 2022-04-23T15:51:47Z

There's the internal trait sys_common::AsInner to go from the public to the inner structs.

Edit: sorry, clicked the wrong button

berendjan · 2022-04-23T17:21:53Z

Ah okay, thank you! Thanks for the tip! Ill update the PR. On 23 Apr 2022, at 17:52, the8472 ***@***.***> wrote: There's the internal trait sys_common::AsInner to go from the public to the inner structs. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.

berendjan · 2022-05-02T16:55:43Z

@the8472 hi, I finally got around to updating the PR, please let me know what I can improve!

the8472 · 2022-05-02T17:46:33Z

library/std/src/os/linux/net.rs

+    ///         .expect("Couldn't connect to the server...");
+    /// stream.set_quickack(true).expect("set_quickack call failed");
+    /// assert_eq!(stream.quickack().unwrap_or(false), true);
+    /// ```


Since these tests are no_run it would be good to have actual tests in a test module.

can I also just remove the no_run?

I don't know how rustdoc tests interact with cfgs and PR CI only runs linux tests, so it were to fail it would likely only fail after a merge.

But if you have some non-linux machine you could try and see what happens there?

ah yes it failed already, I found one test for the nodelay and adapted it for quickack. Hope this works

the8472 · 2022-05-02T17:49:10Z

library/std/src/sys_common/net.rs

+    pub fn quickack(&self) -> io::Result<bool> {
+        self.inner.quickack()
+    }
+


The sys_common module isn't supposed to contain code behind platform-specifc cfgs, instead the linux::net module should do enough unwrapping steps to get to the os::unix types directly.

I added a to_inner method to expose the Socket

berendjan · 2022-08-13T16:41:03Z

FYI I added doc to the cfg include for net module (std/src/os/mod.rs) and AsInner to imports (std/src/sys_common/net.rs)

berendjan · 2022-08-21T19:01:37Z

@Dylan-DPC I know it's been a while and I should have updated sooner, however is it possible to try the roll up again? I added doc to std/src/os/mod.rs, I figured that was the problem since it couldn't find during doc generation.

Dylan-DPC · 2022-08-22T05:35:31Z

i think it is better to get a final review from the reviewer first

dtolnay · 2022-08-26T22:17:11Z

@bors r+ rollup=iffy

bors · 2022-08-26T22:17:12Z

📌 Commit 786e875 has been approved by dtolnay

It is now in the queue for this repository.

bors · 2022-08-27T20:03:42Z

⌛ Testing commit 786e875 with merge 9f32407fb5588105af3dff88fa37a0aa5a7a66ea...

bors · 2022-08-28T00:18:42Z

💥 Test timed out

rust-log-analyzer · 2022-08-28T01:43:36Z

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

berendjan · 2022-08-28T07:10:23Z

@dtolnay I don't understand the error, what happend?

dtolnay · 2022-08-28T10:35:21Z

Not sure. @bors retry

bors · 2022-08-28T12:26:40Z

⌛ Testing commit 786e875 with merge ee285ea...

bors · 2022-08-28T15:07:38Z

☀️ Test successful - checks-actions
Approved by: dtolnay
Pushing ee285ea to master...

rust-timer · 2022-08-29T01:29:06Z

Finished benchmarking commit (ee285ea): 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)	-	-	0
Regressions ❌ (secondary)	5.1%	[2.7%, 6.8%]	3
Improvements ✅ (primary)	-2.3%	[-2.3%, -2.3%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-2.3%	[-2.3%, -2.3%]	1

Cycles

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)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.1%	[-2.1%, -2.1%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-2.1%	[-2.1%, -2.1%]	1

the arithmetic mean of the percent change ↩ ↩²
number of relevant changes ↩ ↩²

rust-highfive assigned kennytm Apr 22, 2022

rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Apr 22, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 22, 2022

rustbot added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Apr 22, 2022

rust-highfive assigned dtolnay and unassigned kennytm Apr 22, 2022

rustbot removed the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Apr 22, 2022

This comment has been minimized.

Sign in to view

the8472 closed this Apr 23, 2022

the8472 reopened this Apr 23, 2022

dtolnay changed the title ~~Branch for Issue #96256 setting TCP_QUICKACK~~ Add setter and getter for TCP_QUICKACK on TcpStream for Linux and Android Apr 27, 2022

dtolnay 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 Apr 27, 2022

This comment has been minimized.

Sign in to view

berendjan changed the title ~~Add setter and getter for TCP_QUICKACK on TcpStream for Linux and Android~~ Add setter and getter for TCP_QUICKACK on TcpStream for Linux May 1, 2022

This comment has been minimized.

Sign in to view

the8472 reviewed May 2, 2022

View reviewed changes

dtolnay approved these changes Aug 26, 2022

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 26, 2022

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 28, 2022

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 28, 2022

joshtriplett approved these changes Aug 28, 2022

View reviewed changes

bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 28, 2022

bors merged commit ee285ea into rust-lang:master Aug 28, 2022

rustbot added this to the 1.65.0 milestone Aug 28, 2022

This was referenced Aug 28, 2022

Add IsTerminal trait to determine if a descriptor or handle is a terminal #98033

Merged

Make std::os::fd public. #98368

Merged

Rollup of 8 pull requests #101124

Closed

berendjan deleted the set_tcp_quickack branch August 29, 2022 18:50

berendjan restored the set_tcp_quickack branch August 29, 2022 18:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add setter and getter for TCP_QUICKACK on TcpStream for Linux #96324

Add setter and getter for TCP_QUICKACK on TcpStream for Linux #96324

berendjan commented Apr 22, 2022 •

edited

Loading

rust-highfive commented Apr 22, 2022

rust-highfive commented Apr 22, 2022

berendjan commented Apr 22, 2022

This comment has been minimized.

the8472 commented Apr 23, 2022

berendjan commented Apr 23, 2022

the8472 commented Apr 23, 2022 •

edited

Loading

berendjan commented Apr 23, 2022 via email

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

berendjan commented May 2, 2022

the8472 May 2, 2022

berendjan May 2, 2022

the8472 May 2, 2022

berendjan May 2, 2022

the8472 May 2, 2022

berendjan May 3, 2022

berendjan commented Aug 13, 2022

berendjan commented Aug 21, 2022

Dylan-DPC commented Aug 22, 2022

dtolnay commented Aug 26, 2022

bors commented Aug 26, 2022

bors commented Aug 27, 2022

bors commented Aug 28, 2022

rust-log-analyzer commented Aug 28, 2022

berendjan commented Aug 28, 2022

dtolnay commented Aug 28, 2022

bors commented Aug 28, 2022

bors commented Aug 28, 2022

rust-timer commented Aug 29, 2022

Add setter and getter for TCP_QUICKACK on TcpStream for Linux #96324

Add setter and getter for TCP_QUICKACK on TcpStream for Linux #96324

Conversation

berendjan commented Apr 22, 2022 • edited Loading

rust-highfive commented Apr 22, 2022

rust-highfive commented Apr 22, 2022

berendjan commented Apr 22, 2022

This comment has been minimized.

the8472 commented Apr 23, 2022

berendjan commented Apr 23, 2022

the8472 commented Apr 23, 2022 • edited Loading

berendjan commented Apr 23, 2022 via email

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

berendjan commented May 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

berendjan commented Aug 13, 2022

berendjan commented Aug 21, 2022

Dylan-DPC commented Aug 22, 2022

dtolnay commented Aug 26, 2022

bors commented Aug 26, 2022

bors commented Aug 27, 2022

bors commented Aug 28, 2022

rust-log-analyzer commented Aug 28, 2022

berendjan commented Aug 28, 2022

dtolnay commented Aug 28, 2022

bors commented Aug 28, 2022

bors commented Aug 28, 2022

rust-timer commented Aug 29, 2022

Overall result: no relevant changes - no action needed

Footnotes

berendjan commented Apr 22, 2022 •

edited

Loading

the8472 commented Apr 23, 2022 •

edited

Loading