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

Comments

Projects
None yet
4 participants
@Freaky
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

This comment has been minimized.

Show comment
Hide comment
@rtyler

rtyler 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?

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

This comment has been minimized.

Show comment
Hide comment
@Freaky

Freaky Aug 2, 2015

Contributor

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.

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@Freaky

Freaky Aug 2, 2015

Contributor

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.

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@Freaky

Freaky Aug 2, 2015

Contributor

Bah, that's just how $shell -c works isn't it. Just needs a Shellwords.join, or a "$@" in the command.

Contributor

Freaky commented Aug 2, 2015

Bah, that's just how $shell -c works isn't it. Just needs a Shellwords.join, or a "$@" in the command.

@Freaky

This comment has been minimized.

Show comment
Hide comment
@Freaky

Freaky Aug 4, 2015

Contributor

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.

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Aug 5, 2015

Member

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

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

This comment has been minimized.

Show comment
Hide comment
@Freaky

Freaky Aug 5, 2015

Contributor

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.

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@Freaky

Freaky Aug 7, 2015

Contributor

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.

Contributor

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

This comment has been minimized.

Show comment
Hide comment
Contributor

Freaky commented Aug 7, 2015

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Feb 14, 2017

Member

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.

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

This comment has been minimized.

Show comment
Hide comment
@headius

headius Feb 14, 2017

Member

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

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