[UPDATED: TclX patch available] Potential solution to "Make TclX's signal trap handlers safe to use with threaded Tcl"

[fredericbonnet]

This issue is not a TclX-specific problem but a bug/shortcoming in the Tcl core, TclX just happens to be the simplest way to expose it at script level. The ticket below provides such a script, however it is entirely possible to reproduce this phenomenon using the C API (more specifically, a combination of Tcl async and low-level signal handlers on which TclX relies).

https://core.tcl.tk/tcl/tktview/f4f44174

I was able to reproduce the problem eventually after several hours, and the deadlock is indeed caused by the async thread self-deadlocking while attempting to lock its mutex twice.

The bug is difficult to reproduce because it's very timing-sensitive, however one can force the hand of destiny by artificially slowing down the async thread, that's what I did and it makes the process deadlock immediately. Tcl_AsyncInvoke has a mutex-protected loop over its registered handlers, so I've just added a sleep(0) within the loop and the test script hangs immediately for the exact same reason (double locking).

Tcl uses pthreads on Unix and Win32 critical sections on Windows. Win32 CSs are reentrant but pthread mutexes are not by default (you have to use the PTHREAD_MUTEX_RECURSIVE and this feature is not available on all systems). So when the async thread is interrupted by a signal while in the middle of a mutex-protected operation, it deadlocks itself on Unix but not on Windows.

The Tcl docs say:

The result of locking a mutex twice from the same thread is undefined. On some platforms it will result in a deadlock.

So this is the expected behavior, however nothing prevents the core from using reentrant mutexes on all platforms that Tcl supports. Implementing reentrancy using non-reentrant mutexes is a trivial task, so OS support is a non-issue. I'm going to write a TIP to make Tcl_Mutex reentrant on all platforms starting at version 8.7.

I've already implemented a quick hack to make mutexes reentrant on Unix:

• a first version uses the native PTHREAD_MUTEX_RECURSIVE,
• a second version uses regular mutexes with a per-thread call counter.

Both versions fix the deadlock problem with or without my extra sleep(0), and with no impact on Tcl tests.

[fredericbonnet]

I've just published TIP #509 here:

https://core.tcl.tk/tips/doc/trunk/tip/509.md

The implementation is available on branch tip-509 in the Fossil repo. Feedback welcome.

In the unfortunate case that the TIP be rejected by the TCT, I have an alternate solution that addresses the original issue by adding reentrancy to the async handler mutex alone. This would be a simple fix to the original ticket f4f44174, and as such it wouldn't require any TIP to reach the core (only proper review from the maintainer). But I think the TIP is a more generic solution to this class of problems.

[undroidwish]

Further ideas regarding my last comment on https://core.tcl.tk/tcl/tktview/f4f44174

Instead of directly calling sigprocmask(2), the tclXsignal.c module needs to call a new (to be defined) Tcl core function which interacts with the notifier thread in order to query or alter its signal mask.

The code path of Tcl_AsyncMark() currently needs to acquire 3 (three!) mutexen before it sets a thread specific flag and carries out a pthread_cond_broadcast() on that thread. No single call of this path seems to be async-signal-safe according to http://man7.org/linux/man-pages/man7/signal-safety.7.html

Safe alternatives are possibly write(2) and sem_post(3), but this most likely requires a full and incompatible redesign (of which I have no idea yet).

[undroidwish]

A follow up on my last post. Check-in https://www.androwish.org/index.html/info/41498ca0a57c2aa7 now implements most of the outlined ideas. POSIX signals (per Tclx and Tcl_AsyncMark) are routed through the notifier thread which delivers it synchronously as part of its main loop. Tcl_AsyncMark() uses (in case it runs in the notifier thread) only the pthread_self() and write() system calls (which are async-signal-safe). A rendez-vous approach is implemented which allows the signal command of Tclx to invoke pthread_sigmask() in the notifier thread in order to block and unblock individual signals.

The test case given in the https://core.tcl.tk/tcl/tktview/f4f44174 ticket seems to work on x86_64 linuxen.

[fredericbonnet]

Forwarding a message from Christian Werner (AKA undroidwish on GitHub and chw on core.tcl.tk) on the Tcl-Core list:

