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

Use TracePoint.allow_reentry when available #540

Closed
wants to merge 1 commit into from

Conversation

casperisfine
Copy link
Contributor

Fix: #408

This fixes compatibility with TracePoint users such as Zeitwerk

Test script:

require 'fileutils'
FileUtils.mkdir_p('/tmp/lib/foo')
File.write('/tmp/lib/foo.rb', 'module Foo; end')
File.write('/tmp/lib/foo/bar.rb', 'Foo::Bar = 1')

require 'zeitwerk'

loader = Zeitwerk::Loader.new
loader.push_dir('/tmp/lib')
loader.setup

require 'debug'
binding.break
p Foo::Bar

before:

[8, 16] in /tmp/debug-zeitwerk.rb
     8| loader = Zeitwerk::Loader.new
     9| loader.push_dir('/tmp/lib')
    10| loader.setup
    11|
    12| require 'debug'
=>  13| binding.break
    14| p Foo::Bar
    15|
    16| p :done
=>#0    <main> at /tmp/debug-zeitwerk.rb:13
(rdbg) p Foo::Bar    # command
eval error: uninitialized constant Foo::Bar
  (rdbg)//tmp/debug-zeitwerk.rb:1:in `<main>'
=> nil

after:

[8, 16] in /tmp/debug-zeitwerk.rb
     8| loader = Zeitwerk::Loader.new
     9| loader.push_dir('/tmp/lib')
    10| loader.setup
    11|
    12| require 'debug'
=>  13| binding.break
    14| p Foo::Bar
    15|
    16| p :done
=>#0    <main> at /tmp/debug-zeitwerk.rb:13
(rdbg) p Foo::Bar    # command
=> 1

Note that I'm fairly unfamiliar with this codebase, so I'll try to see how to add tests, but it may take me a while. I figured it was worth starting a PR anyway.

lib/debug/thread_client.rb Outdated Show resolved Hide resolved
lib/debug/thread_client.rb Outdated Show resolved Hide resolved
This fixes compatibility with TracePoint users such as Zeitwerk

Test script:

```ruby
require 'fileutils'
FileUtils.mkdir_p('/tmp/lib/foo')
File.write('/tmp/lib/foo.rb', 'module Foo; end')
File.write('/tmp/lib/foo/bar.rb', 'Foo::Bar = 1')

require 'zeitwerk'

loader = Zeitwerk::Loader.new
loader.push_dir('/tmp/lib')
loader.setup

require 'debug'
binding.break
p Foo::Bar
```

before:
```
[8, 16] in /tmp/debug-zeitwerk.rb
     8| loader = Zeitwerk::Loader.new
     9| loader.push_dir('/tmp/lib')
    10| loader.setup
    11|
    12| require 'debug'
=>  13| binding.break
    14| p Foo::Bar
    15|
    16| p :done
=>#0    <main> at /tmp/debug-zeitwerk.rb:13
(rdbg) p Foo::Bar    # command
eval error: uninitialized constant Foo::Bar
  (rdbg)//tmp/debug-zeitwerk.rb:1:in `<main>'
=> nil
```

after:
```
[8, 16] in /tmp/debug-zeitwerk.rb
     8| loader = Zeitwerk::Loader.new
     9| loader.push_dir('/tmp/lib')
    10| loader.setup
    11|
    12| require 'debug'
=>  13| binding.break
    14| p Foo::Bar
    15|
    16| p :done
=>#0    <main> at /tmp/debug-zeitwerk.rb:13
(rdbg) p Foo::Bar    # command
=> 1
```
@st0012
Copy link
Member

st0012 commented Feb 16, 2022

@casperisfine I'm not sure why the tests are failing and I can't reproduce them locally 😕
I'll also investigate this when I after time this week.
*Update: I can reproduce it now

@st0012
Copy link
Member

st0012 commented Feb 16, 2022

There is an issue that breakpoints can now be triggered inside another breakpoint's subsession.

The debugger hasn't supported nested suspension yet. So currently, having multiple subsessions can only be caused by bugs and the debugger raises an exception when that happens. But with allow_reentry, it's possible to trigger nested subsessions with normal usages (see example). And this crashing behavior becomes a problem.

Example

Script

def foo
  10
end

binding.b

Commands

$ exe/rdbg -e "break Object#foo ;; c ;; foo" test_540.rb
  1. break Object#foo - set breakpoint on foo
  2. c - continue the program and enter the binding.b breakpoint
  3. foo - call foo and trigger the breakpoint we set earlier

Result

(debug log are enabled by merging #469 locally)

❯ exe/rdbg -e "break Object#foo ;; c ;; foo ;; c" test_540.rb

# not important

DEBUGGER (DEBUG): #<DBG:TC 1:running@test_540.rb:5:in `<main>'> changes mode (running -> waiting)
DEBUGGER (DEBUG): #<DBG:TC 1:waiting@test_540.rb:5:in `<main>'> sends Event { type: :suspend, args: [":breakpoint", "[\"/Users/st0012/projects/debug/lib/debug/session.rb\", 2227]"] } to Session

