twisted's SIGCHLD handler breaks popen.

[jknight]

Highest priority is for regressions and reviews.

[jknight]

This is a simple way to demonstrate:
>>> from twisted.internet import reactor
>>> reactor._handleSignals()
>>> f=os.popen("sleep 5; echo 'Foo'"); f.read()
Traceback (most recent call last):
  File "<stdin>", line 1, in ?
IOError: [Errno 4] Interrupted system call
The solution is easy: our SIGCHLD handler needs to be given a SA_RESTART flag. This will cause it to
not be handled until after the read call returns, as python does not allow the python sighandler code to
be run in the interrupt context. But that's okay.
There's only one minor problem: the python signal() wrapper doesn't provide the flags parameter that's
necessary in order to specify SA_RESTART. So, to fix this, someone should write a C extension module
that does something like this:
void add_signalflags(int sig, int moreflags)
{
  int res;
  struct sigaction act;
  res = sigaction(sig, NULL, &act);
  if (res < 0)
    RAISE ERROR
  act.sa_flags |= moreflags;
  res = sigaction(sig, &act, NULL);
  if (res < 0)
    RAISE ERROR
}

[glyph]

OK, no useful feedback, but AWESOME for figuring this out, james.  Thanks.

[glyph]

OK wait no, I changed my mind.  Will passing this flag still (properly)
interrupt select()?

[jknight]

According to my documentation it will.
     If a signal is caught during the system calls listed below, the call may
     be forced to terminate with the error EINTR, the call may return with a
     data transfer shorter than requested, or the call may be restarted.
     Restart of pending calls is requested by setting the SA_RESTART bit in
     sa_flags.  The affected system calls include open(2), read(2), write(2),
     sendto(2), recvfrom(2), sendmsg(2) and recvmsg(2) on a communications
     channel or a slow device (such as a terminal, but not a regular file) and
     during a wait(2) or ioctl(2).  However, calls that have already committed
     are not restarted, but instead return a partial success (for example, a
     short read count).
May want to make *sure* by testing, though.

[itamarst]

On Redhat 7.3 I had a hard time reproducing this, but:
>>> signal.alarm(1); f=os.popen("sleep 5; echo 'Foo'"); f.read()
reproduced it some of the time.

[ColinAlston]

Reproduced thanks to Gentoo Emerge and its crappy signaling

avail = os.popen('emerge -p --nocolor --nospinner %s | grep %s' % (pack, pack)) k = avail.read()

Something like that, put it anywhere you please, and you will intermitantly generate this bug on Twisted 2.0.1

[spiv]

moshez made an interesting suggestion on IRC -- I wonder if we can implement the missing add_signalflags function with ctypes?

[exarkun]

See also #791 and #1997

[titty]

Feel free to use: http://systemexit.de/repo/sigalter It's a c module, which provides a set_SA_RESTART(SIGNUM) method.

Here's the code (in case the above server is unreachable):

#include <Python.h>
#include <signal.h>
static PyObject *
set_SA_RESTART (void *self, PyObject *args)
{
        struct sigaction sa;
        int signum;
        if (!PyArg_ParseTuple(args, "i:set_SA_RESTART", &signum)) {
                return NULL;
        }
        if (sigaction(signum, NULL, &sa)==-1) {
                goto error;
        }
        if (!(sa.sa_flags & SA_RESTART)) {
                sa.sa_flags |= SA_RESTART;
                if (sigaction(signum, &sa, NULL)==-1) {
                        goto error;
                }
        }
        return Py_BuildValue("");
error:
        PyErr_SetFromErrno(PyExc_OSError);
        return NULL;
}
static PyMethodDef module_function[] = {
        {"set_SA_RESTART", (PyCFunction)set_SA_RESTART, METH_VARARGS,
         "set_SA_RESTART(SIGNUM):\n   sets SA_RESTART flag on the signalhandler for signal SIGNUM"},
        { NULL,    NULL }
};
static char module_doc[] = "provide funtions to alter signal handling";
DL_EXPORT(void) initsigalter(void)
{
        Py_InitModule3("sigalter", module_function, module_doc);
}

[titty]

the ctypes version (for linux):

import ctypes
import signal
libc = ctypes.CDLL("libc.so.6")
libc.siginterrupt(signal.SIGCHLD, 0)

[exarkun]

#791, #1997, and #2535 have significant overlap with this. Fixing any one of these (except for #2535, perhaps) almost certainly means fixing all of them.

The correct fix for most of them is:

• Write a SIGCHLD handler in C. All it needs to do is write to an fd.
• Install the SIGCHLD handler with the SA_RESTART flag. This makes it run when a child exits, but doesn't cause EINTR.
• Include the fd in the reactor so that whenever it is written to, the reactor will wake up and reap dead children.

With this solution:

• No more EINTR
• No more race condition inside ceval.c

However, a custom SIGCHLD handler is still required, so #2535 isn't resolved.

[titty]

http://docs.python.org/dev/library/signal.html:

python 2.6 will have signal.siginterrupt and signal.set_wakeup_fd.

[thijs]

Replying to titty:

http://docs.python.org/dev/library/signal.html: python 2.6 will have signal.siginterrupt and signal.set_wakeup_fd.

Now a Python 2.6 buildslave available http://buildbot.twistedmatrix.com/builders/ubuntu64-python2.6-select/

[exarkun]

(In [25649]) Branching to 'popen-compatibility-733'

[glyph]

Should I kill off the branches for #791? It looks like the code useful for addressing this problem has all been inherited by this branch.

[glyph]

Also if you're working on a ticket, please accept it (or at least reassign it to yourself).

[exarkun]

Deleting branches is so boring. I think I may have gotten all the good bits of the #791 branch at this point, so feel free to delete it if you like. Someday I'll probably write a program that deletes branches which are associated with tickets which have been closed for a while. Until then I don't plan to think about it much.

[exarkun]

(In [26139]) Branching to 'popen-compatibility-733-2'

[glyph]

This is weird.

Under 2.5, using many different variations of James's example, I can no longer reproduce this problem. It still raises an exception on 2.5 though.

[glyph]

Ugh, hit "submit" too early.

Under python 2.5 and 2.6, and Twisted trunk, using many different variations of James's example, I can no longer reproduce this problem. It still raises an exception on python 2.4 though.

I can't see what's changed. PyOS_setsig seems to be the same, at least in 2.5.

[jknight]

So what's left to do on that branch?

From a cusory review, I'd say:

• it needs a test.
• It shouldn't use the custom _sigchld.c module under python 2.6.