Add Deno.kill(pid, signo) and process.kill(signo) (Unix only) #2177
Conversation
kevinkassimo
force-pushed
the
process/kill
branch
from
30463ee to
9661203
Compare
kevinkassimo
changed the title
| const inner = msg.Kill.createKill(builder, pid, signo); | ||
| dispatch.sendSync(builder, msg.Any.Kill, inner); | ||
| } | ||
|
|
||
| export class Process { |
There was a problem hiding this comment.
Unrelated to this commit, but I think we should rename this to Subprocess.
js/process.ts
Outdated
| export enum Signal { | ||
| SIGHUP = 1, | ||
| SIGINT, | ||
| SIGQUIT, |
There was a problem hiding this comment.
Although it's equivalent, I would prefer explicitly assigning values to each of these. It just makes things a bit more obvious if it ever needs to be debugged.
SIGINT = 2,
SIGQUIT = 3,
/* .. */There was a problem hiding this comment.
Actually I've just realized that certain signal numbers are platform dependent (like 10 is SIGBUS on macOS but SIGUSR1 on Linux). I might also need to check the current platform to decide which set of signals to export.
There was a problem hiding this comment.
Ah too bad. I thought maybe posix defined those - apparently not.
I think there's two options:
- Do something similar to Deno.platform where we generate the typescript during build.
- Pass the signals as strings ("SIGINT") and map them to their platform dependent integers outside of V8. This is what we did in Node IIRC.
There was a problem hiding this comment.
Currently resorting to
export const Signal = platform.os === "mac" ? MacOSSignal : LinuxSignal;Using string names also not quite ideal unless we build a similar mapping on the Rust side, since from_str in nix crate assumes mostly linux signal name mappings
kevinkassimo
force-pushed
the
process/kill
branch
from
758ac6a to
add5d7b
Compare
Implemented for Unix only first.
process.kill(pid[, signal])os.kill(pid, sig)int kill(pid_t pid, int sig)os.FindProcess+func (p *Process) Signal(sig Signal) errorAlso add a test to verify if process gets killed on
closeis working fine. If it works, then we might want to close #1494