-
Notifications
You must be signed in to change notification settings - Fork 385
feat(process): implement process.on for signal handling #1293
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
base: main
Are you sure you want to change the base?
Conversation
1e97328 to
f81f2ad
Compare
richarddavison
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR and sorry for the delay of review. I like this feature but there are a couple of issues.
- This doesn't properly use the event emitter mechanisms that we use in other modules, it just overwrites them. Look how event emitter is used elsewhere.
- Do we need to hardcode the signal strings in
modules/llrt_process/src/signal_handler.rsare they in libc crate or in std or somewhere else? If not, we should altest use statics, or maybe we can work with SignalKind directly? - The signal handler spawns a task per signal which is fine, however this will not work if there are multiple runtimes. We need to consider this so the atomic bools is tied to the process instances
|
Thanks for the thorough review, @richarddavison. You raised valid concerns and I appreciate the patience. Addressing the feedback (in 2nd commit):
You are right that I am overriding the event emitter methods rather than properly integrating. I looked at how streams do it - they implement on_event_changed() which sends on a channel, and a task spawned during the stream's operation lifecycle receives and acts. For Process, I couldn't find a clean way to replicate this because:
I am open to suggestions here - if you see a cleaner way to integrate with on_event_changed() I'd be happy to refactor.
Good catch. I refactored to use a single SUPPORTED_SIGNALS static array that pairs signal names with their libc constants: Now
This was a significant oversight. The global AtomicBool statics meant all runtimes shared state, which is wrong. I replaced them with a per-instance HashSet field on Process, so each runtime correctly tracks its own signal handlers. Additional fixes:
Let me know if there is anything else to address. |
e5f350a to
d51a106
Compare
Replace res_bytes.as_slice() with idiomatic &res_bytes[..] to avoid name collision with new [T]::as_slice() method added in Rust nightly (rust-lang/rust#145933, merged 2025-12-18). Issue observed in nightly build checks for PR awslabs#1293. The WriteBuf trait from zstd was only imported to provide as_slice() through its impl for [u8], which is unnecessary coupling. Using slice syntax is more idiomatic and doesn't require external trait imports.
…1295) Replace res_bytes.as_slice() with idiomatic &res_bytes[..] to avoid name collision with new [T]::as_slice() method added in Rust nightly (rust-lang/rust#145933, merged 2025-12-18). Issue observed in nightly build checks for PR #1293. The WriteBuf trait from zstd was only imported to provide as_slice() through its impl for [u8], which is unnecessary coupling. Using slice syntax is more idiomatic and doesn't require external trait imports.
|
Fantastic! Thanks for the quick updates.
Then maybe the best way here is to modify on_event changed to pass a &ctx reference as well. Would clean this up I think. I was also thinking that listening for "each" signal in a separate task could lead to races and is ineffective. However this is kind of complex as in linux/tokio+signals you kind of "hook-in" to signal managed or you don't. It's not designed for on/off use cases. In other words if you remove signal handling for SIGINT, ctrl+c would not work anymore. If we can find a good way to fallback to platform defaults when disabling a single AND use a single thread with a FuturesUnordered.next() for all signals this would be better. I think we have to use something like |
d51a106 to
dbe9231
Compare
Convert process from plain Object to Class with EventEmitter support,
enabling signal handling via process.on('SIGTERM', callback) etc.
Supported signals (Unix only):
- SIGTERM, SIGINT, SIGHUP, SIGQUIT, SIGUSR1, SIGUSR2
Signal handlers are lazily initialized when listeners are first
registered, avoiding unnecessary background tasks.
Closes awslabs#153
Move Unix-specific methods (getuid, getgid, geteuid, getegid, setuid, setgid, seteuid, setegid) out of the #[rquickjs::methods] impl block and add them directly to the process object in init(). The #[cfg(unix)] attribute on individual methods inside #[rquickjs::methods] doesn't work correctly with the proc macro - it generates code referencing those methods even when they're conditionally compiled out, causing build failures on Windows.
- Refactor signal handler to use per-instance HashSet tracking instead of global AtomicBool statics, supporting multiple runtimes correctly - Use SUPPORTED_SIGNALS static array with libc constants instead of hardcoded signal strings and numbers - Add signal handler check to all listener methods (addListener, prependListener, prependOnceListener) for consistency - Add SIGUSR1/SIGUSR2 support to signals.rs and types - Add tests for listener method parity and event emitter functionality - Document on_event_changed() constraint in module docs
c02a67a to
850c7db
Compare
Addresses review feedback on signal handling architecture: 1. Extends Emitter trait's on_event_changed to receive &Ctx and Class<Self>, enabling signal handler setup without manual hooks in listener methods. 2. Replaces per-signal spawned tasks with a single coordinator task that multiplexes all signals via tokio::select!, eliminating race conditions. 3. Implements lazy signal registration - signals are only registered with the OS when the first listener is added for that specific signal. This ensures we don't interfere with default OS behavior for unrequested signals. 4. Tracks listener counts per signal via MPSC channel commands, enabling proper handling when listeners are added/removed. Note: Once a signal is registered, default OS behavior cannot be restored (tokio::signal limitation). Lazy registration mitigates this by only affecting signals the user explicitly requested.
850c7db to
8d0f3ad
Compare
|
@richarddavison Tried to address your feedback in latest commit:
Options not pursued:
Please let me know if this is a path forward for this PR or if I need to further research/investigate alternatives. |
Thanks for this! However, I'm still a bit concerned that deregistering a signal is problematic. For example, say I register a SIGINT and then deregister it. On ctrl+c we no longer quit the program. My best bet here is to look at node.js (or libuvs) internal signal handling to see how they do it. Maybe tokio abstractions is not an option here and we have to rely on libc crate. |
Issue # (if available)
Closes #153
Description of changes
Convert process from plain Object to Class with EventEmitter support, enabling signal handling via process.on('SIGTERM', callback) etc.
Supported signals (Unix only):
Signal handlers are lazily initialized when listeners are first registered, avoiding unnecessary background tasks.
Checklist
tests/unitand/or in Rust for my feature if neededmake fixto format JS and apply Clippy auto fixesmake checktypes/directoryBy submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.