Rework process shutdown, simplified (fixes #3235)

#7404 attempted to use
`waitSignal` to handle shutdown notifications but this turned out to be
unreliable, in particular due to multi-threading and cross-platform
differences.

This PR fixes the crash in signal handlers by removing logging but
otherwise retains the polling nature of the shutdown initiation - it
also allows nimbus eth1/2 to share the same cross-thread mechanism for
initiating a shutdown.

Author: arnetheduck

Makefile

@@ -299,7 +299,8 @@ XML_TEST_BINARIES := \
 # test suite
 TEST_BINARIES := \
 	block_sim \
-	test_libnimbus_lc
+	test_libnimbus_lc \
+	process_state
 .PHONY: $(TEST_BINARIES) $(XML_TEST_BINARIES) force_build_alone_all_tests
 
 # Preset-dependent tests
@@ -384,6 +385,14 @@ block_sim: | build deps
 			$(NIM_PARAMS) && \
 		echo -e $(BUILD_END_MSG) "build/$@"
 
+process_state: | build deps
+	+ echo -e $(BUILD_MSG) "build/$@" && \
+		MAKE="$(MAKE)" V="$(V)" $(ENV_SCRIPT) scripts/compile_nim_program.sh \
+			$@ \
+			"beacon_chain/$@.nim" \
+			$(NIM_PARAMS) && \
+		echo -e $(BUILD_END_MSG) "build/$@"
+
 DISABLE_TEST_FIXTURES_SCRIPT := 0
 # This parameter passing scheme is ugly, but short.
 test: | $(XML_TEST_BINARIES) $(TEST_BINARIES)

beacon_chain/beacon_node_status.nim

@@ -23,9 +23,9 @@ import
     engine_authentication, weak_subjectivity, peerdas_helpers],
   ./sync/[sync_protocol, light_client_protocol, sync_overseer],
   ./validators/[keystore_management, beacon_validators],
-  "."/[
+  ./[
     beacon_node, beacon_node_light_client, deposits,
-    nimbus_binary_common, statusbar, trusted_node_sync, wallets]
+    nimbus_binary_common, process_state, statusbar, trusted_node_sync, wallets]
 
 when defined(posix):
   import system/ansi_c
@@ -395,7 +395,7 @@ proc initFullNode(
 
   proc eventWaiter(): Future[void] {.async: (raises: [CancelledError]).} =
     await node.shutdownEvent.wait()
-    bnStatus = BeaconNodeStatus.Stopping
+    ProcessState.scheduleStop("shutdownEvent")
 
   asyncSpawn eventWaiter()
 
@@ -741,14 +741,14 @@ proc init*(T: type BeaconNode,
     try:
       if config.numThreads < 0:
         fatal "The number of threads --num-threads cannot be negative."
-        quit 1
+        quit QuitFailure
       elif config.numThreads == 0:
         Taskpool.new(numThreads = min(countProcessors(), 16))
       else:
         Taskpool.new(numThreads = config.numThreads)
     except CatchableError as e:
       fatal "Cannot start taskpool", err = e.msg
-      quit 1
+      quit QuitFailure
 
   info "Threadpool started", numThreads = taskpool.numThreads
 
@@ -1931,7 +1931,7 @@ proc onSecond(node: BeaconNode, time: Moment) =
   if node.config.stopAtSyncedEpoch != 0 and
       node.dag.head.slot.epoch >= node.config.stopAtSyncedEpoch:
     notice "Shutting down after having reached the target synced epoch"
-    bnStatus = BeaconNodeStatus.Stopping
+    ProcessState.scheduleStop("stopAtSyncedEpoch")
 
 proc runOnSecondLoop(node: BeaconNode) {.async.} =
   const
@@ -2161,8 +2161,6 @@ proc installMessageValidators(node: BeaconNode) =
   node.installLightClientMessageValidators()
 
 proc stop(node: BeaconNode) =
-  bnStatus = BeaconNodeStatus.Stopping
-  notice "Graceful shutdown"
   if not node.config.inProcessValidators:
     try:
       node.vcProcess.close()
@@ -2181,7 +2179,7 @@ proc stop(node: BeaconNode) =
   notice "Databases closed"
 
 proc run(node: BeaconNode) {.raises: [CatchableError].} =
-  bnStatus = BeaconNodeStatus.Running
+  ProcessState.notifyRunning()
 
   if not isNil(node.restServer):
     node.restServer.installRestHandlers(node)
@@ -2219,9 +2217,13 @@ proc run(node: BeaconNode) {.raises: [CatchableError].} =
   asyncSpawn runQueueProcessingLoop(node.blockProcessor)
   asyncSpawn runKeystoreCachePruningLoop(node.keystoreCache)
 
-  # main event loop
-  while bnStatus == BeaconNodeStatus.Running:
-    poll() # if poll fails, the network is broken
+  block mainLoop:
+    while true:
+      ProcessState.stopping.isErrOr:
+        notice "Shutting down", reason = value
+        break mainLoop
+
+      poll()
 
   # time to say goodbye
   node.stop()
@@ -2439,8 +2441,6 @@ proc doRunBeaconNode(config: var BeaconNodeConf, rng: ref HmacDrbgContext) {.rai
   ignoreDeprecatedOption web3ForcePolling
   ignoreDeprecatedOption finalizedDepositTreeSnapshot
 
-  createPidFile(config.dataDir.string / "beacon_node.pid")
-
   config.createDumpDirs()
 
   # There are no managed event loops in here, to do a graceful shutdown, but
@@ -2453,27 +2453,6 @@ proc doRunBeaconNode(config: var BeaconNodeConf, rng: ref HmacDrbgContext) {.rai
   for node in metadata.bootstrapNodes:
     config.bootstrapNodes.add node
 
-  ## Ctrl+C handling
-  proc controlCHandler() {.noconv.} =
-    when defined(windows):
-      # workaround for https://github.com/nim-lang/Nim/issues/4057
-      try:
-        setupForeignThreadGc()
-      except Exception as exc: raiseAssert exc.msg # shouldn't happen
-    notice "Shutting down after having received SIGINT"
-    bnStatus = BeaconNodeStatus.Stopping
-  try:
-    setControlCHook(controlCHandler)
-  except Exception as exc: # TODO Exception
-    warn "Cannot set ctrl-c handler", msg = exc.msg
-
-  # equivalent SIGTERM handler
-  when defined(posix):
-    proc SIGTERMHandler(signal: cint) {.noconv.} =
-      notice "Shutting down after having received SIGTERM"
-      bnStatus = BeaconNodeStatus.Stopping
-    c_signal(ansi_c.SIGTERM, SIGTERMHandler)
-
   block:
     let res =
       if config.trustedSetupFile.isNone:
@@ -2483,6 +2462,10 @@ proc doRunBeaconNode(config: var BeaconNodeConf, rng: ref HmacDrbgContext) {.rai
     if res.isErr():
       raiseAssert res.error()
 
+  ProcessState.stopping.isErrOr:
+    notice "Shutting down", reason = value
+    return
+
   let node = waitFor BeaconNode.init(rng, config, metadata)
 
   let metricsServer = (waitFor config.initMetricsServer()).valueOr:
@@ -2494,7 +2477,8 @@ proc doRunBeaconNode(config: var BeaconNodeConf, rng: ref HmacDrbgContext) {.rai
 
   node.metricsServer = metricsServer
 
-  if bnStatus == BeaconNodeStatus.Stopping:
+  ProcessState.stopping.isErrOr:
+    notice "Shutting down", reason = value
     return
 
   when not defined(windows):
@@ -2601,7 +2585,11 @@ proc handleStartUpCmd(config: var BeaconNodeConf) {.raises: [CatchableError].} =
   let rng = HmacDrbgContext.new()
 
   case config.cmd
-  of BNStartUpCmd.noCommand: doRunBeaconNode(config, rng)
+  of BNStartUpCmd.noCommand:
+    createPidFile(config.dataDir.string / "beacon_node.pid")
+    ProcessState.setupStopHandlers()
+
+    doRunBeaconNode(config, rng)
   of BNStartUpCmd.deposits: doDeposits(config, rng[])
   of BNStartUpCmd.wallets: doWallets(config, rng[])
   of BNStartUpCmd.record: doRecord(config, rng[])
@@ -2667,7 +2655,7 @@ proc main() {.noinline, raises: [CatchableError].} =
   when defined(windows):
     if config.runAsService:
       proc exitService() =
-        bnStatus = BeaconNodeStatus.Stopping
+        ProcessState.scheduleStop("exitService")
       establishWindowsService(clientId, copyrights, nimBanner, SPEC_VERSION,
                               "nimbus_beacon_node", BeaconNodeConf,
                               handleStartUpCmd, exitService)

beacon_chain/nimbus_beacon_node.nim

@@ -21,7 +21,7 @@ import
   # Local modules
   ./spec/[helpers, keystore],
   ./spec/datatypes/base,
-  ./[beacon_clock, beacon_node_status, conf, version]
+  ./[beacon_clock, conf, process_state, version]
 
 when defaultChroniclesStream.outputs.type.arity == 2:
   from std/os import commandLineParams, getEnv, splitFile
@@ -41,7 +41,7 @@ declareGauge nimVersionGauge, "Nim version info", ["version", "nim_commit"], nam
 nimVersionGauge.set(1, labelValues=[NimVersion, getNimGitHash()])
 
 export
-  confutils, toml_serialization, beacon_clock, beacon_node_status, conf
+  confutils, toml_serialization, beacon_clock, conf
 
 
 type
@@ -296,7 +296,7 @@ proc runSlotLoop*[T](node: T, startTime: BeaconTime,
         fatal "System time adjusted backwards significantly - clock may be inaccurate - shutting down",
           nextSlot = shortLog(nextSlot),
           wallSlot = shortLog(wallSlot)
-        bnStatus = BeaconNodeStatus.Stopping
+        ProcessState.scheduleStop("clock skew")
         return
 
       # Time moved back by a single slot - this could be a minor adjustment,