Skip to content

Set MSG_NOSIGNAL for UnixStream #140005

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

mlowicki
Copy link

@mlowicki mlowicki commented Apr 18, 2025

#139956

Same logic as for

pub fn write(&self, buf: &[u8]) -> io::Result<usize> {
let len = cmp::min(buf.len(), <wrlen_t>::MAX as usize) as wrlen_t;
let ret = cvt(unsafe {
c::send(self.inner.as_raw(), buf.as_ptr() as *const c_void, len, MSG_NOSIGNAL)
})?;
Ok(ret as usize)
}
.

@rustbot
Copy link
Collaborator

rustbot commented Apr 18, 2025

r? @joboet

rustbot has assigned @joboet.
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 O-unix Operating system: Unix-like 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 18, 2025
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@mlowicki mlowicki marked this pull request as ready for review April 18, 2025 12:13
@joboet
Copy link
Member

joboet commented May 5, 2025

@rustbot label +I-libs-nominated

Hey T-libs! Was there a particular reason for this? I think it's reasonable to say that generating signals doesn't make sense for everything but console I/O.

r=me if you agree

@rustbot rustbot added the I-libs-nominated Nominated for discussion during a libs team meeting. label May 5, 2025
@joshtriplett joshtriplett changed the title Set MSG_NOSIGNAL for UnixSteam Set MSG_NOSIGNAL for UnixStream May 5, 2025
@joshtriplett joshtriplett added the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label May 6, 2025
@joshtriplett
Copy link
Member

Nominating for libs-api; I think the handling of broken pipes is an API question.

@joshtriplett
Copy link
Member

In general, ignoring SIGPIPE and producing EPIPE is often a good idea. However, I'm not sure if this change is something we can or should do on behalf of the user without knowing what they want, for multiple reasons.

The minor reason is that the user might not be expecting EPIPE. However, I think that's less of a problem for things other than stdout/stderr.

The larger reason is that switching from write to send could break users who convert something other than a socket to a UnixStream, because send doesn't work on non-sockets.

I think it's still reasonable to consider this change, but we're going to want to announce it as a potential compatibility issue.

@mlowicki
Copy link
Author

mlowicki commented May 6, 2025

In general, ignoring SIGPIPE and producing EPIPE is often a good idea. However, I'm not sure if this change is something we can or should do on behalf of the user without knowing what they want, for multiple reasons.

The minor reason is that the user might not be expecting EPIPE. However, I think that's less of a problem for things other than stdout/stderr.

The larger reason is that switching from write to send could break users who convert something other than a socket to a UnixStream, because send doesn't work on non-sockets.

I think it's still reasonable to consider this change, but we're going to want to announce it as a potential compatibility issue.

FTR MSG_NOSIGNAL is set for e.g. UnixDatagram -

and documentation also doesn't mention that fact so might be worthy to improve API docs too.

@joshtriplett
Copy link
Member

@mlowicki Agreed.

@Amanieu Amanieu added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. I-libs-nominated Nominated for discussion during a libs team meeting. I-libs-api-nominated Nominated for discussion during a libs-api team meeting. labels May 6, 2025
@joshtriplett
Copy link
Member

We discussed this in today's @rust-lang/libs-api call. We're fine with doing this, but we'd like to do an FCP to confirm this change, since it's user-visible (albeit not a documented guarantee).

@rfcbot merge

@rfcbot
Copy link
Collaborator

rfcbot commented May 6, 2025

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels May 6, 2025
@mlowicki
Copy link
Author

@mlowicki Agreed.

Once this gets merged I can create another PR to improve the doc for UnixDatagram and UnixStream.

@tgross35
Copy link
Contributor

@mlowicki Agreed.

Once this gets merged I can create another PR to improve the doc for UnixDatagram and UnixStream.

This can be done here, FCP hasn't started yet so there is time.

@mlowicki
Copy link
Author

@mlowicki Agreed.

Once this gets merged I can create another PR to improve the doc for UnixDatagram and UnixStream.

This can be done here, FCP hasn't started yet so there is time.

Done. Fixed it for UnixStream and TcpStream as the issue affects SOCK_STREAM and not SOCK_DGRAM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. O-unix Operating system: Unix-like proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants