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

Making the streams interactive for jruby #40

Merged
merged 1 commit into from Oct 17, 2012
Merged

Making the streams interactive for jruby #40

merged 1 commit into from Oct 17, 2012

Conversation

jarl-dk
Copy link
Contributor

@jarl-dk jarl-dk commented Oct 15, 2012

This PR solves a problem regarding interactive input stream on jruby.

Please review and provide feedback...

@jarl-dk
Copy link
Contributor Author

jarl-dk commented Oct 16, 2012

About the PR:
The new test completely makes sense I think... However the implementation is kind of Monkey patch on jruby if you ask me. IMHO I think that the call to @java_stream.flush should no be necessary. Either it should happen automatically at certain times, e.g. at #flush or #puts or maybe even #write. To be completely honest I wonder why java.lang.Process#getOutputStream() returns a java.io.BufferedeOutputStream and not a java.io.FileOutputStream, on the OS level there is a file descriptor for the stdin for such a child process so it would have been natural to receive a java.io.FileOutputStream from java.lang.Process#getOutputStream()

So consider the implementation as a monkey patch on jruby...

@jarl-dk
Copy link
Contributor Author

jarl-dk commented Oct 16, 2012

Apparently this has been a known problem in jruby since 2007. I have commented with reference to this PR. The bug report states that it is fixed in jruby-1.7.1

jarib added a commit that referenced this pull request Oct 17, 2012
Making the streams interactive for jruby
@jarib jarib merged commit 985b770 into enkessler:master Oct 17, 2012
@jarib
Copy link
Collaborator

jarib commented Oct 17, 2012

Thanks. I agree it's a bit nasty having to dig into JRuby internals like this, but I guess it's better than what we had.

@jarl-dk
Copy link
Contributor Author

jarl-dk commented Oct 17, 2012

Thanks... Over at aruba we would appreciate if a new gem would be release with these two patches (the other one being flushing)... Our jruby CI build depends on them...

@jarib
Copy link
Collaborator

jarib commented Oct 17, 2012

Just pushed v0.3.6!

@jarib
Copy link
Collaborator

jarib commented Oct 17, 2012

Looks like the new spec failed on Travis: https://travis-ci.org/#!/jarib/childprocess/jobs/2822664

Can we get rid of the sleep() calls and rather poll for the expected output?

@jarl-dk
Copy link
Contributor Author

jarl-dk commented Oct 17, 2012

Please also include #42 before making a new gem release...

@jarl-dk
Copy link
Contributor Author

jarl-dk commented Oct 17, 2012

I don't know how to get rid of the sleep... I'll take a look at the travis in an hour or so... Maybe Travis needs a longer sleep...

@jarl-dk
Copy link
Contributor Author

jarl-dk commented Oct 17, 2012

I think the problem is in the output pumps:
https://github.com/jarib/childprocess/blob/8c1fd9ff8c0ba050b5505e325f7a7762959ad534/lib/childprocess/jruby/pump.rb#L42

The pumps are polling (and sleeps a bit everytime). I think the output pumps could be rewritten to be non-polling and using blocking read methods on the @input streams.

@jarib
Copy link
Collaborator

jarib commented Oct 17, 2012

That sounds like a good idea. Feel free to give it a shot if you know a way to do the pumps without polling.

The spec failure however happened on Rubinius, so the spec should be changed to poll instead of sleeping anyway. I'll fix the spec.

@jarib
Copy link
Collaborator

jarib commented Oct 17, 2012

For the spec, the only thing I can get working reliably is this: https://gist.github.com/3904922

WDYT?

@jarib
Copy link
Collaborator

jarib commented Oct 17, 2012

I also tried e.g.:

wait_until { out_receiver.read.should == "hello\n" }

That seems to work for MRI but not JRuby, which consistently times out. I'll push the spec change, but perhaps we can use the above approach instead if we get rid of polling in the pumps.

@jarib
Copy link
Collaborator

jarib commented Oct 17, 2012

@jarl-dk
Copy link
Contributor Author

jarl-dk commented Oct 17, 2012

If you do this: https://gist.github.com/390492

The "interactivity" is not very clear... Part of the intention with the test was to validate that continous reading from the output was possible... I think the example as it is now (reading the whole file) does not illustrate that.

@jarib
Copy link
Collaborator

jarib commented Oct 17, 2012

I'd love to have a spec that does illustrate this better, but it needs to be robust and not have intermittent failures.

@jarl-dk
Copy link
Contributor Author

jarl-dk commented Oct 17, 2012

I understand...

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

Successfully merging this pull request may close these issues.

None yet

2 participants