Skip to content

Commit 7243c59

Browse files
committed
runtime: let the signal watcher thread exit
Review feedback: the watcher was started as a goroutine with no exit condition. It blocked on a futex forever, so a program that finished with signals kept a thread parked on one for the rest of its life. Nothing observable broke — the thread is idle and process exit tears it down — but a loop with no way out is a property worth not having. The watcher exists only to serve enabled signals, so that is now 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. Stopping sets the flag, bumps the futex value and wakes it. 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 that is about to sleep. On the way out the watcher resets the futex to 0 so the next one can block on it. CAS loops rather than atomic.Uint32.Or/And — those methods are newer than some of the Go versions TinyGo builds against. Verified with a program that blocks on a channel (never time.Sleep, so delivery can only come from the watcher): the signal arrives, signal.Stop lets the thread exit, and a later signal.Notify starts a new watcher that delivers again.
1 parent cccac90 commit 7243c59

1 file changed

Lines changed: 68 additions & 5 deletions

File tree

src/runtime/runtime_unix.go

Lines changed: 68 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -396,38 +396,99 @@ func signal_enable(s uint32) {
396396
// on I/O or channels would otherwise never observe a signal (Ctrl+C would
397397
// be ignored). Start a dedicated watcher thread to cover that case. This is
398398
// a no-op for every other scheduler.
399-
startSignalWatcher()
399+
startSignalWatcher(s)
400400

401401
// It's easier to implement this function in C.
402402
tinygo_signal_enable(s)
403403
}
404404

405-
// signalWatcherStarted guards the one-time start of signalWatcher. signal_enable
406-
// is serialized by os/signal's handlers lock, but this stays defensive.
405+
// signalWatcherStarted guards the start of signalWatcher. signal_enable is
406+
// serialized by os/signal's handlers lock, but this stays defensive.
407407
var signalWatcherStarted atomic.Uint32
408408

409-
// startSignalWatcher starts the signal watcher thread the first time a signal is
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
410420
// enabled, but only under the threads scheduler (!hasScheduler && hasParallelism
411421
// is true only there). The cooperative and multicore schedulers process signals
412422
// from their idle loop (waitForEvents), and the "none" scheduler has no
413423
// goroutines, so none of them need this.
414-
func startSignalWatcher() {
424+
func startSignalWatcher(s uint32) {
415425
if hasScheduler || !hasParallelism {
416426
return
417427
}
428+
for {
429+
old := enabledSignals.Load()
430+
if enabledSignals.CompareAndSwap(old, old|(1<<s)) {
431+
break
432+
}
433+
}
434+
signalWatcherStop.Store(0)
418435
if signalWatcherStarted.Swap(1) == 0 {
419436
go signalWatcher()
420437
}
421438
}
422439

440+
// stopSignalWatcher lets the watcher thread return once the signal it was
441+
// serving is the last one to go away.
442+
//
443+
// Without this the thread is unstoppable by construction: it blocks on a futex
444+
// forever, so a program that finishes with signals keeps a thread parked on one
445+
// for the rest of its life. Nothing observable breaks — the thread is idle and
446+
// the process exit tears it down — but "no exit condition" is a property worth
447+
// not having, and it costs a flag and a wake to avoid.
448+
func stopSignalWatcher(s uint32) {
449+
if hasScheduler || !hasParallelism {
450+
return
451+
}
452+
// CAS rather than And/Or: those methods on atomic.Uint32 are newer than
453+
// some of the Go versions TinyGo builds against.
454+
var remaining uint32
455+
for {
456+
old := enabledSignals.Load()
457+
remaining = old &^ (1 << s)
458+
if enabledSignals.CompareAndSwap(old, remaining) {
459+
break
460+
}
461+
}
462+
if remaining != 0 {
463+
return // still serving something
464+
}
465+
if signalWatcherStarted.Swap(0) == 0 {
466+
return // not running
467+
}
468+
signalWatcherStop.Store(1)
469+
// Wake it so it can observe the flag. The value bump matters as much as the
470+
// wake: Wait(0) returns immediately if the futex is already non-zero, which
471+
// closes the window between the store above and a watcher about to sleep.
472+
signalFutex.Store(1)
473+
signalFutex.Wake()
474+
}
475+
423476
// signalWatcher runs on its own thread under the threads scheduler. It blocks on
424477
// signalFutex and resumes the signal-receiving goroutine (signal_recv) whenever
425478
// a signal arrives, decoupling signal delivery from sleepTicks(). It mirrors the
426479
// signal half of waitForEvents(), which the threads scheduler never calls.
480+
//
481+
// It returns when stopSignalWatcher says the last enabled signal has gone away.
427482
func signalWatcher() {
428483
for {
429484
// Block until the signal handler bumps the futex from 0 to 1.
430485
signalFutex.Wait(0)
486+
if signalWatcherStop.Load() != 0 {
487+
// Leave the futex as we found it for whoever runs next: a later
488+
// signal_enable starts a new watcher, and it must be able to sleep.
489+
signalFutex.Store(0)
490+
return
491+
}
431492
if signalFutex.Swap(0) != 0 {
432493
checkSignals()
433494
}
@@ -441,6 +502,7 @@ func signal_ignore(s uint32) {
441502
// receivedSignals into a uint32 array.
442503
runtimePanicAt(returnAddress(0), "unsupported signal number")
443504
}
505+
stopSignalWatcher(s)
444506
tinygo_signal_ignore(s)
445507
}
446508

@@ -451,6 +513,7 @@ func signal_disable(s uint32) {
451513
// receivedSignals into a uint32 array.
452514
runtimePanicAt(returnAddress(0), "unsupported signal number")
453515
}
516+
stopSignalWatcher(s)
454517
tinygo_signal_disable(s)
455518
}
456519

0 commit comments

Comments
 (0)