fix: restore the terminal on SIGINT/SIGQUIT (closes #12)
The port installed handlers for SIGHUP and SIGTERM only, so SIGINT and
SIGQUIT killed the process with tcell still holding the tty and dropped
the user into a shell with no echo and a scrambled screen. All four
signals now go to one os/signal channel read by one goroutine, and every
path calls Terminal.Fini before os.Exit(0) -- C's leave(), "leave
quickly but curteously" (main.c).
The handlers are installed immediately after term.New(), the call that
raises raw mode, rather than after the game exists. Everything between
those two points ran raw with no handler at all: the save-restore path,
and -d's DeathDemo(), which never returns -- death() blocks in
waitFor('\n') (game/rip.go) -- so a kill -INT during the death demo left
exactly the scrambled terminal this fixes. The game is handed to the
handler afterwards through pendingSaver, whose AutoSave is a no-op until
then: a signal before the game is built restores the terminal and exits
with nothing to save. SIGHUP/SIGTERM autosave on the play path is
unchanged.
The save decision, written into the savesOnSignal comment: SIGHUP and
SIGTERM keep autosaving; SIGINT and SIGQUIT restore and exit without
saving. No path in C saves on INT or QUIT (leave() is endwin-and-exit,
quit() confirms/scores/exits, endit() goes through fatal(), and
save.c auto_save is reserved for HUP/TERM), the semantics agree
(involuntary teardown is worth rescuing a game from; a deliberate "stop
now" must not become a one-keystroke checkpoint against an anti-save-scum
save discipline), and it is the safe choice, since AutoSave gob-encodes
live state the main goroutine is still mutating after removing the old
file.
One reader of one signal is also what closes the corruption window: a
second signal arriving while a SIGHUP's AutoSave is mid-write stays
unread in the buffer instead of exiting out from under the writer.
cmd/rogue/main_test.go pins the membership of handledSignals() itself --
the rest of the file iterates that set, so without that assertion the
suite would pass against a set that had lost SIGINT and SIGQUIT again,
which is the regression this issue exists to prevent -- and covers the
ordering per signal, the save/no-save split (driven from the expectation
table so every entry is read), the mid-save second-signal interleaving,
the pre-game pendingSaver window, and real SIGINT/SIGQUIT/SIGHUP/SIGTERM
delivered to the test process through the same notifySignals wiring the
game uses.
Two premises behind the report were wrong and are recorded rather than
silently fixed: leave() is not installed on SIGINT/SIGQUIT during play
(the wiring is in mdport.c; the shipped build calls md_onsignal_default
and installs nothing, and leave() appears only in the endgame paths of
rip.c and main.c), and Ctrl-C never generated SIGINT here anyway, since
tcell's raw mode clears ISIG and the key arrives as byte 0x03 -- as it
did in C, whose setup() calls curses raw(). The real exposure is
kill -INT / kill -QUIT, a SIGINT to the process group while the ! shell
escape has the screen suspended, and the window after term.New()
described above. Nothing is raw before term.New(), so there was never
anything to cover there.
ARCHITECTURE.md section 9 gains rows for SIGTSTP/tstp() (deliberately
dropped: raw mode means Ctrl-Z cannot reach the process, suspending the
screen from the signal goroutine is a logical race against the drawing
goroutine -- not a data race, since tcell guards Suspend/Resume and Fini
alike -- and C armed tstp only after a successful restore(); the ! shell
escape covers the need), for SIGINT not routing to the interactive
quit() prompt, and for auto_save on the fault signals. Section 5.3's
claim that tcell handles SIGTSTP was false -- tcell registers only
SIGWINCH -- and is corrected, and its "every path restores the terminal"
claim now holds because of the install ordering above.
This commit is contained in:
@@ -1513,8 +1513,27 @@ Ported drawing code keeps its structure: `mvaddch(y, x, ch)` →
|
||||
buffer, preserving the "screen is a data structure" idiom without touching the
|
||||
real terminal. `md_readchar`'s escape decoding is deleted; tcell's `EventKey`
|
||||
provides decoded keys and we translate to the byte codes `command()` already
|
||||
handles (KeyUp → 'k', etc.). SIGTSTP/resume and resize are handled by tcell;
|
||||
SIGHUP/SIGTERM → `autoSave` via `os/signal`.
|
||||
handles (KeyUp → 'k', etc.). Resize is handled by tcell, which registers only
|
||||
SIGWINCH — SIGTSTP is not among the signals it takes, and is dropped (§9).
|
||||
|
||||
Signals are handled in `cmd/rogue/main.go`: one `os/signal` channel read by one
|
||||
goroutine that reads exactly one signal, so a second signal can never call
|
||||
`os.Exit` out from under an in-flight save. SIGHUP and SIGTERM `AutoSave` on the
|
||||
way out (save.c `auto_save`); SIGINT and SIGQUIT restore the terminal and exit
|
||||
without saving, matching C — where `auto_save` is reserved for HUP/TERM and
|
||||
neither `leave()` nor `quit()` nor `endit()` writes a save file — and keeping a
|
||||
deliberate interrupt from becoming a free checkpoint.
|
||||
|
||||
The handlers are installed immediately after `term.New()`, which is the call
|
||||
that puts the tty in raw mode, and before the game exists — the saver is handed
|
||||
over afterwards through `pendingSaver`. That ordering is what makes "every path
|
||||
restores the terminal via `Terminal.Fini` before exiting" actually true: both
|
||||
the save-restoring path and `-d`'s `DeathDemo()` run with the tty already raw,
|
||||
and `DeathDemo()` never returns (it blocks in `waitFor` inside `death()`), so
|
||||
handlers installed after them would leave that whole stretch unarmed. Restoring
|
||||
the terminal is the part that must be armed the instant the tty goes raw, and it
|
||||
needs no game; a signal arriving before the game is built restores and exits
|
||||
with nothing to save. That is C's `leave()`, "leave quickly but curteously".
|
||||
|
||||
### 5.4 Messaging
|
||||
|
||||
@@ -1715,6 +1734,44 @@ exit. Those are the steps referenced above (e.g. "step 5", "step 7").
|
||||
| tty dsusp/ltc character juggling | tcell owns the tty | none |
|
||||
| shell escape (`!`) setuid dance | no privileges to drop | plain `os/exec` shell |
|
||||
| curses window save in save file | screen is derivable | redraw on restore |
|
||||
| `tstp()` SIGTSTP suspend/resume | see below | `!` shell escape |
|
||||
| SIGINT → the interactive `quit()` prompt | see below | `Q`; SIGINT exits cleanly |
|
||||
| `auto_save` on SIGILL/TRAP/FPE/BUS/SEGV/SYS | see below | none |
|
||||
|
||||
The three signal rows warrant more than a table cell.
|
||||
|
||||
**SIGTSTP / `tstp()`.** Not handled, deliberately. Ctrl-Z cannot reach the game
|
||||
as a signal in the first place: tcell puts the tty in raw mode (`term.MakeRaw`
|
||||
clears `ISIG`, which is what makes `VSUSP` live), so Ctrl-Z arrives as key byte
|
||||
`0x1a`, exactly as it did in C, whose `setup()` calls curses `raw()` for the
|
||||
same effect. Only an explicit `kill -TSTP` can deliver it, which is not a player
|
||||
action. Handling it properly would mean calling `Screen.Suspend`/`Resume` from
|
||||
the signal goroutine while the game goroutine may be inside `Render` or
|
||||
`PollEvent`. That is not a _data_ race — tcell guards `Suspend`/`Resume` and
|
||||
`Fini` alike with the screen mutex, which is also why the `Fini` this port does
|
||||
call from the signal goroutine is safe — it is a _logical_ race over the screen
|
||||
state: the game goroutine can redraw into a screen the handler has just
|
||||
suspended, or resume under a half-finished frame. Getting it right means
|
||||
plumbing the signal through the input loop and handling it synchronously, a
|
||||
design change well beyond a signal-safety fix. C's own wiring here is vestigial:
|
||||
`tstp` is armed only by `md_tstpresume()`, which runs after a successful
|
||||
`restore()`, so a freshly started C game never had a SIGTSTP handler either.
|
||||
`term.Tcell.ShellEscape` (the `!` command) already covers getting to a shell and
|
||||
back, doing the same suspend/resume dance synchronously on the game goroutine
|
||||
where it is safe.
|
||||
|
||||
**SIGINT → `quit()`.** Under `md_onsignal_autosave` C routed SIGINT into the
|
||||
interactive "really quit?" prompt. The port exits instead (after restoring the
|
||||
terminal): the handler runs on another goroutine, and re-entering the message
|
||||
and input machinery from there would race every screen and state access the main
|
||||
goroutine makes. The `Q` command reaches the same prompt from inside the turn
|
||||
loop, which is where the player actually quits.
|
||||
|
||||
**Fault signals.** `md_onsignal_autosave` also sent SIGILL, SIGTRAP, SIGFPE,
|
||||
SIGBUS, SIGSEGV and SIGSYS to `auto_save`. In Go these are runtime panics with
|
||||
their own diagnostics, and gob-encoding the state that just faulted would risk
|
||||
replacing a good save file with a corrupt one, so they are left alone. SIGHUP
|
||||
and SIGTERM still autosave.
|
||||
|
||||
## 10. Testing strategy
|
||||
|
||||
|
||||
Reference in New Issue
Block a user