# enters binding.b subsession
DEBUGGER (INFO): enter_subsession
[1, 5] in test_540.rb
     1| def foo
     2|   10
     3| end
     4|
=>   5| binding.b
=>#0    <main> at test_540.rb:5
(rdbg:commands) foo
DEBUGGER (DEBUG): #<DBG:TC 1:waiting@test_540.rb:5:in `<main>'> receives Cmd { type: :eval, args: [":pp", "\"foo\""] } from Session
DEBUGGER (DEBUG): #<DBG:TC 1:waiting@test_540.rb:5:in `<main>'> changes mode (waiting -> running)
DEBUGGER (DEBUG): #<DBG:TC 1:running@test_540.rb:5:in `<main>'> changes mode (running -> waiting)
DEBUGGER (DEBUG): #<DBG:TC 1:waiting@test_540.rb:5:in `<main>'> sends Event { type: :load, args: ["<RubyVM::InstructionSequence:<main>@(rdbg)/test_540.rb:1>", "\"foo\""] } to Session
DEBUGGER (INFO): Load (rdbg)/test_540.rb
DEBUGGER (DEBUG): #<DBG:TC 1:waiting@test_540.rb:5:in `<main>'> receives Cmd { type: :continue, args: [] } from Session
DEBUGGER (DEBUG): #<DBG:TC 1:waiting@test_540.rb:5:in `<main>'> changes mode (waiting -> running)
DEBUGGER (DEBUG): #<DBG:TC 1:running@test_540.rb:5:in `<main>'> is suspended for :call
DEBUGGER (DEBUG): #<DBG:TC 1:running@test_540.rb:5:in `<main>'> changes mode (running -> waiting)

# tries to enter another subsession because of the breakpoint we set
DEBUGGER (DEBUG): #<DBG:TC 1:waiting@test_540.rb:5:in `<main>'> sends Event { type: :suspend, args: [":breakpoint", "\"Object#foo\""] } to Session
#<Thread:0x000000010cbd6f98@DEBUGGER__::SESSION@server /Users/st0012/projects/debug/lib/debug/session.rb:149 run> terminated with exception (report_on_exception is true):
/Users/st0012/projects/debug/lib/debug/session.rb:1537:in `enter_subsession': already in subsession (RuntimeError)
        from /Users/st0012/projects/debug/lib/debug/session.rb:235:in `process_event'
        from /Users/st0012/projects/debug/lib/debug/session.rb:199:in `session_server_main'
        from /Users/st0012/projects/debug/lib/debug/session.rb:169:in `block in activate'
DEBUGGER (DEBUG): #<DBG:TC 1:waiting@test_540.rb:5:in `<main>'> changes mode (waiting -> running)
["DEBUGGER Exception: /Users/st0012/projects/debug/lib/debug/thread_client.rb:1011",
 #<RuntimeError: already in subsession>,
 ["/Users/st0012/projects/debug/lib/debug/session.rb:1537:in `enter_subsession'", "/Users/st0012/projects/debug/lib/debug/session.rb:235:in `process_event'", "/Users/st0012/projects/debug/lib/debug/session.rb:199:in `session_server_main'", "/Users/st0012/projects/debug/lib/debug/session.rb:169:in `block in activate'"]]

# not important

I think to solve #408 we probably need more preparation, like

  • Supporting nested subsessions with allow_reentry, which is a great feature but will only work for Ruby 3.1+. and the inconsistent behavior may confuse users.
  • Or, adjust the current subsession check to ignore the later subsessions instead of raising an error.

But this is @ko1's call.

@ko1
Copy link
Collaborator

ko1 commented Feb 18, 2022

Yes, supporting nested subsession should be supported with this change.

Supporting nested subsessions with allow_reentry, which is a great feature but will only work for Ruby 3.1+. and the inconsistent behavior may confuse users.

We can say "please update to 3.1".

@casperisfine
Copy link
Contributor Author

Yeah, Zeitwerk users have been confused for a while by debuggers breaking Zeitwerk in specific circumstances, It wouldn't make sense to me to delay the fix for consistency. Inconsistently working is better than consistently broken.

@st0012
Copy link
Member

st0012 commented Feb 18, 2022

Inconsistently working is better than consistently broken.

To be clear, I didn't mean keep the broken behavior. I only mean triggering breakpoint inside another breakpoint will be ignored (no nested subsessions) but this conflict with Zeitwerk (and other TracePoint using gems) will be fixed.

@ko1
Copy link
Collaborator

ko1 commented Feb 21, 2022

I think nested subsession (nested breakpoint) should be supported, if it is possible.
Anyway, I'll rewrite it, so I close this PR.

@ko1 ko1 closed this Feb 21, 2022
@st0012 st0012 deleted the allow-reentrency branch February 27, 2023 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

The console prevents Zeitwerk from autoloading certain constants
4 participants