5:32 p.m.
APPROVE COMMIT 21.5
I don't know if this is relevant to 21.4. Should I check, Vin? This
seems to be a very rare crash, up until Uday Reddy and friends found
me a way to produce it frequently. :-)
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).
VM seems to have replaced Gnus as my favorite way to induce XEmacs
crashes.... VM uses Imagemagick's convert program to generate
thumbnails to display with its MIME buttons, and apparently convert
crashes a lot. When this happens, there's a race condition such that
send_process() thinks it has blocked but doesn't know about the crash,
and so calls Faccept_process_output() in hopes of getting the subprocess
unblocked. The SIGCHLD arrives in the interim, status_notify() gets
called inside of Faccept_process_output(), notices the demise of the
child, and calls deactivate_process(), which tries to do a normal close
of all pipes. That calls Lstream_flush(), which writes to the now
nonexistent output pipe, and XEmacs takes the fall because it's not
prepared for a SIGPIPE there.
An analysis of calls of deactivate_process() indicates that it only gets
called when we know the process has already exited, so it makes no
sense to flush the output pipe. This patch removes the flush from the
function closing the output pipe (only).
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 will revert this patch on request. I don't really have an idea of
what bad things could happen if we fail to flush to a live process
that is about to be terminated, but if somebody thinks that's a risk,
I have an alternative patch which reduces the current risk but does
not eliminate it. (Specifically, with the alternative patch, the race
condition still exists in Faccept_process_output(), which is a Lisp
function so we have no idea when it might be called.)
diff -r 3fde0e346ad7 -r 2dbefd79b3d3 src/ChangeLog
--- a/src/ChangeLog Sat Oct 29 00:35:33 2011 +0900
+++ b/src/ChangeLog Sat Oct 29 01:10:32 2011 +0900
ï¼ ï¼ -1,3 +1,16 ï¼ ï¼
+2011-10-29 Stephen J. Turnbull <stephen(a)xemacs.org>
+
+ Prevent SIGPIPEs in deactivate_process().
+
+ * process.c (deactivate_process):
+ Use Lstream_close_noflush on output pipe instead of Lstream_close.
+
+ * lstream.c (Lstream_close_noflush):
+ New. Factored out of Lstream_close.
+ (Lstream_close): Use Lstream_close_noflush.
+
+ * lstream.h (Lstream_close_noflush): Declare it.
+
2011-10-29 Stephen J. Turnbull <stephen(a)xemacs.org>
Prevent assert at frame.c, l. 6311 by initializing glyph cachels.
diff -r 3fde0e346ad7 -r 2dbefd79b3d3 src/lstream.c
--- a/src/lstream.c Sat Oct 29 00:35:33 2011 +0900
+++ b/src/lstream.c Sat Oct 29 01:10:32 2011 +0900
ï¼ ï¼ -791,6 +791,45 ï¼ ï¼
return Lstream_flush (lstr);
}
+/* Close the stream without flushing buffers.
+ In current practice, this is only useful when a subprocess terminates
+ unexpectedly, and the OS closes its pipes without warning. In that case,
+ we do not want to flush our output buffers, as there is no longer a pipe
+ to write to.
+ This nomenclature may deserve review if XEmacs starts getting called as
+ a subprocess. */
+
+int
+Lstream_close_noflush (Lstream *lstr)
+{
+ lstr->flags &= ~LSTREAM_FL_IS_OPEN;
+ lstr->byte_count = 0;
+ /* Note that Lstream_flush() reset all the buffer indices. That way,
+ the next call to Lstream_putc(), Lstream_getc(), or Lstream_ungetc()
+ on a closed stream will call into the function equivalents, which will
+ cause an error. */
+
+ /* We set the pointers to 0 so that we don't lose when this function
+ is called more than once on the same object */
+ if (lstr->out_buffer)
+ {
+ xfree (lstr->out_buffer);
+ lstr->out_buffer = 0;
+ }
+ if (lstr->in_buffer)
+ {
+ xfree (lstr->in_buffer);
+ lstr->in_buffer = 0;
+ }
+ if (lstr->unget_buffer)
+ {
+ xfree (lstr->unget_buffer);
+ lstr->unget_buffer = 0;
+ }
+
+ return 0;
+}
+
/* Close the stream. All data will be flushed out. If the stream is
already closed, nothing happens. Note that, even if all data has
already been flushed out, the act of closing a stream may generate more
ï¼ ï¼ -838,30 +877,7 ï¼ ï¼
rc = -1;
}
- lstr->flags &= ~LSTREAM_FL_IS_OPEN;
- lstr->byte_count = 0;
- /* Note that Lstream_flush() reset all the buffer indices. That way,
- the next call to Lstream_putc(), Lstream_getc(), or Lstream_ungetc()
- on a closed stream will call into the function equivalents, which will
- cause an error. */
-
- /* We set the pointers to 0 so that we don't lose when this function
- is called more than once on the same object */
- if (lstr->out_buffer)
- {
- xfree (lstr->out_buffer);
- lstr->out_buffer = 0;
- }
- if (lstr->in_buffer)
- {
- xfree (lstr->in_buffer);
- lstr->in_buffer = 0;
- }
- if (lstr->unget_buffer)
- {
- xfree (lstr->unget_buffer);
- lstr->unget_buffer = 0;
- }
+ Lstream_close_noflush (lstr);
return rc;
}
diff -r 3fde0e346ad7 -r 2dbefd79b3d3 src/lstream.h
--- a/src/lstream.h Sat Oct 29 00:35:33 2011 +0900
+++ b/src/lstream.h Sat Oct 29 01:10:32 2011 +0900
ï¼ ï¼ -306,6 +306,7 ï¼ ï¼
int Lstream_rewind (Lstream *lstr);
int Lstream_seekable_p (Lstream *lstr);
int Lstream_close (Lstream *lstr);
+int Lstream_close_noflush (Lstream *lstr);
void Lstream_delete (Lstream *lstr);
void Lstream_set_character_mode (Lstream *str);
diff -r 3fde0e346ad7 -r 2dbefd79b3d3 src/process.c
--- a/src/process.c Sat Oct 29 00:35:33 2011 +0900
+++ b/src/process.c Sat Oct 29 01:10:32 2011 +0900
ï¼ ï¼ -2150,8 +2150,20 ï¼ ï¼
/* Must call this before setting the streams to nil */
event_stream_unselect_process (p, 1, 1);
+ /* We can get here in case of a crash in the external process, and then
+ the Lstream_close on output will cause a SIGPIPE, which we're not ready
+ for here. It looks to me like all cases where this function is called
+ we know the process has exited (but I'm not 100% sure for the call in
+ execute_internal_event (event-stream.c)), so it should be OK to use
+ Lstream_close_noflush.
+
+ #### The layering here needs a rethink. We should just be able
+ to call Lstream_close, and let the Lstream's implementation decide
+ if it can flush safely or not. The immediate problem is that the
+ Lstream needs to know the process's status, but I don't think it has
+ a handle to the process. */
if (!NILP (DATA_OUTSTREAM (p)))
- Lstream_close (XLSTREAM (DATA_OUTSTREAM (p)));
+ Lstream_close_noflush (XLSTREAM (DATA_OUTSTREAM (p)));
if (!NILP (DATA_INSTREAM (p)))
Lstream_close (XLSTREAM (DATA_INSTREAM (p)));
if (!NILP (DATA_ERRSTREAM (p)))
_______________________________________________
XEmacs-Patches mailing list
XEmacs-Patches(a)xemacs.org
http://lists.xemacs.org/mailman/listinfo/xemacs-patches
10:47 p.m.
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 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.
I will revert this patch on request. I don't really have an idea
of
what bad things could happen if we fail to flush to a live process
that is about to be terminated, but if somebody thinks that's a risk,
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.
mike
_______________________________________________
XEmacs-Patches mailing list
XEmacs-Patches(a)xemacs.org
http://lists.xemacs.org/mailman/listinfo/xemacs-patches
1:58 a.m.
Mike Kupfer writes:
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.
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.
_______________________________________________
XEmacs-Patches mailing list
XEmacs-Patches(a)xemacs.org
http://lists.xemacs.org/mailman/listinfo/xemacs-patches
5:32 a.m.
Stephen J. Turnbull wrote:
Mike Kupfer writes:
> 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.
Okay. I'll put this on my list of things to look at, in case nobody
gets to it sooner.
mike
_______________________________________________
XEmacs-Patches mailing list
XEmacs-Patches(a)xemacs.org
http://lists.xemacs.org/mailman/listinfo/xemacs-patches
6:07 a.m.
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 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.
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. I think my patch fixes it (at least for Unixoid
platforms). The alternatives require bigger changes and maybe a
rethink of the whole Lstream strategy AFAICS. I don't have an
appetite for that.
_______________________________________________
XEmacs-Patches mailing list
XEmacs-Patches(a)xemacs.org
http://lists.xemacs.org/mailman/listinfo/xemacs-patches
9:31 p.m.
Stephen J. Turnbull wrote:
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.
True, but the reason it's buggy is that it tries to predict when the
Lstream code might push bits out to the pipe, and it only has the signal
catcher enabled in those places. That, too, smells like a layering
violation.
Though I suppose this points at yet another possible fix: change
unix_send_process() to register its SIGPIPE handler after doing the
setjmp, and only restore the old handler just before returning.
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, and have it pass EPIPE failures up
to the caller. unix_send_process() would need some changes, but IMO
that would mostly be getting rid of unnecessary complexity.
If that's a larger change than you want to tackle at this time, I can't
argue with that. It's your time, after all. :-) Changing
unix_send_process() so that it sets the SIGPIPE handler just once would
be my second choice. And I don't really object to your
close-without-flush solution, either, at least as a short-term fix.
cheers,
mike
_______________________________________________
XEmacs-Patches mailing list
XEmacs-Patches(a)xemacs.org
http://lists.xemacs.org/mailman/listinfo/xemacs-patches
11:57 p.m.
Mike Kupfer wrote:
Changing unix_send_process() so that it sets the SIGPIPE handler
just
once would be my second choice.
I'm having second thoughts about this approach. The longjmp in the
SIGPIPE handler seems like it could mess something up.
mike
_______________________________________________
XEmacs-Patches mailing list
XEmacs-Patches(a)xemacs.org
http://lists.xemacs.org/mailman/listinfo/xemacs-patches
4:46 a.m.
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. 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.
Patches welcome!
If that's a larger change than you want to tackle at this time, I
can't
argue with that. It's your time, after all. :-)
Doesn't have to be *my* time. :-)
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!
_______________________________________________
XEmacs-Patches mailing list
XEmacs-Patches(a)xemacs.org
http://lists.xemacs.org/mailman/listinfo/xemacs-patches
9:59 p.m.
Stephen J. Turnbull wrote:
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!
This is sort of why I had second thoughts about having
unix_send_process() just set the SIGPIPE handler once. I didn't
realize that the event handler could run. I was worried about what
might break if XEmacs did a longjmp while cleaning up from an exiting
child.
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).
mike
_______________________________________________
XEmacs-Patches mailing list
XEmacs-Patches(a)xemacs.org
http://lists.xemacs.org/mailman/listinfo/xemacs-patches
4:47 a.m.
Mike Kupfer writes:
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.
That's already true, since the current consequence is a crash due to
the default fatal handler on SIGPIPE. I don't really see a difference
here, it's a matter of philosophy whether it's nicer to the user to
crash or to leave them in an unknown state when we've screwed up
badly. Either way, we have to admit we've screwed up. ;-)
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).
Sure, that makes sense ... but the current problem is that there are
unremovable (at least within the scope of this discussion)
asynchronous flows of execution that were not protected from SIGPIPE
at all. So ignoring SIGPIPE at a low level doesn't really help this
problem. We still need to check all the possible flows of execution.
One thing that the SIGPIPE approach does is to satisfy DRY. All of
the broken PIPE conditions are handled in the same place, even though
we're calling back into the event loop. Offhand, I don't see how to
do that in the EPIPE approach.
_______________________________________________
XEmacs-Patches mailing list
XEmacs-Patches(a)xemacs.org
http://lists.xemacs.org/mailman/listinfo/xemacs-patches
3:22 a.m.
Stephen J. Turnbull wrote:
Mike Kupfer writes:
> 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.
That's already true, since the current consequence is a crash due to
the default fatal handler on SIGPIPE. I don't really see a difference
here, it's a matter of philosophy whether it's nicer to the user to
crash or to leave them in an unknown state when we've screwed up
badly. Either way, we have to admit we've screwed up. ;-)
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?
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.
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.
I'd be happy to have this added to the list of issues to consider in any
potential Lstream redesign. I mean, given my current productivity, that
redesign might happen before I get to this.... :-/
One thing that the SIGPIPE approach does is to satisfy DRY.
I'll think about that, but I'm not convinced. process-unix.c and nas.c
each has its own handler. Maybe there's some twist related to calling
back into the event loop that I'm missing. But the cleanup that's
currently done after the longjmp in the signal handler could just as
easily be packaged up as a function. That's DRY enough for me.
mike
_______________________________________________
XEmacs-Patches mailing list
XEmacs-Patches(a)xemacs.org
http://lists.xemacs.org/mailman/listinfo/xemacs-patches
8:01 a.m.
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. 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!"
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,
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.
Unlikely. I agree with your theory, but it hinges on the assumption
that we sort of know what can go wrong. The problem with that
assumption is that as far as we know, nothing can go wrong. So if
something does go wrong, by definition, we know very little about
it.:-)
I'll think about that, but I'm not convinced. process-unix.c
and nas.c
each has its own handler.
Yeah, I know that NAS has a separate handler. I need to look at that
at some point, but AFAIK NAS is basically a dead letter these days.
Even I don't build it in anymore, although around 2000 I thought it
was one of the coolest features of XEmacs.
_______________________________________________
XEmacs-Patches mailing list
XEmacs-Patches(a)xemacs.org
http://lists.xemacs.org/mailman/listinfo/xemacs-patches
12:57 a.m.
Stephen J. Turnbull wrote:
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!"
I can imagine various scenario where this could happen. I think they're
unlikely, but I've made a note to do some checking.
mike
_______________________________________________
XEmacs-Patches mailing list
XEmacs-Patches(a)xemacs.org
http://lists.xemacs.org/mailman/listinfo/xemacs-patches
8:30 a.m.
Mike Kupfer writes:
[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.
Sure. The scenario that caught me apparently requires (1) starting a
new process every few milliseconds, (2) in a broken way, and (3) on a
multicore system. (I'm not entirely sure about (3).) AFAIK nobody
else has reported it.
_______________________________________________
XEmacs-Patches mailing list
XEmacs-Patches(a)xemacs.org
http://lists.xemacs.org/mailman/listinfo/xemacs-patches
4550
days inactive
4561
days old
13 comments
2 participants
participants (2)
-
Mike Kupfer -
Stephen J. Turnbull