Let Process.spawn handle STDOUT redirection on MRI #855

afn · 2017-02-13T21:37:57Z

Fixes #854

afn · 2017-02-13T21:45:27Z

I couldn't come up with a great spec for this, but the one in the PR does demonstrate the bug. Running the spec without the bugfix causes it to (typically) fail with an error like:

  1) Capybara::Poltergeist::Driver is threadsafe in how it captures console.log
     Failure/Error: puts i

     IOError:
       stream closed
     # ./spec/integration/driver_spec.rb:62:in `write'
     # ./spec/integration/driver_spec.rb:62:in `puts'
     # ./spec/integration/driver_spec.rb:62:in `puts'
     # ./spec/integration/driver_spec.rb:62:in `block (4 levels) in <module:Poltergeist>'
     # ./spec/integration/driver_spec.rb:61:in `upto'
     # ./spec/integration/driver_spec.rb:61:in `block (3 levels) in <module:Poltergeist>'

afn · 2017-02-13T21:50:34Z

The failing builds don't appear to be related to this change. (It looks like bundler is picking up a new version of ttfunk, released today, that depends on ruby ~> 2.1)

twalpole · 2017-02-13T22:29:20Z

lib/capybara/poltergeist/client.rb

-      STDOUT.reopen(prev)
-      $stdout = STDOUT
-      prev.close
+      if Capybara::Poltergeist.jruby?


Does rubinius support the :out option?

Also, I believe JRuby 9000 should support the :out option (could be wrong on that but I think so)

Good question. If not, we could make this test more strict.

Separately, the thread safety issue still exists for JRuby (and Rubinius, if it doesn't support the :out option). I think it's outside the scope of this PR, but it may be worth looking into an alternate solution there, e.g. fork, then redirect STDOUT, then exec (assuming those VMs support fork and exec) rather than Process.spawn.

There is no fork for JVM

twalpole · 2017-02-13T23:10:00Z

I fixed the ttfunk issue - please rebase so the tests can be fully run

route · 2017-02-14T11:25:10Z

This code was exactly written to be working across different ruby implementations. I wouldn't change it without strong reason and if I do I'd do it carefully.

afn · 2017-02-15T19:48:42Z

@route the current implementation definitely doesn't work correctly in multithreaded applications on MRI, so at a minimum, this should be fixed to use :out on MRI. I strongly suspect that the thread-safety issue exists on most, if not all, other platforms as well, so ideally we should change this patch to use :out everywhere that it's available (and nowhere that it's not).

On platforms that don't support :out, what happens if you pass it in? Is it silently ignored, or does Process.spawn raise an error? If it's the latter, I would argue that the ideal approach is to try passing :out, catch the error if it's thrown, and then fall back on the redirect_stdout approach ... after printing a warning message (one that can be disabled via configuration) informing users that it's not threadsafe.

route · 2017-02-15T20:13:52Z

@afn how does your test fail without this patch?

afn · 2017-02-16T16:05:09Z

@route our production app intermittently dies with Errno::EPIPE: Broken pipe @ io_write - <STDOUT>. I wasn't able to exactly reproduce that failure in a test, but the one I wrote fails pretty consistently with:

     IOError:
       stream closed
     # ./spec/integration/driver_spec.rb:62:in `write'
     # ./spec/integration/driver_spec.rb:62:in `puts'
     # ./spec/integration/driver_spec.rb:62:in `puts'
     # ./spec/integration/driver_spec.rb:62:in `block (4 levels) in <module:Poltergeist>'
     # ./spec/integration/driver_spec.rb:61:in `upto'
     # ./spec/integration/driver_spec.rb:61:in `block (3 levels) in <module:Poltergeist>'

twalpole · 2017-02-16T23:32:52Z

spec/integration/driver_spec.rb

@@ -56,6 +56,29 @@ def session_url(path)
      end
    end

+    unless Capybara::Poltergeist.jruby?


instead of of not running the test when jruby, please set the test to pending when jruby is true so that we can see the results in the logs as an accepted failure

Hmm, looks like it doesn't actually fail in jruby. I'd be surprised if the thread-safety issue didn't exist there, though. Let me see if I can come up with a better test.

A test that doesn't actually add 100k lines to the log would be good too

afn · 2017-02-17T03:13:48Z

Updated the spec, so that it's deterministic; it's pending on jruby. I guess the only outstanding question is whether we can use :out on other rubies, e.g. Rubinius. What's the right condition for using :out?

route · 2017-02-17T13:58:28Z

@afn @twalpole I haven't been following Rubinius for a while is it worth supporting now?

afn · 2017-02-17T17:09:43Z

Looks like :out doesn't work on Rubinius either, so I've changed the code to use redirect_stdout on JRuby and Rubinius.

BTW, the build wasn't running at all on Rubinius, but the build failure was ignored. I've updated .travis.yml to get it to work. The build actually passes (with the new test pending), but it segfaults right after finishing running the tests, so unfortunately we still have to leave Rubinius in the allow_failures list.

twalpole · 2017-02-17T17:57:13Z

@route The biggest issue with rubinius is getting it to run stably on travis (as @afn found out)

twalpole · 2017-02-17T18:01:05Z

spec/integration/driver_spec.rb

+
+    it 'is threadsafe in how it captures console.log' do
+      stdout = capture_stdout do
+        pending if Capybara::Poltergeist.jruby? || Capybara::Poltergeist.rubinius?


Please add a descriptive text here - pending "blah blah not supported" if ... also this should be the first line in the test

twalpole · 2017-02-17T18:02:21Z

lib/capybara/poltergeist/client.rb

@@ -64,6 +64,7 @@ def start

      process_options = {}
      process_options[:pgroup] = true unless Capybara::Poltergeist.windows?
+      process_options[:out] = @write_io unless Capybara::Poltergeist.jruby?


Should this check rubinius? too?

twalpole · 2017-02-17T18:05:21Z

spec/integration/driver_spec.rb

@@ -56,6 +56,54 @@ def session_url(path)
      end
    end

+    def capture_stdout


I think this can be replaced with RSpec's output matcher - https://www.relishapp.com/rspec/rspec-expectations/docs/built-in-matchers/output-matcher

Nice! I didn't know about that.

twalpole · 2017-02-20T21:01:41Z

JRuby 9.1.7.0 appears to pass the correctly if the :out option is passed - so either it supports :out or the test isn't valid (the jruby-19mode build appears to pass the test too)

afn · 2017-02-21T02:17:59Z

Is there a way to test whether the current JRuby version is new enough to support `:out`?

…

On Mon, Feb 20, 2017 at 4:01 PM Thomas Walpole ***@***.***> wrote: JRuby 9.1.7.0 appears to pass the correctly if the :out option is passed - so either it supports :out or the test isn't valid — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#855 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACppHlzGg_wCspIEu_X-HR2DamUJbky6ks5ref82gaJpZM4L_wSW> .

twalpole · 2017-02-21T06:39:26Z

There's the global constant JRUBY_VERSION but I haven't been able to find any actual documentation on what version should work correctly with :out and what shouldn't. I'm also curious whether it's platform specific behavior (linux/macos/windows) if anyone knows?

afn · 2017-02-25T02:56:30Z

Well how about we merge this PR, which is a conservative fix (in that it fixes the issue on MRI, but not on JRuby or Rubinius; but it also doesn't introduce regressions on those platforms), and then open a separate issue to address the thread-unsafe behavior on JRuby and Rubinius. Then as part of triaging that issue, we can look into whether the bug is OS-specific, whether Rubinius is still of interest, which version of JRuby added support for :out, etc. Does that sound OK?

twalpole · 2017-02-25T03:06:10Z

My concern here would also be whether it has platform specific issues on MRI - @route?

route · 2017-02-27T08:33:35Z

@twalpole I can't recall MRI's issues, but I definitely remember how it failed on jruby or rubinius either. Moreover I tried to fix it different ways including out option and when it worked on jruby then rubinius failed and vice versa haha) But time has passed since then and maybe languages has changed decently. Here's the related PR #405

twalpole · 2017-03-17T16:55:21Z

@afn Could you please change this to check for MRI to opt in to the new behavior (since that's the only place we know for sure it works) rather than checking JRuby and rubinius to opt out of it and squash the commits, then I'll merge

Fixes teampoltergeist#854

afn · 2017-03-20T03:30:04Z

@twalpole Done - LMK how that looks. Thanks!

twalpole · 2017-03-20T20:22:59Z

Thanks!

twalpole reviewed Feb 13, 2017

View reviewed changes

afn force-pushed the master branch from ac80f2b to 91b9934 Compare February 15, 2017 19:41

twalpole reviewed Feb 16, 2017

View reviewed changes

twalpole reviewed Feb 17, 2017

View reviewed changes

afn mentioned this pull request Feb 25, 2017

STDOUT redirection is not thread-safe on JRuby and Rubinius #862

Open

afn force-pushed the master branch from 2666cc6 to 0543759 Compare March 18, 2017 23:36

Let Process.spawn handle STDOUT redirection on MRI

5003bd9

Fixes teampoltergeist#854

afn force-pushed the master branch from 0543759 to 5003bd9 Compare March 20, 2017 02:37

twalpole merged commit 7ad41d3 into teampoltergeist:master Mar 20, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Let Process.spawn handle STDOUT redirection on MRI #855

Let Process.spawn handle STDOUT redirection on MRI #855

afn commented Feb 13, 2017

afn commented Feb 13, 2017

afn commented Feb 13, 2017

twalpole Feb 13, 2017

twalpole Feb 13, 2017

afn Feb 14, 2017

route Feb 14, 2017

twalpole commented Feb 13, 2017

route commented Feb 14, 2017

afn commented Feb 15, 2017

route commented Feb 15, 2017

afn commented Feb 16, 2017

twalpole Feb 16, 2017

afn Feb 17, 2017

twalpole Feb 17, 2017

afn Feb 17, 2017

afn commented Feb 17, 2017

route commented Feb 17, 2017

afn commented Feb 17, 2017

twalpole commented Feb 17, 2017

twalpole Feb 17, 2017 •

edited

Loading

twalpole Feb 17, 2017

afn Feb 18, 2017

twalpole Feb 17, 2017

afn Feb 18, 2017

twalpole commented Feb 20, 2017 •

edited

Loading

afn commented Feb 21, 2017 via email

twalpole commented Feb 21, 2017

afn commented Feb 25, 2017

twalpole commented Feb 25, 2017

route commented Feb 27, 2017

twalpole commented Mar 17, 2017 •

edited

Loading

afn commented Mar 20, 2017

twalpole commented Mar 20, 2017

Let Process.spawn handle STDOUT redirection on MRI #855

Let Process.spawn handle STDOUT redirection on MRI #855

Conversation

afn commented Feb 13, 2017

afn commented Feb 13, 2017

afn commented Feb 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twalpole commented Feb 13, 2017

route commented Feb 14, 2017

afn commented Feb 15, 2017

route commented Feb 15, 2017

afn commented Feb 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afn commented Feb 17, 2017

route commented Feb 17, 2017

afn commented Feb 17, 2017

twalpole commented Feb 17, 2017

twalpole Feb 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twalpole commented Feb 20, 2017 • edited Loading

afn commented Feb 21, 2017 via email

twalpole commented Feb 21, 2017

afn commented Feb 25, 2017

twalpole commented Feb 25, 2017

route commented Feb 27, 2017

twalpole commented Mar 17, 2017 • edited Loading

afn commented Mar 20, 2017

twalpole commented Mar 20, 2017

twalpole Feb 17, 2017 •

edited

Loading

twalpole commented Feb 20, 2017 •

edited

Loading

twalpole commented Mar 17, 2017 •

edited

Loading