Skip to content

Conversation

arnetheduck
Copy link
Member

The signal handler installed currently is prone to crashing as it logs from within the signal handler which may end up corrupting the memory allocator.

Replace it with a process tracker:

  • shut down the application using chronos' waitSignal when available
  • avoid allocating memory in signal handler
  • track and print shutdown reason

The signal handler installed currently is prone to crashing as it logs
from within the signal handler which may end up corrupting the memory
allocator.

Replace it with a process tracker:

* shut down the application using chronos' waitSignal when available
* avoid allocating memory in signal handler
* track and print shutdown reason
@cheatfate
Copy link
Contributor

If you dont need to support SIGTERM on Windows, what the reason to support SIGTERM on Posix?

@arnetheduck
Copy link
Member Author

If you dont need to support SIGTERM on Window

https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/signal?view=msvc-170

The SIGILL and SIGTERM signals aren't generated under Windows

what the reason to support SIGTERM on Posix?

https://man7.org/linux/man-pages/man1/kill.1.html

If no signal is specified, the TERM signal is sent.

@cheatfate
Copy link
Contributor

cheatfate commented Aug 20, 2025

And you found one more reason why using signal is not a cross-platform solution.

The only way how signal catches SIGINT on Windows is this function SetConsoleCtrlHandler there is no other way how console application could catch CTRL+C/CTRL+BREAK press.

And if you check this function documentation you will find that console application is able to catch CTRL_CLOSE_EVENT, CTRL_LOGOFF_EVENT and CTRL_SHUTDOWN_EVENT, all of this events could be treated as SIGTERM.

If we will use your specific example of kill command - CTRL_CLOSE_EVENT is the nicest alternative solution. And its what chronos is doing for you.

https://github.com/status-im/nim-chronos/blob/master/chronos/internal/asyncengine.nim#L507-L525

Copy link

github-actions bot commented Aug 20, 2025

Unit Test Results

       15 files  ±  0    2 660 suites  ±0   1h 19m 49s ⏱️ - 9m 5s
  6 679 tests  - 17    6 122 ✔️  - 16  557 💤  -   1  0 ±0 
45 936 runs   - 50  45 164 ✔️  - 40  772 💤  - 10  0 ±0 

Results for commit 7d5f45c. ± Comparison against base commit 5ced3c3.

♻️ This comment has been updated with latest results.

@arnetheduck arnetheduck marked this pull request as draft August 23, 2025 09:21
arnetheduck added a commit that referenced this pull request Aug 26, 2025
#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.
@arnetheduck
Copy link
Member Author

Draft pending status-im/nim-chronos#581

arnetheduck added a commit that referenced this pull request Aug 27, 2025
#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.
arnetheduck added a commit that referenced this pull request Sep 24, 2025
Rebase of #7404 following
the adoption of #7418.

When using chronos to terminate the process, we run into various
threading and cross-platform issues captured in
status-im/nim-chronos#581, specially in
`nimbus` which has a more complex threading picture.
@arnetheduck arnetheduck deleted the process-state branch September 24, 2025 11:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants