rtomayko / posix-spawn

Ruby process spawning library
Other
519 stars 52 forks source link

EventMachine hooks #19

Closed pietern closed 9 years ago

pietern commented 12 years ago

I created code for an EM-aware POSIX::Spawn::Child. It passes the same tests as the blocking version, albeit with #callback and #errback instead of return/raise. Do you think it would be a good idea to smooth it out and import it into this repo, to make sure its interface/features/tests stay in sync with that of POSIX::Spawn::Child? I can send a pull request if so.

Thanks!

tmm1 commented 12 years ago

Sounds useful. We could add this as POSIX::Spawn::DeferrableChild, but I think it might be better off in a separate em-posix-spawn gem. How big is the implementation?

pietern commented 12 years ago

It's about 300 lines including some comments (less verbose than the original though). Spinning it off in a different gem would work, but has the maintainability problem of keeping the interfaces/functionality more or less in sync. Not sure if that is a big problem, though, since the EM implementation might more EM specific extra's to the interface down the line.

One thing I just came across is a child process that creates a child process itself, which daemonizes inheriting stdout and stderr. This leaves the those fds open, and thereby prevents the POSIX::Spawn::Child from returning to the caller. I'm taking care of that in the EM code now, using a custom signal handler. Not a related issue to the EM bindings per-se, but an example of something where a synchronized feature set would be preferable.

tmm1 commented 12 years ago

Hm, I'm not sure that belongs at the spawn layer. Usually daemonizing code will call Process.setsid and close all file descriptors to prevent problems like this.

pietern commented 12 years ago

I agree that this shouldn't belong at the spawn layer, but rather at the place where the child is reaped again. In general, daemonizing code should do setsid and/or close stdout/stderr, but sometimes it doesn't (which is why I came across this in the first place), and you're stuck on waitpid. The alternative is to shell out and redirect and/or explicitly close the descriptors, but that kind of defies the purpose of having a fast process spawner in the first place. In addition, I'd like to spawn a child knowing that it terminates or gets terminated within N seconds and not worry about edge cases.

Either way, do you think EM code should belong in this repo with/without the SIGCHLD handler?

tmm1 commented 12 years ago

I'm open to bundling EM support, but also don't want to allow this gem to get bloated.

Can you gist the implementation so we can get a sense of what's involved? If it's pretty simple and straightfoward, I'm :+1: just adding it to this repo.

/cc @rtomayko, thoughts?

tmm1 commented 12 years ago

I don't quite understand how you're using SIGCHLD to fix hanging on Process.wait, or to terminate children within a timeout.

pietern commented 12 years ago

Sorry for the delay, this is the gist: https://gist.github.com/1414341

Instead of waiting for stdout/stderr to close, this assumes the child is running until the parent receives SIGCHLD. When this happens, it does a waitpid which will immediately return with any pid of an exited child (since it just received SIGCHLD), it looks up the object in a pid registry, triggers the callbacks, etc. This handler now reaps any child, so it can't be used in combination with other code that uses waitpid. For this to work it needs to explicitly loop over the list of pids in its registry and call waitpid with WNOHANG to find if a child that registered itself with this handler exited, or it should be handled by other code. It works for me right now, but this may need to be smoothed out before going public.

pietern commented 12 years ago

I have adapted tests for the blocking POSIX::Spawn::Child to work with EM, which the class in the gist passes.

tmm1 commented 12 years ago

I think a separate gem is probably the best solution. The two implementations shouldn't be too hard to keep in sync, and a separate gem will allow for a dependency on eventmachine and more explicit support for async features.

The SIGCHLD stuff in particular is extremely useful with EM, but makes little sense with the the synchronous POSIX::Spawn::Child API.

This gem was designed to execute a single process at a time as fast as possible; in an evented environment there will always be multiple concurrent children, requiring a different feature set. I'd prefer these features were maintained and versioned separately.

Thanks for working on this, it will be quite useful to have all the features of posix-spawn available from EM servers.

dblock commented 11 years ago

There's https://github.com/paasio/em-posix-spawn, too.

rtomayko commented 9 years ago

Closing as stale and https://github.com/paasio/em-posix-spawn is a thing.