Skip to content

Commit e55041f

Browse files
committed
runtime: deliver signals under the threads scheduler when no goroutine sleeps
Under the threads scheduler there is no cooperative idle loop, so checkSignals() — which resumes the parked os/signal signal_recv goroutine — was only ever reached from sleepTicks(). A signal was therefore only noticed while some goroutine happened to be inside time.Sleep, and a program blocked purely on I/O, channels, mutexes or timers (time.NewTicker uses the timer queue, not sleepTicks) never observed it at all. A dedicated signal-watcher thread starts the first time a signal is enabled, gated to the threads scheduler (!hasScheduler && hasParallelism). It blocks on signalFutex and calls checkSignals() on wake, mirroring the signal half of waitForEvents() that the cooperative scheduler runs from its idle loop. Other schedulers are unaffected: the start is a compile-time no-op for them. The watcher exists only to serve enabled signals, so that is its lifetime. enabledSignals tracks the set os/signal wants delivered, the last signal_disable/signal_ignore stops the thread, and a later signal_enable starts a fresh one. Without that it blocked on a futex forever, so a program that had finished with signals kept a thread parked on one for the rest of its life — nothing observable broke, since the thread is idle and process exit tears it down, but a loop with no way out is a property worth not having. Stopping sets the flag, bumps the futex value and wakes ALL waiters. The bump matters as much as the wake: Wait(0) returns immediately when the futex is already non-zero, which closes the window between the store and a watcher about to sleep. WakeAll matters because the watcher is not the only thing sleeping on signalFutex — sleepTicks and waitForEvents do too — and waking a single waiter could wake a sleeping goroutine instead, which consumes the value with its own Swap and leaves the watcher asleep on a futex that is 0 again, never seeing the stop flag. That is the thread leak the stop exists to prevent. The signal handler already uses WakeAll on this futex for the same reason. On the way out the watcher resets the futex to 0 so the next one can block on it. testdata/signal.go now blocks on the receive rather than on a sleep. The sleep was doing the delivery rather than waiting for it: sleepTicks waits on the same futex the signal handler bumps and calls checkSignals on the way out, so the signal arrived on the back of the sleep whatever else was running, and the test passed either way — the wrong property for the test guarding this fix. Blocking on the receive parks the only goroutine there is, so under the threads scheduler the watcher is the only thing left that can deliver. Checked by disabling the watcher: with the sleep the test still passes, with the receive it hangs and is killed. Output is unchanged, so signal.txt stays as it is. Verified: a channel/Accept-blocked program with no time.Sleep receives SIGINT, signal.Stop lets the thread exit, a later signal.Notify starts a new watcher that delivers again, and the skycoin daemon — previously unkillable with Ctrl+C under TinyGo — shuts down cleanly on SIGINT, both idle and during active block sync.
1 parent 801bd48 commit e55041f

2 files changed

Lines changed: 111 additions & 13 deletions

File tree

src/runtime/runtime_unix.go

Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -390,17 +390,114 @@ func signal_enable(s uint32) {
390390
// scheduler (and therefore there is no parallelism).
391391
hasSignals = true
392392

393+
// Under the threads scheduler there is no scheduler idle loop to notice
394+
// signals: checkSignals() is only reached from sleepTicks(), i.e. while
395+
// some goroutine happens to be inside time.Sleep. A program blocked purely
396+
// on I/O or channels would otherwise never observe a signal (Ctrl+C would
397+
// be ignored). Start a dedicated watcher thread to cover that case. This is
398+
// a no-op for every other scheduler.
399+
startSignalWatcher(s)
400+
393401
// It's easier to implement this function in C.
394402
tinygo_signal_enable(s)
395403
}
396404

405+
// signalWatcherStarted guards the start of signalWatcher. signal_enable is
406+
// serialized by os/signal's handlers lock, but this stays defensive.
407+
var signalWatcherStarted atomic.Uint32
408+
409+
// signalWatcherStop asks signalWatcher to return. The watcher reads it after
410+
// waking, so stopping it means setting this and then waking the futex.
411+
var signalWatcherStop atomic.Uint32
412+
413+
// enabledSignals is the set of signals os/signal currently wants delivered. The
414+
// watcher thread exists only to serve them, so it runs exactly while this is
415+
// non-zero: the last disable/ignore stops it, and a later enable starts a fresh
416+
// one.
417+
var enabledSignals atomic.Uint32
418+
419+
// startSignalWatcher starts the signal watcher thread when the first signal is
420+
// enabled, but only under the threads scheduler (!hasScheduler && hasParallelism
421+
// is true only there). The cooperative and multicore schedulers process signals
422+
// from their idle loop (waitForEvents), and the "none" scheduler has no
423+
// goroutines, so none of them need this.
424+
func startSignalWatcher(s uint32) {
425+
if hasScheduler || !hasParallelism {
426+
return
427+
}
428+
enabledSignals.Or(uint32(1) << s)
429+
signalWatcherStop.Store(0)
430+
if signalWatcherStarted.Swap(1) == 0 {
431+
go signalWatcher()
432+
}
433+
}
434+
435+
// stopSignalWatcher lets the watcher thread return once the signal it was
436+
// serving is the last one to go away.
437+
//
438+
// Without this the thread is unstoppable by construction: it blocks on a futex
439+
// forever, so a program that finishes with signals keeps a thread parked on one
440+
// for the rest of its life. Nothing observable breaks — the thread is idle and
441+
// the process exit tears it down — but "no exit condition" is a property worth
442+
// not having, and it costs a flag and a wake to avoid.
443+
func stopSignalWatcher(s uint32) {
444+
if hasScheduler || !hasParallelism {
445+
return
446+
}
447+
// And returns the value BEFORE the mask was applied, so clear the bit from
448+
// it to get what is left enabled.
449+
bit := uint32(1) << s
450+
if enabledSignals.And(^bit)&^bit != 0 {
451+
return // still serving other signals
452+
}
453+
if signalWatcherStarted.Swap(0) == 0 {
454+
return // not running
455+
}
456+
signalWatcherStop.Store(1)
457+
// Wake it so it can observe the flag. The value bump matters as much as the
458+
// wake: Wait(0) returns immediately if the futex is already non-zero, which
459+
// closes the window between the store above and a watcher about to sleep.
460+
//
461+
// WakeAll rather than Wake because the watcher is not the only thing that
462+
// sleeps on this futex: sleepTicks waits on it too, and so does
463+
// waitForEvents. Waking one waiter could wake a sleeping goroutine instead
464+
// — which would consume the value with its own Swap and leave the watcher
465+
// asleep on a futex that is 0 again, never seeing the flag it was told to
466+
// look at. The signal handler wakes this futex the same way.
467+
signalFutex.Store(1)
468+
signalFutex.WakeAll()
469+
}
470+
471+
// signalWatcher runs on its own thread under the threads scheduler. It blocks on
472+
// signalFutex and resumes the signal-receiving goroutine (signal_recv) whenever
473+
// a signal arrives, decoupling signal delivery from sleepTicks(). It mirrors the
474+
// signal half of waitForEvents(), which the threads scheduler never calls.
475+
//
476+
// It returns when stopSignalWatcher says the last enabled signal has gone away.
477+
func signalWatcher() {
478+
for {
479+
// Block until the signal handler bumps the futex from 0 to 1.
480+
signalFutex.Wait(0)
481+
if signalWatcherStop.Load() != 0 {
482+
// Leave the futex as we found it for whoever runs next: a later
483+
// signal_enable starts a new watcher, and it must be able to sleep.
484+
signalFutex.Store(0)
485+
return
486+
}
487+
if signalFutex.Swap(0) != 0 {
488+
checkSignals()
489+
}
490+
}
491+
}
492+
397493
//go:linkname signal_ignore os/signal.signal_ignore
398494
func signal_ignore(s uint32) {
399495
if s >= 32 {
400496
// TODO: to support higher signal numbers, we need to turn
401497
// receivedSignals into a uint32 array.
402498
runtimePanicAt(returnAddress(0), "unsupported signal number")
403499
}
500+
stopSignalWatcher(s)
404501
tinygo_signal_ignore(s)
405502
}
406503

@@ -411,6 +508,7 @@ func signal_disable(s uint32) {
411508
// receivedSignals into a uint32 array.
412509
runtimePanicAt(returnAddress(0), "unsupported signal number")
413510
}
511+
stopSignalWatcher(s)
414512
tinygo_signal_disable(s)
415513
}
416514

testdata/signal.go

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -8,28 +8,28 @@ import (
88
"os"
99
"os/signal"
1010
"syscall"
11-
"time"
1211
)
1312

1413
func main() {
1514
c := make(chan os.Signal, 1)
1615
signal.Notify(c, syscall.SIGUSR1)
1716

18-
// Wait for signals to arrive.
19-
go func() {
20-
for sig := range c {
21-
if sig == syscall.SIGUSR1 {
22-
println("got expected signal")
23-
} else {
24-
println("got signal:", sig.String())
25-
}
26-
}
27-
}()
28-
2917
// Send the signal.
3018
syscall.Kill(syscall.Getpid(), syscall.SIGUSR1)
3119

32-
time.Sleep(time.Millisecond * 100)
20+
// Receive it directly, with nothing sleeping anywhere.
21+
//
22+
// The sleep this replaces was doing the delivery: sleepTicks waits on the
23+
// same futex the signal handler bumps and calls checkSignals on the way
24+
// out, so a signal arrived on the back of the sleep. That hid whether
25+
// anything else delivers it. Blocking on this receive parks the only
26+
// goroutine there is, so under the threads scheduler the signal watcher is
27+
// the only thing left that can — and if it does not, this hangs.
28+
if sig := <-c; sig == syscall.SIGUSR1 {
29+
println("got expected signal")
30+
} else {
31+
println("got signal:", sig.String())
32+
}
3333

3434
// Stop notifying.
3535
// (This is just a smoke test, it's difficult to test the default behavior

0 commit comments

Comments
 (0)