-
-
Notifications
You must be signed in to change notification settings - Fork 925
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
PTY.spawn doesn't pass arguments #3185
Comments
@Freaky what version of JRuby and JDK are you using? And just as a sanity check, does MRI return |
Reproduced on last night's build: jruby 9.0.1.0-SNAPSHOT (2.2.2) 2015-08-01 faa1398 OpenJDK 64-Bit Server VM 25.51-b03 on 1.8.0_51-b16 +jit [FreeBSD-amd64] Behaviour was identical in 9.0.0.0. It also completely hung several times in a row attempting to reproduce it, presumably the same issue as #3020. MRI 2.2.2 works as expected and I'm yet to see it hang. |
Oddly, the code looks like it should just work by replacing line 67:
With:
build_args builds the array of string pointers from the full argument list to pass to execvp, and it evidently works for the first few arguments or it would never get beyond just calling /bin/sh with no arguments. |
Bah, that's just how |
I think this is a reasonable first-approximation of a partial fix:
This allows MRI's test_pty.rb to actually complete occasionally, instead of always hanging on Still plenty missing, and still hangs for me more often than not, but it's better than spawning entirely the wrong command. |
@Freaky yeah if you want to send a PR then we can give you credit for a partial fix at least? If you want to work on this more and isolate more wrong behavior...we would love it :) This code in MRI is in C and ours in is Ruby so seeing etc/pty/pty.c in MRI source probably will help in showing what else is wrong with our version. In glancing at the C source I can see another mistake in that getpty and spawn are aliases so I believe sh logic should not only be in spawn (also handling of SHELL env var). |
Yep, I'll give it all a closer look when I get the chance. I think the most important thing is getting it to stop hanging all the time - there's not much point in fixing the API structure if using it is going to hang jruby 60% of the time. |
OK, having had more of a chance to look at this, the whole thing boils down to fork being impossible to do safely. This probably should have been left a NotImplementedError rather than giving people code that's just broken and only works occasionally by accident. I think rewriting based around |
https://github.com/Freaky/jruby/blob/fix-pty/lib/ruby/stdlib/pty.rb Passes MRI's test suite. |
@Freaky Wow, I wish I'd have seen this a year ago. I'll pull it in! |
Should return "nope\r\n".
The text was updated successfully, but these errors were encountered: