-
Notifications
You must be signed in to change notification settings - Fork 216
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
proper signal handling #36
Comments
I did some investigation of this today. We don't currently propagate signals, however, what we do do is forward the STDIN to the child process. So if the user kills the process using a keyboard shortcut (like However, if we send the That said, I don't know if passing signals along is actually what we want to do, as the child process is a separate process, with a separate PID, from the parent. So it seems reasonable that a user would expect the processes to be mostly independent as far as direct signals are concerned. Lastly, while the main shim child processes do forward along the STDIN, I'm not sure that all of our child processes do. So we should, at the very least, do some auditing to make sure that all of our child processes use |
Any chance of taking a second look at this issue? I just ran into this in a CI script running a node server for testing. As a simple repro:
const server = require('http').createServer((_, response) => {
response.end('hello there\n');
});
server.listen(10200);
#!/bin/sh
node test-server.js &
sleep 5
curl http://localhost:10200
sleep 5
kill $!
curl http://localhost:10200 When using volta, the second #36 (comment) brings up good points about expectations (and |
@brendankenny Thanks for raising this up again! I think that the scripted use-case is an important point, since it's a spot where you can't (reasonably) send a Ctrl+C to the process to kill it, you instead need to be able to pass a signal. |
I'm working on an Electron App, which spawns child node processes (using the host machines' installed nodejs), and I need to terminate these processes with signals. When using Volta, |
I would argue the opposite here. Volta shims should, arguably, be as transparent to the end user as possible. A user of Volta shouldn't need to work with the node process any differently than if they'd installed it natively. That in my mind was one of the nice selling points of Volta as a tool from the get-go in terms of how it holds node/npm/global versions of all your dependencies and seamlessly switches between them. Fudging with child processes to properly kill running processes seems to be the antithesis of how Volta sells itself. I realize actually implementing this might not be trivial. But it seems like an important piece to get right. |
@vanstinator That's a good point and well stated, thanks! There's a lot of platform-specific nuance with signal handling (since Windows doesn't have signals in the same way as MacOS / Linux), but I agree that getting it right will make Volta more appropriately transparent, especially for these "launch as a sub-process" use-cases. |
Any progress on this? I'm developing a process manager that works with Volta, and I want to shutdown a given node process. |
Tool::exec
doesn't propagate signals yet.The text was updated successfully, but these errors were encountered: