Reviewers: If you have a clue about Unix processes, please check my analysis below, especially with respect to the call of deactivate_process() in execute_internal_event() (event-stream.c).
I can't speak to the exact scenario that you listed, but it does seem plausible that XEmacs would try to write to a subprocess, the subprocess exits for whatever reason, and XEmacs doesn't handle the cleanup properly.
I would like to do the signal(2) dance in the relevant Lstream implementation (filedesc), but it doesn't know about pipes and processes, only about FDs. Is it worth creating a pipe-specific variant of the filedesc Lstream?
I wonder what select(2) does if the other end of the pipe has gone away. That might be a way for the Lstream code to tell whether it should try to write to the fd. On the other hand, even if that worked, there'd still a very small window between the select() call and the write(), where the process could go away. So maybe it's better to just ignore SIGPIPE. On Unixy systems, the write(2) call will still fail; you just won't get the signal.
Stephen J. Turnbull wrote: > Reviewers: If you have a clue about Unix processes, please check my > analysis below, especially with respect to the call of > deactivate_process() in execute_internal_event() (event-stream.c). I can't speak to the exact scenario that you listed, but it does seem plausible that XEmacs would try to write to a subprocess, the subprocess exits for whatever reason, and XEmacs doesn't handle the cleanup properly.
I have a backtrace for the exact scenario. I meant to put it on the tracker for posterity (and because there's a related layering issue earlier pointed out by Jan Vroonhof), but haven't done so yet.
> I would like to do the signal(2) dance in the relevant Lstream > implementation (filedesc), but it doesn't know about pipes and > processes, only about FDs. Is it worth creating a pipe-specific > variant of the filedesc Lstream? I wonder what select(2) does if the other end of the pipe has gone away. That might be a way for the Lstream code to tell whether it should try to write to the fd.
On Mac OS X it returns EBADF. Thanks for the suggestion, I will investigate. As you say this is probably not worth it here because it doesn't fix the race condition, but it might be useful.
On the other hand, even if that worked, there'd still a very small window between the select() call and the write(), where the process could go away. So maybe it's better to just ignore SIGPIPE. On Unixy systems, the write(2) call will still fail; you just won't get the signal.
Of course, we don't want to ignore SIGPIPE, because we need to clean up our end of the pipe, and the XEmacs structures associated with it. But we already have the necessary mechanism in place, the question here is how to connect it without race conditions.
If the process is about to be terminated, there's no guarantee that it would read the flushed data before exiting anyway. So I don't see this as an issue.
> On the other hand, even if that worked, there'd still a very small > window between the select() call and the write(), where the process > could go away. So maybe it's better to just ignore SIGPIPE. On Unixy > systems, the write(2) call will still fail; you just won't get the > signal. Of course, we don't want to ignore SIGPIPE, because we need to clean up our end of the pipe, and the XEmacs structures associated with it. But we already have the necessary mechanism in place, the question here is how to connect it without race conditions.
I agree that XEmacs needs to find out about the condition and clean up. But my understanding is that the SIGPIPE is only generated when XEmacs tries to write to the pipe. AFAIK, XEmacs doesn't need the asynchronous notification that a signal provides; it can just check for an error return from write() (EPIPE). But for XEmacs to actually get an error return, it either needs to tell the OS not to generate SIGPIPE, or it needs to ignore SIGPIPE. Ignoring SIGPIPE seems to be the more portable of the two approaches.
> If the process is about to be terminated, there's no guarantee that it > would read the flushed data before exiting anyway. So I don't see this > as an issue. Well, not exactly. By terminate I didn't mean "kill -9", I meant any of the various ways we might politely tell the subprocess to go away as well. I *think* that "deactivate_process" is only called *after* we receive a SIGCHLD or other strong indication that the subprocess has already gone away, and it is never called as (part of) the way to say "go away". But I'm not experienced in this area either in general or in the XEmacs process control implementation, so I'd like confirmation.
I agree that XEmacs needs to find out about the condition and clean up. But my understanding is that the SIGPIPE is only generated when XEmacs tries to write to the pipe. AFAIK, XEmacs doesn't need the asynchronous notification that a signal provides; it can just check for an error return from write() (EPIPE).
Well, in my (imperfect) understanding, in theory that's true, but it would require a new Lstream implementation for pipes, or maybe changes to the current filedesc implementation (which doesn't expect EPIPE). Also, some amount of higher-level code would need to be rewritten and reviewed for race conditions (it is designed to assume a reliable stream, and to ignore the details of when data is actually written to the pipe except when an explicit flush is called for).
But for XEmacs to actually get an error return, it either needs to tell the OS not to generate SIGPIPE, or it needs to ignore SIGPIPE. Ignoring SIGPIPE seems to be the more portable of the two approaches.
Mike Kupfer writes: > I agree that XEmacs needs to find out about the condition and clean up. > But my understanding is that the SIGPIPE is only generated when XEmacs > tries to write to the pipe. AFAIK, XEmacs doesn't need the asynchronous > notification that a signal provides; it can just check for an error > return from write() (EPIPE). Well, in my (imperfect) understanding, in theory that's true, but it would require a new Lstream implementation for pipes, or maybe changes to the current filedesc implementation (which doesn't expect EPIPE). Also, some amount of higher-level code would need to be rewritten and reviewed for race conditions (it is designed to assume a reliable stream, and to ignore the details of when data is actually written to the pipe except when an explicit flush is called for).
But the Lstream abstraction already requires the caller to check for an error code, for both the write and the flush. unix_send_process() does this SIGPIPE dance, with a flush at the end; it could easily check for an error return instead. AFAICS, the only benefit to the current structure is that it lets us mark the output stream stream as closed and mark the subprocess as dead, all in one place in the code. And I'm not sure that being able to mark the output stream as closed there is really an advantage, as it smells like a layering (abstraction) violation to me.
Except there's a third approach (the one actually implemented in XEmacs), which is to provide non-fatal SIGPIPE handling. The problem is that it's buggy.
Changing unix_send_process() so that it sets the SIGPIPE handler just once would be my second choice.
That's "clean", but it also requires a syscall, which will slow things down if we do it at too low level. We could have XEmacs itself ignore SIGPIPE, but then I suspect that code that improperly ignores EPIPE will be hard to detect and hard to diagnose when it causes a bug. Ie, here SIGPIPE functions (to some extent) like an assert. It might be a good idea to have XEmacs catch SIGPIPE globally, and just throw to top-level if the code isn't otherwise prepared for it. Having a subprocess go away should not be fatal to XEmacs (just as having the last connected display go away should not be fatal if gnuserv is running).
and have it pass EPIPE failures up to the caller.
Why do you think the caller will necessarily be prepared for an EPIPE?
unix_send_process() would need some changes, but IMO that would mostly be getting rid of unnecessary complexity.
Mike Kupfer writes: > I still think a cleaner solution is possible without a major rewrite to > the Lstream code, or a separate Lstream implementation for pipes. Just > have the Lstream code ignore SIGPIPE, That's "clean", but it also requires a syscall, which will slow things down if we do it at too low level.
Well, it would add two system calls per write system call (one to ignore SIGPIPE, the other to restore the default handler). I don't have a sense for how much this would slow things down in practice, but I acknowledge it's a potentially fatal problem with my proposal. Maybe there's a clean way to configure the Lstream when it's set up, so that only pipes get this treatment (without having a whole new Lstream implementation just for pipes).
It might be a good idea to have XEmacs catch SIGPIPE globally, and just throw to top-level if the code isn't otherwise prepared for it. Having a subprocess go away should not be fatal to XEmacs (just as having the last connected display go away should not be fatal if gnuserv is running).
The thing that makes me nervous about this approach is that it could leave internal state messed up. Any calls that could cause an Lstream write would have to be done in a place where it's okay if processing is thrown back to top-level.
> and have it pass EPIPE failures up to the caller. Why do you think the caller will necessarily be prepared for an EPIPE?
unix_send_process() would need to be changed, of course. The reason I'm not worried about arbitrary callers is that I'm assuming they're coded to check for a few specific errors, and to bail out for any other errors. I think EPIPE will fit into that framework. OTOH, my understanding of XEmacs internals is pretty meager, so I concede this could be a case of "ignorance is bliss".
> unix_send_process() would need some changes, but IMO that would > mostly be getting rid of unnecessary complexity. Patches welcome!
Okay, I'll add this to my list of things to hack on. In the meantime, I don't want to hold up your fix. And to clarify a remark I made yesterday, I think that your refactoring of the Lstream close code is perfectly reasonable for the long term, as there may be other situations where we want a close-no-flush function.
One thing that bothers is that the event loop code can run inside some of these functions, in particular unix_send_process. That means that anything can happen. Do you have an intuition that in practice nothing will happen? :-) I could just be seeing ghosts, but given my state of 95% ignorance, I can't quite justify disbelieving in them!
unix_send_process() would need to be changed, of course. The reason I'm not worried about arbitrary callers is that I'm assuming they're coded to check for a few specific errors, and to bail out for any other errors. I think EPIPE will fit into that framework.
But we want a specific definition of "bail out" for EPIPE. Ie, "assume the subprocess died untimely, so politely shutdown our end of the pipes, finalize the process structures, and then report the error to the user." We know that that is the current definition of how to handle SIGPIPE. I don't see any reason to suppose that is the definition of how to handle an unknown error throughout all code that might now see an EPIPE. Checking that seems like a pretty hairy task to me.
> Patches welcome! Okay, I'll add this to my list of things to hack on.
Well, that's up to you. FWIW, I was sort of joking; I'd be happy to have an alternative patch to review, but my feeling is that a redesign is a hairball that we should avoid right now, unless you think you personally will get something out of this (eg, a better understanding of parts of XEmacs you want to hack on in the future). I don't see a guarantee that you'll get an accept-able patch out of it. N.B. There are other problems in the Lstream code IMO. Specifically, it's too hard to localize errors and exceptional conditions (such as the presence of markers and extents) in streams when there are stacked Lstreams (and stacked Lstreams are very common because of the need to do Mule-coding in essentially all I/O contexts). If and when we do something about that, we could revisit the SIGPIPE vs. EPIPE design decision. If you like that option, we can put a comment in the code to that effect.
In the meantime, I don't want to hold up your fix.
Oh, that's already committed since it is certainly a fix (I'm just worried about whether it's introducing other risks, and none have been pointed out so far).
If anything, my intuition is that all sorts of chaos is possible. That's why I want to ignore SIGPIPE (remove asynchronous flows of execution from the mix).
I don't see a huge difference here, either, but then I don't like either choice. I think we can do better with EPIPE. It's true that unprepared code won't do all the cleanup that we want, but I expect there would be some sort of error passed back to the user. I'm guessing the worst we would see are resource leaks (memory and zombie child processes). I'd rather have some resource leaks than a crash. Or are there nasty side effects from failing to do proper cleanup that I'm failing to consider?
I'd need to review process-unix.c, process.c, and process-nt.c (not that I know anything about the Windows API). nas.c sets a SIGPIPE handler, but it doesn't appear to use Lstreams. I don't see why I'd have to do an exhaustive code flow analysis. I think that once the Lstream code starts seeing EPIPE, future calls will continue to return EPIPE, or possibly some other error indicating that the stream is dead. Eventually the code that needs to see the error should see it, and it can clean up then. And if it doesn't, we just have some resource leaks. Or at least that's the theory. I grant that there are a lot of suppositions here. I'm well aware that once I actually start coding and testing, I could run into issues that would make me reconsider this whole idea.
I'd be happy to have an alternative patch to review, but my feeling is that a redesign is a hairball that we should avoid right now, unless you think you personally will get something out of this (eg, a better understanding of parts of XEmacs you want to hack on in the future). I don't see a guarantee that you'll get an accept-able patch out of it.
Okay, thanks for the warning. This isn't something I'd be likely to get to anytime soon, given that I don't know when I'm even going to finish the package updates (Gnus, MH-E) I've already started on.
N.B. There are other problems in the Lstream code IMO. Specifically, it's too hard to localize errors and exceptional conditions (such as the presence of markers and extents) in streams when there are stacked Lstreams (and stacked Lstreams are very common because of the need to do Mule-coding in essentially all I/O contexts). If and when we do something about that, we could revisit the SIGPIPE vs. EPIPE design decision. If you like that option, we can put a comment in the code to that effect.
One thing that the SIGPIPE approach does is to satisfy DRY.
I don't see a huge difference here, either, but then I don't like either choice. I think we can do better with EPIPE. It's true that unprepared code won't do all the cleanup that we want, but I expect there would be some sort of error passed back to the user.
Yeah, probably, but I don't see how that's any better that what we currently have. And you still need a *global* ignore on the SIGPIPE.
I'm guessing the worst we would see are resource leaks (memory and zombie child processes). I'd rather have some resource leaks than a crash.
Well, crashes are typically easier to diagnose than resource leaks. And if you want we can have a global SIGPIPE handler that ignores.
Or are there nasty side effects from failing to do proper cleanup that I'm failing to consider?
Infloops. If we don't catch the EPIPE, then our end of the pipe is still open AFA XEmacs CT. "Heh heh heh Beavis, that was fun!" "Yeah Butthead, let's do that again!"
Sure, assuming it does. But we're talking about situations where the competent and responsible code doesn't see the EPIPE somehow.
Or at least that's the theory. I grant that there are a lot of suppositions here. I'm well aware that once I actually start coding and testing, I could run into issues that would make me reconsider this whole idea.
I'll think about that, but I'm not convinced. process-unix.c and nas.c each has its own handler.
Mike Kupfer writes: > I don't see a huge difference here, either, but then I don't like either > choice. I think we can do better with EPIPE. It's true that unprepared > code won't do all the cleanup that we want, but I expect there would be > some sort of error passed back to the user. Yeah, probably, but I don't see how that's any better that what we currently have.
It's better because it makes a crash less likely. Though your comment about resource leaks being harder to troubleshoot is giving me pause. I'll want to think about that some more.
And you still need a *global* ignore on the SIGPIPE.
No, the Lstream code can ignore SIGPIPE just when writing. Though I wouldn't necessarily object to a global ignore of SIGPIPE.
> Or are there nasty side effects from failing to do proper cleanup > that I'm failing to consider? Infloops. If we don't catch the EPIPE, then our end of the pipe is still open AFA XEmacs CT. "Heh heh heh Beavis, that was fun!" "Yeah Butthead, let's do that again!"
[EPIPE] is better than [SIGPIPE] because it makes a crash less likely.
> And you still need a *global* ignore on the SIGPIPE. No, the Lstream code can ignore SIGPIPE just when writing.
No, it *can't*. If that were safe, then the current code would be safe (or could be made safe). The current code doesn't crash *because* we catch SIGPIPE, it crashes because we get a SIGPIPE where we don't expect it. If we don't ignore SIGPIPE *all* the places where we need to handle EPIPE, then we're no better off than we were with SIGPIPE. Another way to put it is that your proposal (which I admit has a good chance of being as safe or safer as my patch) depends for its safety on ignoring SIGPIPE at a very low level. I think that is likely to result in a perceptible slowdown in I/O because of all the system calls needed. So the obvious solution is to ignore (or catch) SIGPIPE globally, which will mean only one system call is needed, at XEmacs initialization.
I can imagine various scenario where this could happen. I think they're unlikely, but I've made a note to do some checking.