Make TCP connect handle EINTR correctly #116132

darthunix · 2023-09-25T03:04:02Z

According to the POSIX standard, if connect() is interrupted by a signal that is caught while blocked waiting to establish a connection, connect() shall fail and set errno to EINTR, but the connection request shall not be aborted, and the connection shall be established asynchronously. When the connection has been established asynchronously, select() and poll() shall indicate that the file descriptor for the socket is ready for writing.

The previous implementation differs from the recomendation: in a case of the EINTR we tried to reconnect in a loop and sometimes get EISCONN error (this problem was originally detected on MacOS).

More details about the problem in an article.
The original issue.

rustbot · 2023-09-25T03:04:09Z

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

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

@rustbot author: the review is finished, PR author should check the comments and take action accordingly
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

the8472 · 2023-09-25T12:32:18Z

You'll have to split that based on platform and move it to the appropriate submodules in sys.

darthunix · 2023-10-04T16:38:20Z

After some research (linux/darwin-xnu sources, golang connect) I think it is safe to call connect() in a loop as we do now. The only thing we should do is to handle EISCONN error when connect() was interrupted by a signal and connected asynchronously before the next loop iteration (as it often happens on MacOS).

the8472 · 2023-10-04T17:04:53Z

You shouldn't change cvt_r though since it's also used in other places which don't care about EISCONN.

darthunix · 2023-10-06T07:39:10Z

@the8472 can you take a look at the current implementation? I have no experience with hermit and soild os, so I've left everything as it (for solid os cvt_r is equal to cvt, so I've made a small refactoring)

workingjubilee · 2023-10-11T20:11:31Z

cc @kawadakk for Solid
cc @stlankes @mkroening for Hermit
Your review isn't required, strictly speaking, but you might want to be aware.

mkroening

Thanks for the notification! :)

The Hermit changes do not compile.

mkroening · 2023-10-12T09:57:38Z

library/std/src/sys/hermit/net.rs

@@ -56,6 +56,11 @@ impl Socket {
        unimplemented!()
    }

+    pub fn connect(&self, addr: &SocketAddr) -> io::Result<()> {
+        let (addr, len) = addr.into_inner();
+        cvt_r(unsafe { netc::connect(self.as_raw_fd(), addr.as_ptr(), len) })


This does not compile.

Suggested change

cvt_r(unsafe { netc::connect(self.as_raw_fd(), addr.as_ptr(), len) })

cvt_r(|| unsafe { netc::connect(self.as_raw_fd(), addr.as_ptr(), len) })?;

Ok(())

Thanks for this catch. I have originally used cvt instead of the cvt_r here, but then realized that I don't know anything about hermit os and whether it can interrupt connect by a signal or not. So, finally I decided to leave everything as it was before. BTW, is there any documentation to read about connect on hermit?

is there any documentation to read about connect on hermit?

I am afraid not. There's the function definition in hermit_abi::connect and there is the actual implementation. It's not documented, though, and may change in the future. Currently, we mimic BSD sockets.

mkroening

Compiles for Hermit now, thanks. :)

kawadakk

Thank you for heads-up!

kawadakk · 2023-10-13T09:36:01Z

library/std/src/sys/solid/net.rs

@@ -233,12 +233,14 @@ impl Socket {
        }
    }

+    pub fn connect(&self, addr: &SocketAddr) -> io::Result<()> {
+        let (addr, len) = addr.into_inner();
+        cvt(unsafe { netc::connect(self.0.raw(), addr.as_ptr(), len) })


Suggested change

cvt(unsafe { netc::connect(self.0.raw(), addr.as_ptr(), len) })

cvt(unsafe { netc::connect(self.0.raw(), addr.as_ptr(), len) })?;

Ok(())

According to the POSIX standard, if connect() is interrupted by a signal that is caught while blocked waiting to establish a connection, connect() shall fail and set errno to EINTR, but the connection request shall not be aborted, and the connection shall be established asynchronously. If asynchronous connection was successfully established after EINTR and before the next connection attempt, OS returns EISCONN that was handled as an error before. This behavior is fixed now and we handle it as success. The problem affects MacOS users: Linux doesn't return EISCONN in this case, Windows connect() can not be interrupted without an old-fashoin WSACancelBlockingCall function that is not used in the library. So current solution gives connect() as OS specific implementation.

kawadakk

This compiles for SOLID targets now. Thanks!

cuviper · 2023-10-18T23:33:44Z

@bors r+

bors · 2023-10-18T23:33:46Z

📌 Commit dfadd17 has been approved by cuviper

It is now in the queue for this repository.

Make TCP connect handle EINTR correctly According to the [POSIX](https://pubs.opengroup.org/onlinepubs/009695399/functions/connect.html) standard, if connect() is interrupted by a signal that is caught while blocked waiting to establish a connection, connect() shall fail and set errno to EINTR, but the connection request shall not be aborted, and the connection shall be established asynchronously. When the connection has been established asynchronously, select() and poll() shall indicate that the file descriptor for the socket is ready for writing. The previous implementation differs from the recomendation: in a case of the EINTR we tried to reconnect in a loop and sometimes get EISCONN error (this problem was originally detected on MacOS). 1. More details about the problem in an [article](http://www.madore.org/~david/computers/connect-intr.html). 2. The original [issue](https://git.picodata.io/picodata/picodata/tarantool-module/-/issues/157).

Rollup of 4 pull requests Successful merges: - rust-lang#116037 (Add `-Zstack-protector` test for Windows targets) - rust-lang#116132 (Make TCP connect handle EINTR correctly) - rust-lang#116810 (Add FileCheck annotations to mir-opt tests.) - rust-lang#116896 (Only check in a single place if a pass is enabled.) r? `@ghost` `@rustbot` modify labels: rollup

bors · 2023-10-19T11:22:31Z

⌛ Testing commit dfadd17 with merge 3fbcfd2...

bors · 2023-10-19T13:24:32Z

☀️ Test successful - checks-actions
Approved by: cuviper
Pushing 3fbcfd2 to master...

rust-timer · 2023-10-19T15:56:20Z

Finished benchmarking commit (3fbcfd2): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.2%	[2.2%, 2.2%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

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

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

Binary size

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

Bootstrap: 629.346s -> 628.089s (-0.20%)
Artifact size: 303.99 MiB -> 304.03 MiB (0.02%)

rustbot assigned cuviper Sep 25, 2023

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 Sep 25, 2023

This comment has been minimized.

Sign in to view

darthunix force-pushed the connect_poll branch from 4be1dfb to f1136b7 Compare September 26, 2023 17:00

darthunix force-pushed the connect_poll branch from f1136b7 to e668eb3 Compare October 4, 2023 15:18

rustbot added the O-unix Operating system: Unix-like label Oct 4, 2023

darthunix force-pushed the connect_poll branch from e668eb3 to 022abe6 Compare October 4, 2023 16:34

darthunix force-pushed the connect_poll branch from 022abe6 to 1714afa Compare October 5, 2023 09:16

rustbot added O-hermit Operating System: Hermit O-solid Operating System: SOLID O-windows Operating system: Windows labels Oct 5, 2023

darthunix marked this pull request as ready for review October 5, 2023 09:38

darthunix force-pushed the connect_poll branch from 1714afa to 4a10d17 Compare October 6, 2023 07:34

mkroening suggested changes Oct 12, 2023

View reviewed changes

darthunix force-pushed the connect_poll branch from 4a10d17 to a3d422a Compare October 13, 2023 07:11

mkroening approved these changes Oct 13, 2023

View reviewed changes

kawadakk suggested changes Oct 13, 2023

View reviewed changes

darthunix force-pushed the connect_poll branch from a3d422a to dfadd17 Compare October 13, 2023 11:13

kawadakk approved these changes Oct 16, 2023

View reviewed changes

bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 18, 2023

bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Oct 18, 2023

fmease mentioned this pull request Oct 19, 2023

Rollup of 4 pull requests #116924

Closed

bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 19, 2023

bors merged commit 3fbcfd2 into rust-lang:master Oct 19, 2023
12 checks passed

rustbot added this to the 1.75.0 milestone Oct 19, 2023

bors mentioned this pull request Oct 19, 2023

kmc-solid: I/O safety #115159

Merged

github-actions bot mentioned this pull request Oct 31, 2023

Automated pull from upstream master ferrocene/ferrocene#74

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make TCP connect handle EINTR correctly #116132

Make TCP connect handle EINTR correctly #116132

darthunix commented Sep 25, 2023 •

edited

rustbot commented Sep 25, 2023

This comment has been minimized.

the8472 commented Sep 25, 2023

darthunix commented Oct 4, 2023

the8472 commented Oct 4, 2023

darthunix commented Oct 6, 2023

workingjubilee commented Oct 11, 2023 •

edited

mkroening left a comment

mkroening Oct 12, 2023

darthunix Oct 13, 2023

mkroening Oct 13, 2023

mkroening left a comment

kawadakk left a comment

kawadakk Oct 13, 2023

darthunix Oct 13, 2023

kawadakk left a comment

cuviper commented Oct 18, 2023

bors commented Oct 18, 2023

bors commented Oct 19, 2023

bors commented Oct 19, 2023

rust-timer commented Oct 19, 2023

	cvt_r(unsafe { netc::connect(self.as_raw_fd(), addr.as_ptr(), len) })
	cvt_r(\|\| unsafe { netc::connect(self.as_raw_fd(), addr.as_ptr(), len) })?;
	Ok(())

	cvt(unsafe { netc::connect(self.0.raw(), addr.as_ptr(), len) })
	cvt(unsafe { netc::connect(self.0.raw(), addr.as_ptr(), len) })?;
	Ok(())

Make TCP connect handle EINTR correctly #116132

Make TCP connect handle EINTR correctly #116132

Conversation

darthunix commented Sep 25, 2023 • edited

rustbot commented Sep 25, 2023

This comment has been minimized.

the8472 commented Sep 25, 2023

darthunix commented Oct 4, 2023

the8472 commented Oct 4, 2023

darthunix commented Oct 6, 2023

workingjubilee commented Oct 11, 2023 • edited

mkroening left a 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

mkroening left a comment

Choose a reason for hiding this comment

kawadakk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kawadakk left a comment

Choose a reason for hiding this comment

cuviper commented Oct 18, 2023

bors commented Oct 18, 2023

bors commented Oct 19, 2023

bors commented Oct 19, 2023

rust-timer commented Oct 19, 2023

Overall result: ❌ regressions - no action needed

darthunix commented Sep 25, 2023 •

edited

workingjubilee commented Oct 11, 2023 •

edited