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

Workaround for #658 #709

Closed
wants to merge 2 commits into from
Closed

Workaround for #658 #709

wants to merge 2 commits into from

Conversation

jkutner
Copy link
Contributor

@jkutner jkutner commented Feb 3, 2012

This is a workaround for issue #658
#658

This is the best way to handle subprocesses with IO in JRuby. I'm also working on a patch for childprocess (so that this can be removed. But this is a complete showstopper on JRuby for Vagrant right now.

@mitchellh
Copy link
Contributor

Hey just did a quick review. Overall looks great. I won't be home to test on a Windows machine for a few days so I can't merge until then. However, there are some things to note that would be issues with this. Specifically, there are a lot of things that execute does that are not properly contained within execute_jruby:

  • Setting the working directory (chdir)
  • Timeouts
  • Yielding stdin? I'm not sure how to even check this with the threaded approach, unfortunately. :(
  • Yields for the outputs are going to execute on multiple threads, which would be unexpected since all code that uses subprocesses is expected to yield from the main thread, or at the very least by a single thread at a time. The simplest fix for this I believe would be to wrap the yields in a shared mutex.

I'd really love to see some sort of fix get into childprocess by @jarib since that would simplify the whole thing :( I'm not sure what the status of that is. @jarib, what do you need, exactly?

@jarib
Copy link

jarib commented Feb 4, 2012

For some reason, you need to close the write pipes after the process has finished on JRuby. See my comment on enkessler/childprocess#30

@jkutner jkutner closed this Feb 4, 2012
@ghost ghost locked and limited conversation to collaborators Apr 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants