Implement blocking unnamed_socket #4072

tiif · 2024-12-04T16:25:42Z

Support blocking for socketpair and pipe.

tiif · 2024-12-04T17:27:58Z

Planning to open a minor clean-up PR to remove some unused variable here before getting back to this.

@rustbot label S-blocked

bors · 2024-12-05T17:37:42Z

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

src/shims/unix/unnamed_socket.rs

tiif · 2024-12-16T14:18:38Z

src/shims/unix/unnamed_socket.rs

+    let Some(self_ref) = weak_self_ref.upgrade() else {
+        // FIXME:  We should raise a deadlock error if the self_ref upgrade failed.
+        throw_unsup_format!("This will be a deadlock error in future")
+    };


I think this PR is big enough, so I decided to temporarily make this throws unsupported format.

I wanted to write a test for this, and potentially tweak the diagnostic a bit if it is too confusing (because of the reason stated here).

But I certainly won't mind putting it in this PR if anyone prefers.

tiif · 2024-12-16T14:20:27Z

@rustbot ready

oli-obk

just some nits, then this is ready, please apply and squash

src/shims/unix/unnamed_socket.rs

RalfJung · 2024-12-20T13:53:00Z

src/shims/unix/unnamed_socket.rs

+            // 0 bytes successfully read indicates end-of-file.
+            return ecx.return_read_success(ptr, &[], 0, &dest);
+        } else {
+            // Blocking socketpair with writer and empty buffer.


How do we know it is blocking?

If that's a precondition, please add an assert! further up this function.

Oh yea, I should add a check here. The general idea is in #4072 (comment), since readbuf.borrow().buf.is_empty() && self.is_nonblock is already handled in read, this is always blocking. It is also impossible to change an fd from blocking type to non-blocking type for now. But it is likely that I will move the checks around, I will add it if it is still necessary.

RalfJung · 2024-12-20T13:53:31Z

src/shims/unix/unnamed_socket.rs

+            return ecx.return_read_success(ptr, &[], 0, &dest);
+        } else {
+            // Blocking socketpair with writer and empty buffer.
+            let weak_self_ref = weak_self_ref.clone();


Why do you have to clone this reference? Seems like you should move it.

RalfJung · 2024-12-20T13:56:02Z

src/shims/unix/unnamed_socket.rs

-                }
-            }
+
+        if readbuf.borrow().buf.is_empty() && self.is_nonblock {


The separation of concerns between read and anonsocket_read is a bit unclear. Why is this one case handled here and everything else handled below?

I think the code would be easier to follow if you moved this logic into anonsocket_read as well.

My idea is keeping the non-blocking && error logic in read and write. In anonsocket_read and anonsocket_write, we either go through the block_thread path or the actual read/write path.

For now, if readbuf.borrow().buf.is_empty() && self.is_nonblock check will never be hit by blocking fd (the one without SOCK_NONBLOCK flag) after unblocked, so I just kept them in read/write.

It is fine to move it in anonsocket_read too if it is clearer.

Splitting the logic across two functions leads to a very odd (and not documented!) precondition for anonsocket_read. It also means I have to scroll up and down to look at the entire logic. The logic really isn't that complicated so IMO it'd be better to have it in a single function where one can easily see the entire thing at a single glance.

This makes sense, I can combine them together.

Actually the new logic here is just wrong. When the buffer is empty, we have to first check self_anonsocket.peer_fd().upgrade() and then check is_nonblock.

In the future please just keep the logic the way it is rather than trying to break it apart into two functions. :)

RalfJung · 2024-12-20T13:57:20Z

src/shims/unix/unnamed_socket.rs

