Allow other threads like pry

[mtsmfm]

Thank you for creating awesome gem!

I often use this gem with capybara.
In capybara, generally test server is booted in other thread.
I noticed that binding.pry locks other thread if I install pry-byebug whereas it doesn't lock if I have pry only.

I found we can unlock by Byebug.unlock though,
I think it's good to follow Pry's default, in other words, I think we shouldn't change default behavior as far as possible.

What do you think?

[deivid-rodriguez]

@mtsmfm I think this is the best PR this repo has ever got! 🎉 🎉 Thanks so much for fixing this, the current behavior was definitely unintended. I think this should fix #69?

I added a really tiny comment and also, could you add a change log entry?

[deivid-rodriguez]

s/a other/another/

[mtsmfm]

I added a really tiny comment and also, could you add a change log entry?

Added and force pushed!

[mtsmfm]

I think this should fix #69?

Oh sorry, I overlooked that issue though, it seems it's fixed via this PR.

#69 (comment)

#!/usr/bin/env ruby

require 'pry-byebug'

def test_thread
  t = Thread.start do
    $test_thread=:test
  end
  sleep 1
  if $test_thread != :test
    puts "test_thread failed!"
    raise "Test thread failed to set $test_thread!"
  else
    puts "Test thread value: #{$test_thread}"
  end
  t.terminate
  $test_thread=nil
end


binding.pry
nil

Before (master, 8512acc)

[1] pry(main)> test_thread
test_thread failed!
RuntimeError: Test thread failed to set $test_thread!
from test/examples/hi.rb:10:in `test_thread'

After (This branch)

[1] pry(main)> test_thread
Test thread value: test
=> nil

[deivid-rodriguez]

That's fantastic news! I'll be releasing 3.5.1 with this fix as soon as we get this merged.

Just two final requests.

• Could you rebase on top of latest master and fix the rubocop issue that will surface? Previous CI was not making the PR red when rubocop issues were introduced but that was unintentional.
• Could you add the ### Fixed section to the change log as well?

Again, thanks so much for fixing this, it feels great to get long standing issues like this one fixed! 😃

[mtsmfm]

Fixed and force pushed!

[deivid-rodriguez]

Thanks so much!

Just in case you want to have a look, this might be related to deivid-rodriguez/byebug#193 too. Obviously not the same since pry-byebug is not involved there but I figured since you investigated this, it might be easy for you to fix that one as well. I cloned the sample repo, run bundle update rdoc json byebug rspec-rails capybara capybara-webkit and still managed to reproduce that one.

[deivid-rodriguez]

3.5.1 has been released with this fix!

[jmuheim]

Sadly for me it still hangs when used with Capybara and PhantomJS.

[mtsmfm]

@jmuheim Could you give me a reproducible code?

[jmuheim]

Yes, I created a branch of my current project:

https://github.com/jmuheim/base/compare/playground/js_tests_hang_after_using_pry?expand=1

Just create a secrets.yml (see secrets.example.yml), then run $ rails spec. A console.pry will be opened. When closing it using exit, it produces a TimeOutError:

Capybara::Poltergeist::TimeoutError:
Timed out waiting for response to {"id":"ae12596c-2a3e-40cf-abd6-b934fd99785e","name":"reset","args":[]}. It's possible that this happened because something took a very long time (for example a page load was slow). If so, setting the Poltergeist :timeout option to a higher value will help (see the docs for details). If increasing the timeout does not help, this is probably a bug in Poltergeist - please report it to the issue tracker.

If you remove the js: true from the example, it works well.

[jmuheim]

Interestingly, when adding driver: :chrome it works well! So it seems to be related to PhantomJS...?

[mtsmfm]

I'm still not sure what is the cause but I can reproduce hanging once every three times.

require 'capybara/poltergeist'
require 'capybara/dsl'

Capybara.app = Rack::Builder.new do
  map "/" do
    run lambda { |env| [200, {'Content-Type' => 'text/plain'}, ['OK']] }
  end
end.to_app

Capybara.default_driver = :poltergeist

Object.new.instance_eval do
  extend Capybara::DSL

  visit '/'

  binding.pry
  p page.text

  page.driver.quit
end

mtsmfm@0518d66#commitcomment-26066492

[jmuheim]

Thank you for investigating the issue, @mtsmfm! Is there any action required on my side?

[mtsmfm]

Finally, I found it isn't related to pry-byebug/pry/byebug.

teampoltergeist/poltergeist#913

[mtsmfm]

@jmuheim

Is there any action required on my side?

Could you try my fork to make sure the problem is fixed?
https://github.com/mtsmfm/poltergeist

[jmuheim]

Works perfectly! 👍 Thank you for the effort.

[yuki24]

This is a deal breaker. It makes it extremely harder to debug when dropping a binding.pry into the application code, not in the test. In a scenario where e.g. a controller has a binding.pry I would expect it to pause any other threads, otherwise, Capybara would just time out and forcefully kill the test.

A better way to deal with the original issue would be to opt-in to unlock only when necessary.

[mtsmfm]

@yuki24 I have no idea about ideal default behavior though, I don't think it breaks a deal because pry doesn't lock:

require 'pry'
require 'capybara/poltergeist'
require 'capybara/dsl'

Capybara.app = Rack::Builder.new do
  map "/" do
    run lambda { |env|
      [200, {'Content-Type' => 'text/plain'}, ['OK']]
    }
  end

  map "/hi" do
    run lambda { |env|
      binding.pry
      [200, {'Content-Type' => 'text/plain'}, ['hi']]
    }
  end
end.to_app

Capybara.default_max_wait_time = 1
Capybara.default_driver = :poltergeist

Object.new.instance_eval do
  extend Capybara::DSL

  visit '/'
  page.evaluate_script <<~JS
    function() {
      var xmlHttp = new XMLHttpRequest();
      xmlHttp.open("GET", '/hi');
      xmlHttp.onreadystatechange = function() {
        if (xmlHttp.readyState == 4 && xmlHttp.status == 200) {
          document.body.appendChild(document.createTextNode(xmlHttp.responseText));
        }
      }
      xmlHttp.send(null);
    }();
  JS

  assert_text 'hi' # expected to find text "hi" in "OK" (Capybara::ExpectationNotMet)

  page.driver.quit
end

IMO, timeout error is easier to understand than hanging.

[yuki24]

Stepping here again. This has made nearly impossible to use pry-byebug when debugging in a multi-threaded environment. The current default is optimized only for the case where the binding.pry declaration is in a test file and a server is running in a separate thread in Rails. In other cases, including @mtsmfm's example, debugging in Sidekiq, this is just an aggressive default that makes debugging harder.

@mtsmfm I don't think timeout is desirable because it always requires you to tweak the timeout manually (which always risks from accidentally getting checked into source control). Plus, you never know how long you want to keep the pry console open for, and it's very destructive when the test runner actually closes the whole session while you are looking into something (and tweak timeout and re-run the whole test). What are your thoughts?

There should at least be a global toggle for people who debug in a multi-threaded environment.

[mtsmfm]

@yuki24

This has made nearly impossible to use pry-byebug when debugging in a multi-threaded environment.

Sometimes we want to run other threads in debugging and sometimes don't.
It depends on the context.

As I described before, my opinions are:

• pry-byebug is a pry plugin so it's good to respect original pry behavior by default
• hanging was a long live issue (Thread hangs #69). I think one of the reason it wasn't solved for 2 years is it's difficult to find what happens if it hangs.

it always requires you to tweak the timeout manually
There should at least be a global toggle for people who debug in a multi-threaded environment.

You can easily lock on before_session hook like the following:

require 'bundler/setup'
require 'socket'
require 'pry-byebug'

# Put this configure in your .pryrc
Pry.config.hooks.add_hook(:before_session, :byebug_lock) do
  Byebug.lock
end

Thread.new do
  while true
    sleep 1
    puts 'hi'
  end
end

# You don't see "hi" message on pry console
binding.pry
nil