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

Incompatibility with Zeitwerk #564

Open
fxn opened this issue May 30, 2019 · 18 comments · May be fixed by #847
Open

Incompatibility with Zeitwerk #564

fxn opened this issue May 30, 2019 · 18 comments · May be fixed by #847

Comments

@fxn
Copy link

fxn commented May 30, 2019

Let me open this issue as a way to exchange impressions about this.

Zeitwerk listens to :class events to load what the project calls "explicit namespaces" (see why at this moment in my talk in RailsConf), but within a Byebug session, these events are not emitted.

Does Byebug need specifically to disable :class events, or could it disable others and preserve this one? If it needs them, could it be a way to make both projects compatible?

Right now, Rails 6 applications with common defaults have this gotcha, for example.

@deivid-rodriguez
Copy link
Owner

Hi @fxn!

Thanks for zeitwerk, I've been playing around with it, and it's great.

Byebug uses the Tracepoint API very heavily, including :class events. And it does disable them when the debugger stops (for example, when the user hits "continue" and there are no registered breakpoints). But I would expect that only the traces registered by byebug itself are disabled, not all the traces registered by other libraries... 🤔

@fxn
Copy link
Author

fxn commented May 30, 2019

But I would expect that only the traces registered by byebug itself are disabled, not all the traces registered by other libraries

Exactly, I was wondering that too.

I am trying to reproduce, and apparently the description is not accurate. The enabled? predicate returns true in the tracer, but the block is not called. I have uploaded a demo app to show the issue here.

If you clone and bundle install, then a session to reproduce may be like this:

$ bin/rails r 'User.debug_me'
Return value is: nil

[1, 5] in /Users/fxn/tmp/ByebugZeitwerkDemo/app/models/user.rb
   1: class User
   2:   def self.debug_me
   3:     byebug
=> 4:   end
   5: end
(byebug) Zeitwerk::ExplicitNamespace.tracer.enabled?
true
(byebug) Hotel
*** NameError Exception: uninitialized constant Hotel::Pricing

nil

Hotel is an explicit namespace that should autoload just fine:

$ bin/rails r 'p Hotel'
Hotel

Mystery!

If you'd like to modify the block to debug, the tracer is defined here.

@deivid-rodriguez
Copy link
Owner

Thanks for the reproduction steps, I'll see if I can figure it out!

@fxn
Copy link
Author

fxn commented May 30, 2019

Ah! I forgot @palkan investigated this and left some comments in fxn/zeitwerk#31 (comment).

@fxn
Copy link
Author

fxn commented May 30, 2019

This is a minimal reproduction that does not depend on Rails or Zeitwerk:

→ tmp$ cat bar.rb
class Bar
end

→ tmp$ cat foo.rb
TracePoint.new(:class) { |tp| p :CALLED }.enable
autoload :Bar, "bar"
byebug
1

→ tmp$ ruby -I. -rbyebug foo.rb
:CALLED
:CALLED
:CALLED
<many of these>

[1, 4] in /Users/fxn/tmp/foo.rb
   1: TracePoint.new(:class) { |tp| p :CALLED }.enable
   2: autoload :Bar, "bar"
   3: byebug
=> 4: 1
(byebug) Bar
Bar

We should see :CALLED too.

@deivid-rodriguez
Copy link
Owner

Nice. Yeah, after reading the message you linked to I understood the problem, and I think it should be fixable inside byebug. I'll prioritize this.

@deivid-rodriguez
Copy link
Owner

deivid-rodriguez commented Jun 6, 2019

Hei @fxn!

So I had a closer look at this yesterday and managed to narrow down the problem. In principle, I think the solution belongs in ruby-core 😞.

I think the reason why this doesn't work is the same reason why the following script

TracePoint.trace(:class) do |_tp|
  puts "CLASS"
end

# trigger a class event
binding.eval("class Hola; end")

TracePoint.trace(:line) do |tp|
  puts "LINE"

  # trigger a class event, ignored :(
  tp.binding.eval("class Adios; end")
end

# trigger a line event
1 + 1

prints

CLASS 
LINE

instead of

CLASS
LINE
CLASS

The problem is that when we are in the debugger prompt, we're actually in the middle of a TracePoint event handler (normally, a :line event). And the TracePoint API forbids events to be triggered during handler's code. It makes sense to do so, otherwise the previous script would loop forever, since the handler code for line events would trigger line events itself.

See https://github.com/ruby/ruby/blob/02880d1f4a9ebd1c0a807376fcb25ccd908334b4/vm_trace.c#L383-L400.

A potential solution would be to allow some level of reentrancy by keeping a stack instead of a single flag, and check that the current event type is not already in the stack. That would allow call events to be triggered from line events, but not line events to be triggered from line events, for example.

I'll bring this to ruby-core.

@zreisman
Copy link

zreisman commented Jun 9, 2020

Just wondering if there is any intention to support this? Moving project to 'break' at this point. Wondering if you've considered adopting whatever approach they are using?

@deivid-rodriguez
Copy link
Owner

Sorry, I haven't had any chance to work on this, but I'd love to support it of course! Contributions welcome!

@amfarrell
Copy link

@zreisman could you paste a snippet of the code you used to get break to work?
It would help those folks like me who've run into this problem to get debugging to work again.

@eregon
Copy link
Contributor

eregon commented May 22, 2021

Link to the Ruby issue about this: https://bugs.ruby-lang.org/issues/15912

@eregon
Copy link
Contributor

eregon commented May 23, 2021

Does anyone know what's the trick that break uses to avoid the nested TracePoint problem?
It seems both byebug and break use TracePoint, so not sure what's the difference.
Also: rails/rails#42264

@fxn
Copy link
Author

fxn commented May 23, 2021

@eregon if my memory does not fail me, it has to do with running things in different fibers /cc @gsamokovarov.

@gsamokovarov
Copy link

gsamokovarov commented May 25, 2021

break's idea is to run the debugging input code in a different fiber so it can be executed a bit later and outside the current tracepoint context.

https://github.com/gsamokovarov/break/blob/950b3ba137ba65fc2935bc83cea270c887c383b7/lib/break/commands/tracepoint_command.rb#L16-L23

https://github.com/gsamokovarov/break/blob/950b3ba137ba65fc2935bc83cea270c887c383b7/lib/break/commands/tracepoint_command.rb#L41-L45

This approach has its drawbacks as well. You can't debug code in a fiber initiated by another thread. The nice thing about the approach is that it can be done in another layer of the debugger, say closer to the REPL itself rather than the actual internals.

fxn added a commit to rails/rails that referenced this issue Sep 8, 2021
ruby/debug is a new debugger that is going to ship with CRuby.

It makes sense for Rails to switch to this one because that is
where the language is heading, and because Byebug is not fully
compatible with Zeitwerk. See

    deivid-rodriguez/byebug#564

While ruby/debug has not been heavily tested with Zeitwerk,
casual usage seems to suggest it works without issues, including
explicit namespaces, which is where Byebug and Zeitwerk conflict.

Byebug is terrific, thanks a lot for all these years. ❤️
jyoun-godaddy pushed a commit to jyoun-godaddy/activestorage that referenced this issue Jul 5, 2022
ruby/debug is a new debugger that is going to ship with CRuby.

It makes sense for Rails to switch to this one because that is
where the language is heading, and because Byebug is not fully
compatible with Zeitwerk. See

    deivid-rodriguez/byebug#564

While ruby/debug has not been heavily tested with Zeitwerk,
casual usage seems to suggest it works without issues, including
explicit namespaces, which is where Byebug and Zeitwerk conflict.

Byebug is terrific, thanks a lot for all these years. ❤️
@d4rky-pl
Copy link

d4rky-pl commented Jul 5, 2023

Seems that TracePoint.allow_reentry is now a thing, would that make solving this issue possible? @deivid-rodriguez

@brasic
Copy link

brasic commented Jul 5, 2023

@d4rky-pl as far as I’m aware, yes. the same issue affected the debug gem and was fixed by the use of TracePoint.allow_reentry. See ruby/debug#408 and https://bugs.ruby-lang.org/issues/15912.

@d4rky-pl
Copy link

d4rky-pl commented Jul 7, 2023

@brasic unfortunately the TracePoint part in byebug is implemented in C which is way above my pay grade so I can't contribute 🥲

Here's hoping someone will be interested in picking this up. I really love byebug and pry and can't imagine working without them. It's probably one of the most important libraries in my developer career 😅

@marshall-lee
Copy link

marshall-lee commented Oct 11, 2023

Hey guys I came up with a solution. Your reviews & testing are welcome #847

@deivid-rodriguez please rake docker:build_all from my branch to update container images for testing. I added Ruby 3 boxes and bumped the ruby versions in 2.x containers.

hackartisan added a commit to pulibrary/figgy that referenced this issue Nov 27, 2023
I kept hitting this undefined constant error, see
deivid-rodriguez/byebug#564

to use debug insert `binding.break`
hackartisan added a commit to pulibrary/figgy that referenced this issue Nov 27, 2023
I kept hitting this undefined constant error, see
deivid-rodriguez/byebug#564

to use debug insert `binding.break`
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 a pull request may close this issue.

9 participants