+    let Some(peer_fd) = self_anonsocket.peer_fd().upgrade() else {
+        // If the upgrade from Weak to Rc fails, it indicates that all read ends have been
+        // closed.
+        return ecx.set_last_error_and_return(ErrorKind::BrokenPipe, &dest);


Is it really necessary to duplicate this check (it is both here and in write above)? I think that should be avoidable, so please reduce the code duplication.

Yea we can always do the check in anonsocket_write, and remove the one in write. This was still in write because I wanted to throw error as early as possible.

RalfJung · 2024-12-20T13:57:44Z

src/shims/unix/unnamed_socket.rs

-                // Blocking socketpair with a full buffer.
-                throw_unsup_format!("socketpair/pipe/pipe2 write: blocking isn't supported yet");
-            }
+        if available_space == 0 && self.is_nonblock {


This should also be moved into anonsocket_write I think, again to keep all the logic in the same place.

RalfJung · 2024-12-20T13:58:32Z

src/shims/unix/unnamed_socket.rs

        let available_space =
            MAX_SOCKETPAIR_BUFFER_CAPACITY.strict_sub(writebuf.borrow().buf.len());


Please do not duplicate this logic (that same line now exists twice in this file, always a bad sign).

RalfJung · 2024-12-20T13:59:19Z

src/shims/unix/unnamed_socket.rs

+        let peer_anonsocket = peer_fd.downcast::<AnonSocket>().unwrap();
+        // Unblock all threads that are currently blocked on peer_fd's read.


The let peer_anonsocket is already part of the "unblock all threads", right? So the comment should be moved up by 1 line.

RalfJung · 2024-12-20T13:59:28Z

src/shims/unix/unnamed_socket.rs

+            let peer_anonsocket = peer_fd.downcast::<AnonSocket>().unwrap();
+            // Unblock all threads that are currently blocked on peer_fd's write.


The let peer_anonsocket is already part of the "unblock all threads", right? So the comment should be moved up by 1 line.

rustbot added the S-waiting-on-author Status: Waiting for the PR author to address review comments label Dec 4, 2024

rustbot added the S-blocked Status: blocked on something happening somewhere else label Dec 4, 2024

tiif mentioned this pull request Dec 5, 2024

cleanup: avoid passing byte slice to anonsocket_read #4074

Merged

tiif force-pushed the blockop branch from c4d53ab to 5c1f3f8 Compare December 6, 2024 04:49

oli-obk reviewed Dec 6, 2024

View reviewed changes

src/shims/unix/unnamed_socket.rs Show resolved Hide resolved

oli-obk reviewed Dec 6, 2024

View reviewed changes

src/shims/unix/unnamed_socket.rs Outdated Show resolved Hide resolved

oli-obk reviewed Dec 13, 2024

View reviewed changes

src/shims/unix/unnamed_socket.rs Outdated Show resolved Hide resolved

tiif commented Dec 14, 2024

View reviewed changes

src/shims/unix/unnamed_socket.rs Show resolved Hide resolved

tiif changed the title ~~[WIP] blocking unnamed_socket~~ Implement blocking unnamed_socket Dec 16, 2024

tiif commented Dec 16, 2024

View reviewed changes

rustbot added S-waiting-on-review Status: Waiting for a review to complete and removed S-waiting-on-author Status: Waiting for the PR author to address review comments S-blocked Status: blocked on something happening somewhere else labels Dec 16, 2024

tiif marked this pull request as ready for review December 16, 2024 14:22

oli-obk requested changes Dec 18, 2024

View reviewed changes

Implement blocking unnamed_socket

f76c9fa

tiif force-pushed the blockop branch from 3cc8622 to f76c9fa Compare December 18, 2024 16:54

oli-obk approved these changes Dec 18, 2024

View reviewed changes

oli-obk added this pull request to the merge queue Dec 18, 2024

Merged via the queue into rust-lang:master with commit c9c28f4 Dec 18, 2024
7 checks passed

tiif deleted the blockop branch December 19, 2024 09:06

RalfJung reviewed Dec 20, 2024

View reviewed changes

tiif mentioned this pull request Dec 23, 2024

Follow-up changes for blocking unamed_socket #4099

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement blocking unnamed_socket #4072

Implement blocking unnamed_socket #4072

tiif commented Dec 4, 2024 •

edited

Loading

tiif commented Dec 4, 2024

bors commented Dec 5, 2024

tiif Dec 16, 2024

tiif commented Dec 16, 2024

oli-obk left a comment

RalfJung Dec 20, 2024

tiif Dec 20, 2024

RalfJung Dec 20, 2024

RalfJung Dec 20, 2024

tiif Dec 20, 2024 •

edited

Loading

RalfJung Dec 21, 2024 •

edited

Loading

tiif Dec 22, 2024

RalfJung Dec 27, 2024

RalfJung Dec 20, 2024

tiif Dec 20, 2024 •

edited

Loading

RalfJung Dec 20, 2024

RalfJung Dec 20, 2024 •

edited

Loading

RalfJung Dec 20, 2024

RalfJung Dec 20, 2024

		let available_space =
		MAX_SOCKETPAIR_BUFFER_CAPACITY.strict_sub(writebuf.borrow().buf.len());

		let peer_anonsocket = peer_fd.downcast::<AnonSocket>().unwrap();
		// Unblock all threads that are currently blocked on peer_fd's read.

Implement blocking unnamed_socket #4072

Implement blocking unnamed_socket #4072

Conversation

tiif commented Dec 4, 2024 • edited Loading

tiif commented Dec 4, 2024

bors commented Dec 5, 2024

Choose a reason for hiding this comment

tiif commented Dec 16, 2024

oli-obk 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

Choose a reason for hiding this comment

tiif Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

RalfJung Dec 21, 2024 • edited Loading

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

tiif Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiif commented Dec 4, 2024 •

edited

Loading

tiif Dec 20, 2024 •

edited

Loading

RalfJung Dec 21, 2024 •

edited

Loading

tiif Dec 20, 2024 •

edited

Loading

RalfJung Dec 20, 2024 •

edited

Loading