Hello Fred,

IMO TIP #509 is consistent for one definite mutex semantic on all platforms. However, I believe it is not part of the signal bounty problem. Thus, I've tried another approach on AndroWish/undroidwish
which follows core-8-6-branch:

1. use the notifier task to handle signals exclusively by having the first (main) thread blocking all signals, remembering its signal mask, and in the notifier thread re-install this signal
   mask before blocking in select() and blocking all signals after select() returned. This gives a very clear view at runtime when signals are to be handled.

2. add function TclAsyncInNotifier() in tclUnixNotfy.c which sets an integer to a specified value and writes a single byte to the trigger pipe when called from the notifier thread. Both pthread_self() and write() may be used in a signal handler.

3. change tclAsync.c to call the function from point 2 before going the old fashioned way of taking mutexes and alert the target thread (which shouldn't happen anyway, due to all
   signals being blocked in all threads except the notifier).

4. add function TclAsyncMarkFromNotifier() in tclUnixNotfy.c which deals with those async handlers which where flagged by the function from point 2. This is called as part of the notifier's thread main loop and Tcl_AsyncMarks() all async handlers caught by the notifier thread.

5. changed TSD specific locking in tclAsync.c to a single process wide mutex since signals are a process characteristic

So far, the signal handler path should be free of mutex locks, provided everything signal ever is handled in the notifier thread.

Now, the left over Tclx signal features are the "signal block" and "signal unblock" commands. A solution implemented is to add a new platform stub called TclpSigProcMask which has the signature of sigprocmask/pthread_sigmask and carries out an RPC in the notifier thread to modify its signal mask.

The currently implemented code is available for both, unix and macosx platforms with the last check-in on

https://www.androwish.org/index.html/info/28bd3791feec27a5

With these changes the code snippet from ticket https://core.tcl.tk/tcl/tktview/f4f44174 doesn't lock up
after few hundred or thousand iterations anymore.

Now for the problems, questions, ideas, etc.

• when the new epoll/kqueue approach is in effect, there's no more notifier thread. Would a new signal handler thread be needed?

• Linux supports a signalfd(2) file descriptor type which allows to synchronously deal with signals. Would that fit the epoll approach? Have BSDen/MacOS something similar?

• the AndroWish beast is different: the Tcl main thread is spit out of a threaded black hole whose signal disposition remains in the darkest darkness. How to deal with it?

Comments welcome,
Christian

[fredericbonnet]

I respectually disagree on the point that TIP #509 doesn't address the original
problem. Let me explain.

As you wrote yourself in your last section, some thread-enabled Tcl cores don't
have a notifier thread. This means that in simple, single-threaded cases, a
thread-enabled core behaves the same as a thread-disabled one, mutexes aside.
The code paths are the same but Tcl_Mutex functions are:

• no-ops in thread-disabled cores,
• pthread mutexes in thread-enabled cores.

I managed to reproduce the problem described in ticket
https://core.tcl.tk/tcl/tktview/f4f44174 by splitting the original 2-threaded
scripts into one single-threaded signal target process and one signal emitter
process. The signal target script deadlocks eventually because standard Tcl
mutexes are not reentrant. Here is the signal receiver script, notice that it no
longer uses the Thread package:

package require Tclx

signal trap SIGHUP handler

proc handler args {
    puts stderr "SIGHUP [incr ::HUPS] $args"
}

proc gotinput {chan} {
    gets $chan d
    puts "hupity $d"
}

puts [pid]
vwait result

And here is the signal emitter, written in Tcl though it could be written in any
other language actually:

package require Tclx
set pid [lindex $argv 0]
while 1 {
    kill HUP $pid
    puts [incr ::HUP]
}

The test procedure is as follows:

• Run first script with the tclsh version you want to test
• Note the PID printed on stdout
• Run second script with any tclsh version you want, passing the PID as first
  argument

And the results:

1. On a thread-disabled tclsh, the first script runs just fine for millions of
   signals sent.
2. On a thread-enabled tclsh, it deadlocks eventually when the signal is
   caught in a mutex-protected section of the async handler (just as the bounty
   states).
3. On a thread-enabled tclsh with reentrant mutexes, it behaves exactly the
   same as the thread-disabled version.

