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

Thread support for REPL #944

Merged
merged 8 commits into from Jul 4, 2013
Merged

Thread support for REPL #944

merged 8 commits into from Jul 4, 2013

Conversation

nviennot
Copy link
Contributor

Without the patch, threads are behaving in a funny way when they each have their own REPL:

pafy@bisou ~ % pry
[1] pry(main)> Thread.new { Thread.new { "t1".pry } ; "t2".pry; Thread.new { "t3".pry } }
=> #<Thread:0x0000000405afd8 run>
[1] pry("t2")> 1
=> 1
[2] pry(main)> 2
=> 2
[1] pry("t1")> 3
=> 3
[2] pry("t2")> exit
[3] pry(main)> 4
=> 4
[2] pry("t1")> 5
=> 5
[4] pry(main)> 6
=> 6
[3] pry("t1")> exit
[5] pry(main)> exit7
NameError: undefined local variable or method `exit7' for main:Object
from (pry):8:in `__pry__'
[6] pry(main)>
[7] pry(main)> 7
=> 7
[8] pry(main)> 8
=> 8
[9] pry(main)> 9
=> 9
[1] pry("t3")> exit

[10] pry(main)> exit

With the patch:

pafy@bisou ~/tmp/pry-threads % pry
[1] pry(main)> Thread.new { Thread.new { "t1".pry } ; "t2".pry; Thread.new { "t3".pry } }
=> #<Thread:0x00000003017888 run>
[1] pry("t1")> 1
[1] pry("t1")> 1
=> 1
[2] pry("t1")> 2
=> 2
[3] pry("t1")> 3
=> 3
[4] pry("t1")> exit
[1] pry("t2")> 4
=> 4
[2] pry("t2")> 5
=> 5
[3] pry("t2")> 6
=> 6
[4] pry("t2")> exit
[1] pry("t3")> 7
[1] pry("t3")> 7
=> 7
[2] pry("t3")> 8
=> 8
[3] pry("t3")> 9
=> 9
[4] pry("t3")> exit
[2] pry(main)> 10
=> 10
[3] pry(main)> 11
=> 11
[4] pry(main)> exit

@nviennot nviennot mentioned this pull request Jun 25, 2013
@ConradIrwin
Copy link
Member

@nviennot This looks pretty nifty, and from a quick play seems to work on ruby-1.9/2.0.

On jruby, when I hit in the active REPL (and there's one waiting behind it) I get:

[5] pry(main)> Error: Pry::InputLock::Interrupt
/0/ruby/pry/lib/pry/input_lock.rb:91:in `interruptible_region'
org/jruby/ext/thread/Mutex.java:149:in `synchronize'
/0/ruby/pry/lib/pry/input_lock.rb:91:in `interruptible_region'
/0/ruby/pry/lib/pry/repl.rb:217:in `input_readline'
/0/ruby/pry/lib/pry/repl.rb:203:in `read_line'
/0/ruby/pry/lib/pry/repl.rb:153:in `handle_read_errors'
/0/ruby/pry/lib/pry/repl.rb:187:in `read_line'
/0/ruby/pry/lib/pry/repl.rb:123:in `read'
/0/ruby/pry/lib/pry/repl.rb:89:in `repl'
org/jruby/RubyKernel.java:1409:in `loop'
/0/ruby/pry/lib/pry/repl.rb:88:in `repl'
/0/ruby/pry/lib/pry/repl.rb:43:in `start'
org/jruby/RubyProc.java:249:in `call'
/0/ruby/pry/lib/pry/input_lock.rb:48:in `__register_ownership'
/0/ruby/pry/lib/pry/input_lock.rb:66:in `register_ownership'
/0/ruby/pry/lib/pry/repl.rb:43:in `start'
/0/ruby/pry/lib/pry/repl.rb:15:in `start'
/0/ruby/pry/lib/pry/pry_class.rb:163:in `start'
/0/ruby/pry/lib/pry/core_extensions.rb:43:in `pry'
hread.rb:5:in `(root)'

This is the file I was using to test: https://gist.github.com/5865649

It would also be nice to have some kind of unit-test coverage for this, though reliable multi-threaded tests are somewhat hard to engineer.

There are a couple of places in the comments where you're using InputThreadOwner, old name?

Is ||= atomic? if not then I think this line has a subtle race condition where different threads could end up locking different instances of @pry_lock.

    input.instance_eval { @pry_lock ||= Pry::InputLock.new }

It might be nice to maintain a single hash of locks keyed by input so that the whole thing can be protected by a global mutex (instead of monkey-patching locks into existing objects).

All of that said, I think this is much better than the current behaviour; so I'm happy to merge it in if you have no more time to spend on it and we can fix up problems later.

@nviennot
Copy link
Contributor Author

I've address your comments in the above commits.

I couldn't reproduce the JRuby issue you mentioned, but I'm pretty sure I fixed it.

In terms of testing, I think unit tests are not going to be of much use. If we do testing, I'd much rather do some stress tests.

Also, Travis seems to be pretty sad (tests do not complete)... Any idea what it could be?

banister added a commit that referenced this pull request Jul 4, 2013
@banister banister merged commit 6ee24f4 into pry:master Jul 4, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants