Re: SCM_SYSCALL

[Mark H Weaver]

Hi Ludovic,

address@hidden (Ludovic Courtès) writes:

> Mark H Weaver <address@hidden> skribis:
>
>> Hmm.  Shouldn't our signal handlers be run in a different thread?  Maybe
>> we can't make this change until 2.2, but it seems to me that there are
>> very serious problems trying to run signal handlers from within asyncs,
>> analogous to the problems running finalizers within asyncs.  Commonly,
>> signal handlers need to mutate some global state, but that cannot in
>> general be done safely from within asyncs, because asyncs might be
>> called while the global state is in an inconsistent state, at least for
>> data structures implemented in Scheme.
>>
>> What do you think?
>
> I think the rationale was that signal handlers in Guile would be a
> simplified version of what POSIX provides.  That is, they are called in
> the thread that called ‘sigaction’, and there are no restrictions on
> what procedures can be used from within the handler.  From that
> perspective, I think it fits the bill.
>
> Now, of course that introduces concurrency, but that’s what signals are
> about anyway: asynchronous notifications.  Thus I don’t have any
> particular problems with this implementation.

I looked more carefully, and agree that our current API is fine.  It
makes it easy to handle signals in a different thread, if desired, or to
avoid the complications of multi-threaded programming and rely instead
of blocking asyncs.

So, back to the problem at hand:

> However, with a fixed SCM_SYSCALL, the result is pretty much the same as
> with SA_RESTART (see <http://bugs.gnu.org/14640>): when SCM_ASYNC_TICK
> is called right after we get EINTR, chances are that the async hasn’t
> been queued yet, so we get back to our read(2) call, and thus the
> Scheme-level signal handler is never called.  (Typically, when running
> the test through strace, it passes, because the timing is “better”, but
> it fails without strace.)

Right, so the problem is that, when Guile is built with thread support,
our signal delivery mechanism depends on the signal handling thread
executing, which adds an unpredictable amount of latency.

Initially I looked at how to fix the test case to work around this
problem, but really I think we need to fix the way that signals are
delivered.  If one chooses to deliver signals to a thread that's doing a
'read' (or other interruptible system call), then we ought to arrange
things so that the async is queued in time to be run before restarting
the call.

I think the best solution is to get rid of our internal signal handler
thread altogether, and instead arrange for signals to be delivered
directly to the thread that the user specified, by setting the thread
signal masks appropriately.  The C-level signal handler would then set
some global state that would be noticed by the SCM_SYSCALL loop.

In some ways, this would bring us closer to the non-thread signal
handling mechanism in scmsigs.c, which queued the asyncs directly from
the signal handler.  Unfortunately, that code is not safe.  For example,
if the non-thread 'take_signal' (the second one in scmsigs.c) is run
while 'scm_async_click' (async.c) is in between the following two lines:

      asyncs = t->active_asyncs;
      t->active_asyncs = SCM_EOL;

Then the signal will be lost.  Other problems can happen if the
non-threaded 'take_signal' interrupts itself (e.g. if two different
signals are delivered at nearly the same time).

So we'd need to devise a new mechanism that _is_ safe.
It is certainly doable.

If you're okay with this general approach, I'll look into it.

What do you think?

      Mark