set SO_NOSIGPIPE on apple platforms #36426
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
|
I’m not sure about modifying the standard library for that. You can set signal handing mode in debugger: or shorter You can also put such commands in your lldb configuration file (usually |
|
Yes, the "process handle" thing works, but is very cumbersome to work with and gets in the way. Xcode runs lldb as a library and apparently ignores both .lldbinit and .lldbinit-Xcode. The only way we could find to get the "process handle" command to work relies on a hackish breakpoint on the main function: http://stackoverflow.com/a/10456557/172537 . This has to be done on every developper workstation. But even then, this is cumbersome: whenever one disable breakpoints (and this is a frequent use case in debugging apps, as you often have to do manually put the app in the state you're about to debug), you'll also disable the main() breakpoint and trip over SIGPIPE again. Also, there is something that does not feel right in having these signals thrown to the process every time a socket is closed (or so), just to be ignored. It has to cost something. The rust standard library ignores them anyway (which to be honest, feels a bit wrong too: what if a non-rust library relies on them somewhere else in the process ?). |
|
Thanks for the PR @kali! As I'm sure you're already aware, the standard library already ignores My next question would be whether your application controls these sockets or whether the sockets are created by the standard library? This does seem like it's a bit of a heavyweight solution, but we could certainly add a |
|
Hi @alexcrichton! Thanks for your interest in this. I'm aware rust stdlib makes a signal(SIGPIPE, SIG_IGN). It does work as specified, including on iOS: this does not suppress the signal, it handles it by ignoring it. We have no problem in "production" mode (release build on an actual phone) with or without the proposed patch in stdlib. It is a just a major annoyance for people in charge of developing the iOS application that uses my rust library: using lldb is the "natural workflow" set by xcode, and lldb catches the SIGPIPE before the SIG_IGN has a chance to ignore it. As a result, the app developer workflow is completely disrupted. There is a workaround, but it still very cumbersome. In my case, the sockets are actually created and controller by hyper. I guess a set_ignore_sigpipe() should work, if I can get a patch in hyper. Note that if we want to have something consistent on linux, we would need to piggyback our flag on the socket struct to know if we should set the MSG_NOSIGNAL at each and every call on send(). I must say I'm not sure I see any real problem with my approach, and I also kind of think it's better in the long run... Signal are not free, why would we "pay" for this signal to be actually emitted more or less regularly on typical network operations just to get ignored by a process wide handler, when the libc offers a clean way to solve the question at its root ? Rust stdlib has already decided that SIGPIPE was obsolete anyway... I would honestly go for setting both SO_NOSIGPIPE on ios and macos, as proposed, and setting MSG_NOSIGNAL on linux (and everywhere it's supported) as well. I'm ready to do that if you think it's preferable. |
Jeez, that is indeed quite annoying! Out of curiosity, does basically all socket programming on iOS set this option? Surely this would be a major annoyance for anyone doing anything network related on iOS?
Ah that's unfortunate! But also a very good point about how you don't always control all your sockets.
I think this approach is fine as well, but to land we'd probably want consistency across all platforms and apply this to Linux as well (e.g. @kali want to prototype what |
alexcrichton
added
the
T-libs-api
|
Here is a minimalistic fix setting MSG_NOSIGNAL on linux. As on apple platforms, I could manually check the issue before the patch, and it is gone with it. That said I'm not sure if it can be unit-tested easily. We may want to move the constant definition to libc, but I'm not sure about that : it would save the double definition, but on the other hand exposing the constant seems useless. |
| #[cfg(target_os = "linux")] | ||
| const MSG_NOSIGNAL: c_int = 0x4000; | ||
| #[cfg(not(target_os = "linux"))] | ||
| const MSG_NOSIGNAL: c_int = 0x0; // unused dummy value |
There was a problem hiding this comment.
Could these be added to liblibc so we can verify their values as well?
There was a problem hiding this comment.
ok, sure, I'll do that.
| @@ -221,11 +226,12 @@ impl TcpStream { | |||
|
|
|||
| pub fn write(&self, buf: &[u8]) -> io::Result<usize> { | |||
| let len = cmp::min(buf.len(), <wrlen_t>::max_value() as usize) as wrlen_t; | |||
| let flags = if cfg!(target_os = "linux") { MSG_NOSIGNAL } else { 0 }; | |||
There was a problem hiding this comment.
This could just always be MSG_NOSIGNAL, right, b/c the #[cfg] is above?
There was a problem hiding this comment.
Yes indeed. I was was just trying to emulate https://github.com/rust-lang/rust/blob/master/src/libstd/sys/unix/net.rs#L26 .
You prefer to pass MSG_NOSIGNAL all the time ? I guess we can dispense with the let flags and put it straight in the send call in that case.
There was a problem hiding this comment.
Oh passing it there sometimes and not others is actually somewhat unrelated. Here yeah we should always pass MSG_NOSIGNAL
add MSG_NOSIGNAL (linux, android) see rust-lang/rust#36426
|
Looks good to me! Want to rebase and update the libc module with the merge commit? |
While integrating a Rust library in an iOS application, an app developper will typically run the application in the simulator with lldb. In this setup, when the rust library hit a EPIPE, SIGPIPE is also sent to the process, and stops execution in lldb. And this happens often.
Handling the SIGPIPE at the process level with signal or sigaction does not help here: lldb will intercept the signal and interrupt the execution regardless the signal handling.
On iOS and MacOS, SIGPIPE can be disabled at the socket level with setsockopt. Other platforms have other ways to do it (linux use a flag on send()).
It looks worse than it really is, but it's a major annoyance in our project, xcode making it very cumbersome to alter this particular lldb behavior. Also, it tends to freak out iOS developers, and this is certainly not what we want :)