childprocess icon indicating copy to clipboard operation
childprocess copied to clipboard

A backend based on Process.spawn

Open eregon opened this issue 3 years ago • 8 comments

Process.spawn is the official Ruby core API for spawning subprocesses, and I think it has all features needed to implement ChildProcess on top of it.

I think it is the best way to implement ChildProcess, as it is an official Ruby core API supported by all Ruby implementations, and other backends could actually be removed as they would be redundant.

Process.spawn exists for longer than I can remember and certainly since Ruby 2.4, so I think it should work on all supported Ruby versions (https://github.com/enkessler/childprocess#requirements)

From https://github.com/oracle/truffleruby/issues/1525#issuecomment-445780033 From that note, posix_spawn is unsafe because it might Dir.chdir() concurrently to some other thread executing some code relying on the cwd. And fork is of course not portable. So Process.spawn seems better than both backends.

eregon avatar Dec 04 '20 14:12 eregon

Hey @eregon—supportive of the request, though would prefer this be contributed by folks whose use case necessitates it so they can test it in a real-world scenario.

I don't have context on why the original ChildProcess backends were chosen over Process.spawn, but I assume there was some reason. Would be happy to review a tested pull request implementing this functionality.

sds avatar Dec 10 '20 17:12 sds

I don't have context on why the original ChildProcess backends were chosen over Process.spawn, but I assume there was some reason.

I don't know for sure, but I would bet it's because ChildProcess already existed in the Ruby 1.8 area, and Process.spawn didn't exist then (it was added in Ruby 1.9, just checked). There were still patch releases of 1.8 in 2010 when the project was started: https://github.com/enkessler/childprocess/tags?after=v0.1.1, so it seems a likely reason.

eregon avatar Dec 10 '20 18:12 eregon

I'll try to find some time to make a prototype but if anyone else is interested, go ahead and just mention it.

eregon avatar Dec 10 '20 18:12 eregon

Here is a prototype, and it already passes all the specs, at least on Linux: https://github.com/enkessler/childprocess/compare/master...eregon:process-spawn

I would think it works fine on Windows too, however the superclass is Unix::Process so there might be some assumptions there.

eregon avatar Dec 10 '20 19:12 eregon

If Ruby itself has gotten better about cross-platform process handling, I'd be perfectly content for childprocess to become just a more convenient API on top of the core library instead of it having to handle a lot of the more 'fiddly' stuff that might have been needed back in the day.

enkessler avatar Jun 10 '21 01:06 enkessler

https://github.com/enkessler/childprocess/pull/175 already works well on UNIX, but not yet on Windows. I don't have a Windows dev machine, so that's rather impractical as I'd need to wait for CI to know if something works or not. Maybe I can try to force it to take the Windows branch locally for faster development.

Are constants like ChildProcess::Unix considered public API or not? I guess not given they are conditionally defined based on the current platform.

eregon avatar Jun 10 '21 09:06 eregon

Update: it works on Windows now, there are only 2 failures left: https://github.com/enkessler/childprocess/pull/175#issuecomment-864585821 And the diff is +166 −2,045.

eregon avatar Jun 20 '21 17:06 eregon

A couple thousand fewer lines of code to have to worry about is nice!

The second is the issue that there is no builtin way to kill a process group on Windows.

Would it be possible to incrementally replace the internal implementation? Swap the OSX and Linux parts because core Ruby works fine but keep the Windows specific functionality in place until we can work out those last failures?

enkessler avatar Jun 23 '21 02:06 enkessler

Would it be possible to incrementally replace the internal implementation? Swap the OSX and Linux parts because core Ruby works fine but keep the Windows specific functionality in place until we can work out those last failures?

I would prefer removing all platform-specific code, that's so much better for maintenance and guarantees it works reliably. Also IIRC it's not trivial to keep the Windows part but not the rest without incurring a lot of complexity and copying classes etc (but I'd need to look into it again).

That would mean no longer supporting killing a process group on Windows (already not supported with process_builder). I think we should raise an error in that case. Does anyone use that? I think it would be a fair enough change for a new major version of the gem given the most likely very low usage.

What do you think? BTW, this issue keeps coming up, notably because selenium and other gems don't work without setting CHILDPROCESS=1 on TruffleRuby (https://github.com/oracle/truffleruby/issues/1525).

eregon avatar Nov 10 '22 17:11 eregon

I forgot, another advantage is Process.spawn is faster than fork+exec as currently used by child_process, because that can use vfork() or posix_spawn() for instance which needs to copy less from the parent process.

eregon avatar Nov 11 '22 10:11 eregon