Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support WM_CLOSE for graceful shutdown on Windows #5321

Open
brusherru opened this issue Nov 30, 2023 · 2 comments · May be fixed by #5389
Open

Support WM_CLOSE for graceful shutdown on Windows #5321

brusherru opened this issue Nov 30, 2023 · 2 comments · May be fixed by #5389
Assignees

Comments

@brusherru
Copy link
Member

brusherru commented Nov 30, 2023

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: nodejs/node#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
Copy link
Member Author

brusherru commented Nov 30, 2023

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

@kacpersaw
Copy link
Contributor

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 kacpersaw linked a pull request Dec 22, 2023 that will close this issue
@kacpersaw kacpersaw linked a pull request Dec 22, 2023 that will close this issue
spacemesh-bors bot pushed a commit that referenced this issue Jan 17, 2024
## Motivation
Issue #5321

Listening for \q will allow smapp to gracefully shutdown the node.

Co-authored-by: Matthias <5011972+fasmat@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: 📋 Backlog
Development

Successfully merging a pull request may close this issue.

3 participants