Process trees left running upon exiting multiview about multiview HOT 7 CLOSED

It's my pleasure, and major seems like the safer choice here.

from multiview.

@efokschaner Thank you for bringing attention to this and for submitting a PR. Do you know if the problem exists on other platforms? Based on the link you shared, it seems like this is windows specific?

from multiview.

I haven't tested multiview on other platforms, however in my experience Unix , via sighup, generally offers processes the tools to do "what's right for them" (see https://stackoverflow.com/a/284443/740958) whereas on windows, parents must generally intentionally kill or provide custom signalling to their children, while CMD (which multiview always uses) does neither to stop children when it itself is killed. Therefore I believe it is reasonable to make this generalisation on windows that multiview shuts down the process tree it started.

from multiview.

@efokschaner I haven't forgotten about this. I'd like to think this through before accepting/merging though. I'm mostly concerned that this may present still inconsistent behavior on various platforms in general.

I wonder if using a module like tree-kill would be an effective way to make the behavior predictable and consistent across platforms.

I'd also be concerned that terminating children ungracefully could cause issues with some programs, not getting a chance to properly shut down.

from multiview.

Take your time and let me know if you have more thoughts. I've found process tree management on windows/POSIX to be so different that i don't have the expectation of tools having completely identical behaviour across platforms. tree-kill on other platforms might be limiting the options of program authors unnecessarily. If you can find something less aggressive on windows that still tends to support cleanup of process trees by default that would be fine, I think we'd still see scenarios where the behaviour is not identical across platforms because the building blocks / abstractions just aren't the same.

from multiview.

This issue seems to affect macos as well. I get process trees left running when I ctrl-c multiview on mac.

Taking a step back from the issues. What I'm looking for is for doing ctrl-c in multiview to be as similar as possible to if I ran the commands separately in different shells, and ctrl-c'ed all those shells.

I tried to learn more about what ctrl-c on each OS actually does:
POSIX: https://en.wikipedia.org/wiki/POSIX_terminal_interface

Signals generated at the terminal are sent to all processes that are members of the terminal's foreground process group

So on posix ctrl-c to a shell sends SIGINT to all the foreground processes in that shell.

Windows: https://docs.microsoft.com/en-us/windows/console/ctrl-c-and-ctrl-break-signals

The system creates a new thread in each client process to handle the event.

Windows appears to start a remote thread in each process that's attached to the shell to invoke a handler.

So essentially the terminal mechanisms on both platforms are sending a sigint / CTRL_C_EVENT to all the processes of the process tree that are foreground / attached to the shell.
Because multiview decouples the spawned process trees from the terminal because it needs pipes for the processes stdio, it would need to somehow substitute this behaviour.

Sending a real CTRL_C_EVENT on windows using nodejs seems like quite a hassle, especially without something like python's builtin ctypes module.

So perhaps perfect emulation of forwarding all kinds of signals like a shell is a bit ambitious, and we should focus on the higher level behaviour I'm after and simply go for "kill multiview" == "kill all children". For this "kill all children" to happen, multiview has to signal / kill the whole tree as that's what the shells were doing without multiview.

Ok, that was a long walk back to what you suggested but I wanted to capture my reasoning.

I'll give tree-kill a try and if it goes well i'll open a new PR.

from multiview.

@efokschaner this is great work. Really really appreciate the investigation.

Your PR looks good as well, but I have a few questions. Perhaps I'm not understanding a few things.

Either way, I think this is an important change/fix. I'm struggling whether to consider this a breaking change (eg. major version bump) or a bug fix (patch version bump). Since it may have unintended consequences for some users, I'd be inclined to make this a major version bump.

from multiview.