Skip to content
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

Setup and kill process group instead of just immediate child #41

Open
passcod opened this issue Mar 31, 2017 · 6 comments
Open

Setup and kill process group instead of just immediate child #41

passcod opened this issue Mar 31, 2017 · 6 comments

Comments

@passcod
Copy link

passcod commented Mar 31, 2017

See how watchexec does it: https://github.com/mattgreen/watchexec/blob/master/src/process.rs

The stdlib Process sends a kill syscall to the process: https://github.com/rust-lang/rust/blob/master/src/libstd/sys/unix/process/process_unix.rs#L246

However, that doesn't kill all processes, especially with servers, see watchexec/cargo-watch#25.

I really don't want to implement process handling myself. That's what Duct does. So it would be really neat if Duct could handle it :)

@oconnor663
Copy link
Owner

oconnor663 commented Mar 31, 2017

I thought about using process groups when I was figuring out how to implement kill safely in https://github.com/oconnor663/shared_child.rs. The major downside with process groups though, is that if you have children that aren't in the same group as the parent, then Ctrl-C in the terminal won't kill them when it kills the parent. The solution there is to have the parent handle SIGINT and kill the children group itself before exiting, and it looks like watchexec does something like that. But duct is just a library, and it doesn't own the process-wide signal handlers, so we can't do that. I don't know of any other solutions.

Using process groups to guarantee cleanup might run into a related issue, where if any of the grandchild processes change their group, they won't receive signals anymore. That might happen if watchexec executes another instance of watchexec, for example, maybe via some script that runs watchexec under the covers. I think really solving that would require using some platform-specific features like cgroups. Or Docker, which is based on cgroups. And speaking of Docker, even cgroup-based cleanup can be incomplete if there's any service running on the machine that will spawn child processes in response to commands, like dockerd. At that point the OS has no idea who's really responsible for those processes, and all bets are off :p

I worry that if duct did expose process groups, the documentation for how it all works would be full of warnings and limitations, since different use cases have such different requirements (trusted children vs untrusted, interactive commands vs services, Rust programs vs libraries linked from other environments). My assumption has mostly been that, if you're a program whose primary job is to manage child processes, you'll want to get your hands on all the low level functions yourself rather than letting duct hide them from you. But I could be convinced otherwise, if there were a few different callers that had mostly compatible requirements?

@passcod
Copy link
Author

passcod commented Mar 31, 2017

Hmm, maybe just having a way to expose the process ID of the running expression, so Duct-users can still get its abstractions but also call OS functions directly?

I'm actually fine with figuring out low-level for specialised usecases, but it seems so… wasteful, at a personal and at a community level, to do all of it over and over and over again, when 80% of what I need is covered by Duct.

@oconnor663
Copy link
Owner

One thing I've been thinking about is a constructor that lets you build an Expression from a std::process::Command object that you've prepared yourself. That would let you get at the Unix-specific CommandExt trait functions as needed, without requiring duct to have much of an opinion about what's exposed and what isn't. Would that work for you?

Exposing the SharedChild objects from inside a Handle could be possible too. Tell me more about how you might want to use them?

@passcod
Copy link
Author

passcod commented Apr 1, 2017

That… could work, yes. Especially combined with an externalised sh from the other issue, so I could build commands whether they be straight calls or shell calls, attach the control I want, and then pass them into Duct.

@passcod
Copy link
Author

passcod commented Apr 1, 2017

Having the PID of a command would mean, I believe, I could create a process group, and then assign commands to the group. With setpgid, I can assign a pid to a pgid. On Windows, I can use AssignProcessToJobObject to do the same. I'd need to create the process groups / job objects myself prior to running the commands, but that's probably fine.

@oconnor663
Copy link
Owner

Thinking out loud: Giving you the PIDs is going to be tricky, because of then. The right side of a then expression doesn't execute until the left side is finished. A method like Handle::get_all_pids() wouldn't be able to give you a complete list. And if you did any kind of out-of-band signaling that duct didn't know about, it could produce weird results. (Duct knows that if you kill a running expression, the right side of a then should not be started after that, even if the left side is unchecked or happened to have just exited. But if you did a group kill, the right side of then could still execute.) Can we solve these problems?

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

No branches or pull requests

2 participants