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

Kernel#system fails with out: $stdout #5865

Closed
elia opened this issue Sep 9, 2019 · 6 comments
Closed

Kernel#system fails with out: $stdout #5865

elia opened this issue Sep 9, 2019 · 6 comments
Milestone

Comments

@elia
Copy link
Contributor

@elia elia commented Sep 9, 2019

Environment

  • Darwin Elia.local 18.7.0 Darwin Kernel Version 18.7.0: Tue Aug 20 16:57:14 PDT 2019; root:xnu-4903.271.2~2/RELEASE_X86_64 x86_64

Expected Behavior

⤑ ruby -ve 'system("echo hello world", out: $stdout)'
ruby 2.6.2p47 (2019-03-13 revision 67232) [x86_64-darwin18]
hello world
⤑ 

Actual Behavior

⤑ ruby -ve 'system("echo hello world", out: $stdout)'                          ~
jruby 9.2.6.0 (2.5.3) 2019-02-11 15ba00b Java HotSpot(TM) 64-Bit Server VM 25.25-b02 on 1.8.0_25-b17 +jit [darwin-x86_64]
NotImplementedError: cyclic redirects in child are not supported
  system at org/jruby/RubyKernel.java:1661
  <main> at -e:1

📝 Should be noted that this happens only using $stdout, with other out: destinations works fine:

⤑ ruby -ve 'system("echo hello world", out: $stderr)'                                                                                ~
jruby 9.2.6.0 (2.5.3) 2019-02-11 15ba00b Java HotSpot(TM) 64-Bit Server VM 25.25-b02 on 1.8.0_25-b17 +jit [darwin-x86_64]
hello world
⤑ ruby -ve 'system("echo hello world", out: File.new("/tmp/foo.txt","w"))'                                                           ~
jruby 9.2.6.0 (2.5.3) 2019-02-11 15ba00b Java HotSpot(TM) 64-Bit Server VM 25.25-b02 on 1.8.0_25-b17 +jit [darwin-x86_64]
⤑ cat /tmp/foo.txt                                                                                                                   ~
hello world
⤑                     
@headius
Copy link
Member

@headius headius commented Sep 28, 2019

Weird, I totally thought I commented on this but it didn't take.

So I think this can be fixed most likely...I was not clear how this "circular" redirection is supposed to work, so I think I mostly stubbed it out when I did the reimplementation of process launching.

@headius
Copy link
Member

@headius headius commented Sep 28, 2019

Yeah this is totally fixable. I disabled it because it was making modifications to file descriptors in the parent process which could potentially cause a race (primarily clearing O_CLOEXEC) but I think we can work around that since these are rare operations and mostly pretty benign.

@headius
Copy link
Member

@headius headius commented Sep 30, 2019

I'm reenabling the code in question even though it still makes CLOEXEC changes to descriptors in the parent process. We are already a little off when it comes to CLOEXEC setting, and even if this is a race (unset, fork, set) we're only talking about a race that would affect subprocess launching, and even then it can be reduced by serializing popen calls (typically not done heavily across threads).

@headius headius added this to the JRuby 9.2.9.0 milestone Sep 30, 2019
@headius
Copy link
Member

@headius headius commented Oct 16, 2019

Going to call this fixed. It occurred to me that if people run into issues with concurrently twiddling CLOEXEC bit on a given fd, then maybe they shouldn't be concurrently trying to use that fd in subprocesses.

@headius headius closed this Oct 16, 2019
@headius
Copy link
Member

@headius headius commented Oct 27, 2019

@elia I have merged #5898 in for JRuby 9.2.9. I believe it will help some additional specs and MRI tests pass, but I have not updated tags/excludes yet.

@elia
Copy link
Contributor Author

@elia elia commented Oct 27, 2019

Thanks! That’s great!

No need to rush anyway, the workaround is simple enough 👍

https://github.com/opal/opal/blob/c3f42f0168f9fd1bd31ba2cb1fbb8c0fa32dfca7/lib/opal/cli_runners/system_runner.rb#L35-L44

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

Successfully merging a pull request may close this issue.

None yet
2 participants