On my standard Linux system, the Tcl notifier uses epoll, and all 3 instances
above are single-threaded as they should be. On systems that use select you have
one extra notifier thread but I don't think this changes anything to the
observed behavior (the deadlock occurs before the notifier thread is triggered).
Your proposed approach is interesting but implies that we reinstall a notifier
thread on epoll-based cores, which adds an extra layer of complexity and
potential performance problems.

Your analysis of the signal-safety of the async handler's code path is very
valuable, however :

• Tclx signalling has been there for years on thread-disabled Tcl so we can
  safely assume that it works without major trouble, and it's outside the scope
  of the bounty anyway. And I don't think signal-safety is at play in the scope
  of the Tcl core (see below).

• But if we are really concerned with signal-safety then we'd better start
  working on improving Tclx (see below) ; for example it uses sigprocmask even
  though POSIX states that:

  The use of the sigprocmask() function is unspecified in a multi-threaded
  process.

• The only difference between the 3 single-threaded test contexts above is
  Tcl_Mutex semantics. So multithreading IS NOT the root cause of the
  deadlock problem ; mutex-reentry (or lack thereof) IS the root cause. And
  removing mutexes implies a significant refactoring of the existing Tcl core.

• I strongly believe that signal handling should be kept outside of the Tcl
  core, as it will open a whole new can of worms. Tcl doesn't deal with signals
  itself, it only provides the facilities to deal with asynchronous events (even
  though the man page cites signal handling as an example). In our case, Tclx is
  the sole responsible for signal handling and Tcl should know nothing about it
  IMHO. If Tcl starts dealing with signals then it should
  do it completely and provide all the necessary features, this would amount to
  merging parts of Tclx into the core but it's waaaay outside the scope of the
  bounty. More on that below.

You also suggests that only the notifier thead should receive signals and that
they should be blocked everywhere else. I don't think that's going to work
because there is nothing that prevents client code from installing its own
signal handlers, especially given that Tcl is totally signal-agnostic (hence
Tclx). Nor does it prevent client code from starting its own threads without
using Tcl API (a common situation when Tcl is embedded as a library or when
integrating with third-party libraries that spawn their own threads). At the
very least, the notifier thread (when it exists), and all the 'utility' threads
in general, should rather block all signals as a defensive measure using
pthread_sigmask to ensure that they get routed to the right recipient and not
to itself.

Moreover, centralizing signals on the notifier thread introduces a
single point of failure and a potential bottleneck with performance issues, on
all versions of the core even those with thread-local notification (epoll).

A last point: both Tcl notifier and async APIs are public, and specifically
designed to be used in a variety of situations (see the introduction of the
Notifier man page and the mention of external event loops). So you can make no
assumption on the way these APIs will be called in client code. Implementors can
create their own notifiers, and the Async man page explicitly states that:

Applications may also call Tcl_AsyncInvoke at interesting times for that application

That's how the NRE engine deals with async events during tailcall actually.

So you can make no assumption on how the async and notifier features will be
used and what kind of impact your changes will have on existing code, because
nothing prevents client code from building apps that are very different from
Tclsh from an architectural point of view and yet use the same public API. Tclsh
and Wish are just client apps among others.

About Tclx: TOP #509 only addresses the bounty issue on the Tcl core side.
I'm currently working on the Tclx side to ensure that there are no other
problems with signal handling in multi-threaded contexts (such as the
sigprocmask issue described above) and plan to propose a patch soon, but this
has nothing to do with the Tcl core and thus doesn't require a TIP or the
involvement of the Tcl Core team.

[undroidwish]

Fred, I see your point regarding assumptions about the runtime environment. And I'd like your TIP to be adopted, soon, not later. However, can we please agree on these points:

0. signal handlers are a process wide resource, signal masks are thread specific
1. POSIXly correct signal handlers are subject to a limited safe set of system/libc functions
2. signal handlers are executed in an arbitrary thread subject to thread specific signal masks
3. a specified way to asynchronously deal with a Tcl thread is calling Tcl_AsyncMark on a Tcl_AsyncHandler which is tied to that thread

Now the implications as interpreted by my humble self:

