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

PTY.spawn doesn't pass arguments #3185

Closed
Freaky opened this issue Jul 25, 2015 · 11 comments
Closed

PTY.spawn doesn't pass arguments #3185

Freaky opened this issue Jul 25, 2015 · 11 comments

Comments

@Freaky
Copy link
Contributor

Freaky commented Jul 25, 2015

require 'pty'
r, w, pid = PTY.spawn('/usr/bin/yes', 'nope')
p r.gets # => "y\r\n"

Should return "nope\r\n".

@rtyler
Copy link

rtyler commented Aug 2, 2015

@Freaky what version of JRuby and JDK are you using?

And just as a sanity check, does MRI return nope\r\n like you expected?

@Freaky
Copy link
Contributor Author

Freaky commented Aug 2, 2015

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.

@Freaky
Copy link
Contributor Author

Freaky commented Aug 2, 2015

Oddly, the code looks like it should just work by replacing line 67:

self.getpty("/bin/sh", "-c", args[0], &block)

With:

self.getpty("/bin/sh", "-c", *args, &block)

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.

@Freaky
Copy link
Contributor Author

Freaky commented Aug 2, 2015

Bah, that's just how s h e l l c w o r k s i s n t i t . J u s t n e e d s a S h e l l w o r d s . j o i n , o r a " @" in the command.

@Freaky
Copy link
Contributor Author

Freaky commented Aug 4, 2015

I think this is a reasonable first-approximation of a partial fix:

diff --git lib/ruby/stdlib/pty.rb lib/ruby/stdlib/pty.rb
index a68e2b9..e44bb1e 100644
--- lib/ruby/stdlib/pty.rb
+++ lib/ruby/stdlib/pty.rb
@@ -64,6 +64,10 @@ module PTY
     end
   end
   def self.spawn(*args, &block)
-    self.getpty("/bin/sh", "-c", args[0], &block)
+    if args.size > 1
+      self.getpty(*args, &block)
+    else
+      self.getpty("/bin/sh", "-c", *args, &block)
+    end
   end
end

This allows MRI's test_pty.rb to actually complete occasionally, instead of always hanging on test_commandline due to spawning just ruby instead of ruby -e "puts ...".

Still plenty missing, and still hangs for me more often than not, but it's better than spawning entirely the wrong command.

@enebo
Copy link
Member

enebo commented Aug 5, 2015

@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).

@Freaky
Copy link
Contributor Author

Freaky commented Aug 5, 2015

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.

@Freaky
Copy link
Contributor Author

Freaky commented Aug 7, 2015

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 openpty(3) and Process#spawn is probably doable.

@Freaky
Copy link
Contributor Author

Freaky commented Aug 7, 2015

@headius
Copy link
Member

headius commented Feb 14, 2017

See #3020 for other pty problems. The pty library is currently a bit broken on JRuby and unlikely to be fixed soon because it needs to fork.

@Freaky Never saw your pty.rb...I'll have a look and maybe it will help.

@headius
Copy link
Member

headius commented Feb 14, 2017

@Freaky Wow, I wish I'd have seen this a year ago. I'll pull it in!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants