go-spacemesh icon indicating copy to clipboard operation
go-spacemesh copied to clipboard

Support WM_CLOSE for graceful shutdown on Windows

Open brusherru opened this issue 1 year ago • 2 comments

Description

Currently go-spacemesh handles correctly the case when you run go-spacemesh under the attached console (cmd.exe) and press CTRL+C. However, it does not work when you run it without an attached console, for example, as a child process. So if you send SIGINT from another application (that runs it detached or not in a console environment) — it will be ignored or forcefully killed.

Here is a topic from Node.js repo that is referencing this "common" (not only node.js) problem: https://github.com/nodejs/node/issues/35172

In our case, Smapp is killing go-spacemesh (when trying to send SIGINT or SIGTERM signals) without a chance to shut down properly. It also may cause db corruption.

Solution

Let's add support of TASKKILL without /F argument, aka WM_CLOSE.

Steps to reproduce

  1. Save the attached below JS code into some file
  2. Change the path to go-spacemesh binary
  3. Run it using node.js in cmd.exe, e.g. node script.js

In production, you can run Smapp on Windows and then click on Close and choose "QUIT" in a prompt. Open the logs and you won't see there any log messages related to graceful shutdown.

Environment

Please complete the following information:

  • OS: Windows (any)
  • Node Version: <=1.2.8
  • Smapp version (if applicable): any

Additional Resources

The script:

const { spawn, exec } = require('child_process');

const cmd = spawn('C:\\Users\\username\\AppData\\Local\\Programs\\Spacemesh\\node\\go-spacemesh.exe');

setTimeout(() => {
  exec(
    'TASKKILL /PID ' + cmd.pid,
    console.log
    // Error: Command failed: TASKKILL /PID 2928
    // ERROR: The process with PID 2928 could not be terminated.
    // Reason: This process can only be terminated forcefully (with /F option).
  );
}, 5000);

setTimeout(() => {
  console.log('SIGINT');
  cmd.kill('SIGINT'); // <-- killed, not gracefully
}, 10000);

cmd.stdout.on('data', (data) => {
  console.log(`STDOUT: ${data}`);
});
cmd.stderr.on('data', (data) => {
  console.log(`STDERR: ${data}`);
});
cmd.on('exit', (code) => {
  console.log(`Child exited with code ${code}`);
});

brusherru avatar Nov 30 '23 14:11 brusherru

I'm not 100% sure, but probably WM_CLOSE is for GUI applications only.

If so — then we need to pick some other solution:

  • starting some gRPC service ASAP, which implements the Shutdown endpoint
  • listening for /q command in stdin
  • something else

brusherru avatar Nov 30 '23 15:11 brusherru

I think the second option (\q) will be better choice for now, as we want to keep new API dependent on DB layer only.

kacpersaw avatar Dec 22 '23 12:12 kacpersaw