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

Listen for \q to gracefully shutdown the node

Open kacpersaw opened this issue 1 year ago • 6 comments

Motivation

Issue https://github.com/spacemeshos/go-spacemesh/issues/5321

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

kacpersaw avatar Dec 22 '23 14:12 kacpersaw

Codecov Report

Attention: 18 lines in your changes are missing coverage. Please review.

Comparison is base (3675992) 0.0% compared to head (234a33b) 77.6%. Report is 42 commits behind head on develop.

Files Patch % Lines
node/node.go 0.0% 18 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##           develop   #5389      +/-   ##
==========================================
+ Coverage         0   77.6%   +77.6%     
==========================================
  Files            0     266     +266     
  Lines            0   30906   +30906     
==========================================
+ Hits             0   24003   +24003     
- Misses           0    5389    +5389     
- Partials         0    1514    +1514     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Dec 22 '23 14:12 codecov[bot]

  • Works as expected on Windows.
  • And works the same on MacOS except for one thing not related to these changes.
  • I didn't check it on Linux, but I believe it should work as well :)

Inconsistent behavior when Node is compacting the database and receives \q:

  • On MacOS: it does not produce any log messages anymore (like "app cleanup starting..."), and just closes after a while. It works the same on previous versions of go-spacemesh using SIGTERM signal.
  • On Windows: it produces log messages and closes after a while

Except for this inconsistent behavior — LGTM. So I'd be glad to have these changes if nobody is against the approach with stdin :)

brusherru avatar Jan 04 '24 07:01 brusherru

I don't think listening to stdin is necessary to make a graceful shutdown on windows work. Instead we should try adding syscall.SIGINT to the argument list of signal.NotifyContext in node.go this should capture multiple signals on windows that are used to trigger an application to exit.

See also this change for it: https://go-review.googlesource.com/c/go/+/187739 it was added in Go 1.14. Relevant part in the release notes: https://go.dev/doc/go1.14#ossignalpkgossignal

EDIT: my bad - the problem is not the receiving program in windows its the sender. A assume GeneralConsoleCtrlEvent cannot be used? https://learn.microsoft.com/en-us/windows/console/generateconsolectrlevent?redirectedfrom=MSDN

fasmat avatar Jan 11 '24 12:01 fasmat

bors merge

fasmat avatar Jan 17 '24 18:01 fasmat

Build failed:

spacemesh-bors[bot] avatar Jan 17 '24 19:01 spacemesh-bors[bot]

The merge failed because reading from os.Stdin immediately returns io.EOF when the node runs inside docker - shutting down the app shortly after starting.

fasmat avatar Jan 18 '24 00:01 fasmat

I feel like this can be closed, reading from os.Stdin won't work when the node is run inside a container (there is no stdin in that environment) and using a hook for syscall.SIGINT won't work on windows.

I believe a proper solution would be to add a handler to the private grpc endpoint that can be called to shut down the node gracefully. It would then call stop/cancel on the top level context, that the grpc service needs to be passed in which will gracefully shut down the node on all platforms as expected.

I will close this PR for now, but we should re-visit fixing #5321 at some point in the future.

fasmat avatar Jun 27 '24 13:06 fasmat