Points 2 and 3: the thread addressed by Tcl_AsyncMark is not necessarily the thread which runs the signal handler.

Point 1: pthread_mutex_lock/pthread_mutex_unlock are not part of the safe set, independent of being recursive, non-recursive, process-shared, or whatever. And this is my main argument, that TIP#509 is not part of the solution (and I should have been more precise here) how a signal handler achieves to safely set a mark on the async handler.

Best,
Christian

[undroidwish]

One more thought while glimpsing of the list of signal safe syscalls (http://man7.org/linux/man-pages/man7/signal-safety.7.html):

Instead of handling signals in a dedicated thread it should be possible in a special signal safe version of Tcl_AsyncMark to determine the current thread using pthread_self, compare it against the originThreadId of the AsyncHandler struct. In case the signal was caught by a different thread, it is now time to do a pthread_kill with the same signal number but with the owning originThreadId of the AsyncHandler struct as the target.

But the root problem remains: the mark on the AsyncHandler must be set using means which are guaranteed to be async-signal-safe.

Thus, at least two core changes seem unavoidable:

1. a signal aware version (including signal number) of Tcl_AsyncMark
2. a code path free of pthread_mutex_* from Tcl_ThreadAlert down to Tcl_AlertNotifier

[fredericbonnet]

I don't have the time to post a longer answer to both your messages right now but the solution I'm working on works along these lines: let Tclx trap async signals in a dedicated thread (using sigwait for example), then forwards them asynchronously to an other thread using async-safe functions such as sem_post. That second thread can then call Tcl_AsyncMark safely. At present Tclx calls Tcl_AsyncMark directly from the signal handler.

More on that later.

[undroidwish]

The longer I think over Tcl_AsyncMark reading the first sentence of the manpage ("These procedures provide a safe mechanism for dealing with asynchronous events such as signals...") the more am I convinced that the current implementation in Tcl 8.6 is plain wrong when the core is built with threading enabled. Of course, there's no problem when the core is un-threaded, and for this the design still holds. However, now that un-threaded builds are phased out, there's a pressing need for reform, IMHO.

Your outline using a thread which does sigwait sounds interesting, and BTW isn't that different to my approach in the notifier thread. But then, do both solutions not both introduce a bottleneck and SPOF? And still seems sigwait only half the solution, since it does not channel the kernel's signal delivery, but pthread_sigmask does.

Meanwhile, I think we're in desperate need of a Tcl_AsyncMarkEx(Tcl_AsyncHandler async, int sigNumber), which can do on POSIX something like

#if TCL_THREADS
  if (Tcl_GetCurrentThread() != async->originThreadId) {
    /* Redirect the signal to the target thread owning the async handler. */
    pthread_kill((pthread_t) async->originThreadId, sigNumber);
    return;
  }
#endif
  /*
   * Do some async-signal-safe magic to wake up the thread, which
   * must be now the thread owning the async handler,
   * e.g. by writing to the eventFd or triggerPipe
   */

Still something more is needed to allow proper semantic of the "signal block" and "signal unblock" Tclx commands, but this could boil down to a simple per async handler mask and flag when a little more reform is carried out.

To extrapolate a flawed interface or to introduce binary incompatibility, that is the fine question.

[undroidwish]

Meanwhile my first approach evolved, see check-in https://www.androwish.org/index.html/info/40790af1e8e4ec9f

The basic idea is now to redirect signals to the notifier thread using a new core function Tcl_AsyncMarkFromSignal(), which compared to Tcl_AsyncMark() has the signal number added. For the normal notifier this is the implementation:

void TclAsyncNotifier(int sigNumber, Tcl_ThreadId threadId, int *flagPtr, int value)
{
    if (pthread_equal(pthread_self(), (pthread_t) notifierThread)) {
        if (notifierThreadRunning) {
            *flagPtr = value;
            asyncPending = 1;
            write(triggerPipe, "S", 1);
        }
        return;
    }
    pthread_kill((pthread_t) notifierThread, sigNumber);
}

The notifier thread is run with all signals blocked but unblocks/reblocks all signals when performing it's select() system call.

For the new epoll/kqueue notifier the function above needs to test for the target thread of the AsyncHandler, if unequal it has to redirect the signal to the target thread. The signal handling in the target thread then finally writes a single byte to the pipe/event_fd of the target thread's notifier file descriptor.

Overall result: the code path in signal contexts is clean regarding the use of async-signal-safe system calls.

Regarding the "signal" command in Tclx, I've replaced the sigprocmask() calls with few global variables which track the blocked/unblocked/pending state for all signals. Still needs the "signal" command a major redesign to proper support threading.

[fredericbonnet]

Here is the patch that I sent to Peter Da Silva from FlightAware last week, I couldn't post it here directly because I had ISP issues and no access to the web.

signalthread.zip

This solution is much simpler and it involves no changes to the Tcl Core (as it should be IMHO). On MT builds we use a dedicated thread for signal processing in order to avoid deadlocking and signal-unsafe calls in Tcl_AsyncMark:

• The signal-handling thread waits for all incoming signals using sigwait() and calls Tcl_AsyncMark() safely each time it wakes up.
• Signal handlers called outside of the signal-handling thread forward trapped signals to this thread using pthread_kill().
• sigprocmask() behavior is undefined in multithreaded processes according to POSIX, and were replaced with pthread_sigmask().

I've validated this architecture with success in a separate proof-of-concept implementation that tested a variety of configurations (threads created/destroyed and signals blocked/unblocked dynamically at different times), and it behaved reliably even under heavy load and hundreds of millions of signals sent. It showed that no further changes were needed for neither TclX nor the Tcl core, especially anything involving signal handlers and masks. More specifically, there is no need to block signals explicitly in newly spawned threads, as the global signal handlers are thread-agnostic, and thus no need for e.g. thread hook procs for signal setup.

I haven't noticed any difference in performances of behavior (deadlocks notwithstandinn obviously) between the unpatched and patched versions.

On the Tcl side this solution is independent from the actual technique used by the Tcl notifier (select, epoll...).

The side effect is that Tcl #509 no longer relates to this bounty issue so I'm going to change the title accordingly.

PS: the patch also adds some bits of thread-safety in TclX global state management but it's unrelated to the root issue.

[mutability]

A couple of comments:

1. blocking all signals in the signal handling thread is probably not right. It means that all signals delivered directly to that thread will be treated as if they had a tclx-installed handler, even if they actually have a different handler or a default disposition. A simple way to demonstrate this is to send a SIGINT (which should kill the process with no handler installed) to the signal-handling thread:

% package require Tclx
8.4
% vwait forever
SIGINT signal received
SIGINT signal received
SIGINT signal received

2. The signal handling thread needs to be resurrected somehow over fork (nb: just plain fork, not fork-exec):

package require Tclx

signal trap ALRM {puts stderr "got an alarm signal in process [pid]"}

if {[fork] == 0} {
    puts "[pid] is the child"
    alarm 5
    vwait forever
}

puts "[pid] is the parent"
alarm 5
vwait forever

13675 is the parent
13677 is the child
got an alarm signal in process 13675
[... nothing more from the child ...]

[fredericbonnet]

Sorry I can't reproduce the first test case:
First shell:

vagrant@vagrant:~$ tclsh8.7
% package require Tclx
8.4
% pid
3308
% vwait forever

vagrant@vagrant:~$ 

Second shell:

vagrant@vagrant:~$ kill -SIGINT 3308
vagrant@vagrant:~$

However if this causes problems then I have a fix ready. It involves restarting the signal-handling thread each time the mask is modified, it works very well. Actually, the first version of my PoC used this method before I realized the current implementation was sufficient (well, maybe not after all).

About the second test case, I have to admit I didn't see it coming :) However the fix is super-easy, it uses pthread_atfork() to reinstall the signal-handling thread in the newly forked child:

vagrant@vagrant:~$ tclsh8.7
% package require Tclx
8.4
% signal trap ALRM {puts stderr "got an alarm signal in process [pid]"}
% if {[fork] == 0} {
    puts "[pid] is the child"
    alarm 5
    vwait forever
}
3317 is the child
% puts "[pid] is the parent"
3315 is the parent
% alarm 5
0.0
% vwait forever
got an alarm signal in process 3317
got an alarm signal in process 3315

Here is the new patch:
signalthread.v2.patch.zip