Skip to content
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.

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

Refactor AnonSocket::read/write for blocking socketpair #4037

Merged
merged 1 commit into from
Nov 24, 2024

Conversation

tiif
Copy link
Contributor

@tiif tiif commented Nov 17, 2024

This PR moves the actual read/write operations of AnonSocket::read/write into separate functions, in preparation for #3665. The new functions will be used as blocking callback.

Comment on lines +171 to +170
// TODO: We might need to decide what to do if peer_fd is closed when read is blocked.
anonsocket_read(self, self.peer_fd().upgrade(), &mut bytes, ptr, dest, ecx)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not entirely sure if closing peer_fd when it is blocking should be allowed. I currently prefer to leave the TODO here and revisit it again when implementing blocking (and write a testcase for this).

Copy link
Member

Choose a reason for hiding this comment

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

I would expect that closing one side wakes up the other side, and makes it return an EOF read.

I'm a bit surprised that all that if readbuf.borrow().buf.is_empty() { stuff is not insider the helper, but 🤷 I guess it can be moved later if it turns out to be useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

'm a bit surprised that all that if readbuf.borrow().buf.is_empty() { stuff is not insider the helper,

Ah right I should've moved them inside helper, I guess I will just separate this into another commit when I implement the blocking operation.

@tiif
Copy link
Contributor Author

tiif commented Nov 17, 2024

@rustbot ready

@rustbot rustbot added the S-waiting-on-review Status: Waiting for a review to complete label Nov 17, 2024
@@ -146,7 +146,7 @@ impl FileDescription for AnonSocket {
// corresponding ErrorKind variant.
throw_unsup_format!("reading from the write end of a pipe");
};
let mut readbuf = readbuf.borrow_mut();
let readbuf = readbuf.borrow_mut();
if readbuf.buf.is_empty() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if readbuf.buf.is_empty() {
if readbuf.borrow_mut().buf.is_empty() {

Then you don't need the explicit drop

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice! I wonder if it would be correct to assume that Ref or RefMut will be automatically dropped at the end of the statement if it is not used elsewhere?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. Temporary values are dropped at the end of their statement

@oli-obk
Copy link
Contributor

oli-obk commented Nov 21, 2024

Lgtm. Please squash

@RalfJung RalfJung added this pull request to the merge queue Nov 24, 2024
Merged via the queue into rust-lang:master with commit 1c6bfbb Nov 24, 2024
7 checks passed
@tiif tiif deleted the blockpipe branch November 25, 2024 14:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Waiting for a review to complete
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants