From 42c6eec4ef7752c4f48ace2899a44840df95da9c Mon Sep 17 00:00:00 2001 From: Timothee Cour Date: Sun, 18 Apr 2021 06:34:29 -0700 Subject: fix #17749 ignore SIGPIPE signals, fix nim CI #17748 (#17752) * fix #17749 SIGPIPE * fix for windows --- changelog.md | 20 ++++++++++++-------- koch.nim | 4 ++++ lib/system/excpt.nim | 16 +++++++++++++--- tests/stdlib/tosproc.nim | 23 +++++++++++++++++++++-- tests/system/tsigexitcode.nim | 11 +++++++---- 5 files changed, 57 insertions(+), 17 deletions(-) diff --git a/changelog.md b/changelog.md index 5efde5659..8fab425f8 100644 --- a/changelog.md +++ b/changelog.md @@ -39,6 +39,18 @@ - In `std/os`, `getHomeDir`, `expandTilde`, `getTempDir`, `getConfigDir` now do not include trailing `DirSep`, unless `-d:nimLegacyHomeDir` is specified (for a transition period). +- On POSIX systems, the default signal handlers used for Nim programs (it's + used for printing the stacktrace on fatal signals) will now re-raise the + signal for the OS default handlers to handle. + + This lets the OS perform its default actions, which might include core + dumping (on select signals) and notifying the parent process about the cause + of termination. + +- On POSIX systems, we now ignore `SIGPIPE` signals, use `-d:nimLegacySigpipeHandler` + for previous behavior. + + ## Standard library additions and changes - Added support for parenthesized expressions in `strformat` @@ -219,14 +231,6 @@ `ValueError` when the real command line is not available. `parseopt` was previously excluded from `prelude` for JS, as it could not be imported. -- On POSIX systems, the default signal handlers used for Nim programs (it's - used for printing the stacktrace on fatal signals) will now re-raise the - signal for the OS default handlers to handle. - - This lets the OS perform its default actions, which might include core - dumping (on select signals) and notifying the parent process about the cause - of termination. - - Added `system.prepareStrMutation` for better support of low level `moveMem`, `copyMem` operations for Orc's copy-on-write string implementation. diff --git a/koch.nim b/koch.nim index 3e41310d7..49540eb6a 100644 --- a/koch.nim +++ b/koch.nim @@ -542,6 +542,10 @@ proc runCI(cmd: string) = # boot without -d:nimHasLibFFI to make sure this still works kochExecFold("Boot in release mode", "boot -d:release -d:nimStrictMode") + when false: # debugging: when you need to run only 1 test in CI, use something like this: + execFold("debugging test", "nim r tests/stdlib/tosproc.nim") + doAssert false, "debugging only" + ## build nimble early on to enable remainder to depend on it if needed kochExecFold("Build Nimble", "nimble") diff --git a/lib/system/excpt.nim b/lib/system/excpt.nim index 06fa45097..da034e57e 100644 --- a/lib/system/excpt.nim +++ b/lib/system/excpt.nim @@ -602,6 +602,9 @@ when defined(cpp) and appType != "lib" and not gotoBasedExceptions and quit 1 when not defined(noSignalHandler) and not defined(useNimRtl): + type Sighandler = proc (a: cint) {.noconv, benign.} + # xxx factor with ansi_c.CSighandlerT, posix.Sighandler + proc signalHandler(sign: cint) {.exportc: "signalHandler", noconv.} = template processSignal(s, action: untyped) {.dirty.} = if s == SIGINT: action("SIGINT: Interrupted by Ctrl-C.\n") @@ -652,7 +655,11 @@ when not defined(noSignalHandler) and not defined(useNimRtl): else: quit(1) + var SIG_IGN {.importc: "SIG_IGN", header: "".}: Sighandler + proc registerSignalHandler() = + # xxx `signal` is deprecated and has many caveats, we should use `sigaction` instead, e.g. + # https://stackoverflow.com/questions/231912/what-is-the-difference-between-sigaction-and-signal c_signal(SIGINT, signalHandler) c_signal(SIGSEGV, signalHandler) c_signal(SIGABRT, signalHandler) @@ -661,14 +668,17 @@ when not defined(noSignalHandler) and not defined(useNimRtl): when declared(SIGBUS): c_signal(SIGBUS, signalHandler) when declared(SIGPIPE): - c_signal(SIGPIPE, signalHandler) + when defined(nimLegacySigpipeHandler): + c_signal(SIGPIPE, signalHandler) + else: + c_signal(SIGPIPE, SIG_IGN) registerSignalHandler() # call it in initialization section proc setControlCHook(hook: proc () {.noconv.}) = # ugly cast, but should work on all architectures: - type SignalHandler = proc (sign: cint) {.noconv, benign.} - c_signal(SIGINT, cast[SignalHandler](hook)) + when declared(Sighandler): + c_signal(SIGINT, cast[Sighandler](hook)) when not defined(noSignalHandler) and not defined(useNimRtl): proc unsetControlCHook() = diff --git a/tests/stdlib/tosproc.nim b/tests/stdlib/tosproc.nim index 96cff1468..67731322a 100644 --- a/tests/stdlib/tosproc.nim +++ b/tests/stdlib/tosproc.nim @@ -206,8 +206,8 @@ else: # main driver var line = newStringOfCap(120) while true: if outp.readLine(line): - result[0].string.add(line.string) - result[0].string.add("\n") + result[0].add(line) + result[0].add("\n") else: result[1] = peekExitCode(p) if result[1] != -1: break @@ -279,3 +279,22 @@ else: # main driver when defined(posix): doAssert execCmdEx("echo $FO", env = newStringTable({"FO": "B"})) == ("B\n", 0) doAssert execCmdEx("echo $PWD", workingDir = "/") == ("/\n", 0) + + block: # bug #17749 + let output = compileNimProg("-d:case_testfile4", "D20210417T011153") + var p = startProcess(output, dir) + let inp = p.inputStream + var count = 0 + when defined(windows): + # xxx we should make osproc.hsWriteData raise IOError on windows, consistent + # with posix; we could also (in addition) make IOError a subclass of OSError. + type SIGPIPEError = OSError + else: + type SIGPIPEError = IOError + doAssertRaises(SIGPIPEError): + for i in 0..<100000: + count.inc + inp.writeLine "ok" # was giving SIGPIPE and crashing + doAssert count >= 100 + doAssert waitForExit(p) == QuitFailure + close(p) # xxx isn't that missing in other places? diff --git a/tests/system/tsigexitcode.nim b/tests/system/tsigexitcode.nim index 6922cb8eb..249256b40 100644 --- a/tests/system/tsigexitcode.nim +++ b/tests/system/tsigexitcode.nim @@ -11,10 +11,13 @@ proc main() = discard posix.raise(signal) else: # synchronize this list with lib/system/except.nim:registerSignalHandler() - let fatalSigs = [SIGINT, SIGSEGV, SIGABRT, SIGFPE, SIGILL, SIGBUS, - SIGPIPE] - for s in fatalSigs: + let sigs = [SIGINT, SIGSEGV, SIGABRT, SIGFPE, SIGILL, SIGBUS, SIGPIPE] + for s in sigs: let (_, exitCode) = execCmdEx(quoteShellCommand [getAppFilename(), $s]) - doAssert exitCode == 128 + s, "mismatched exit code for signal " & $s + if s == SIGPIPE: + # SIGPIPE should be ignored + doAssert exitCode == 0, $(exitCode, s) + else: + doAssert exitCode == 128+s, $(exitCode, s) main() -- cgit 1.4.1-2-gfad0