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

Feature Request: Push debugger activation to first breakpoint hit instead of require "debug" #797

Open
st0012 opened this issue Nov 9, 2022 · 18 comments

Comments

@st0012
Copy link
Member

st0012 commented Nov 9, 2022

Proposal

  • require "debug" won't activate the debugger. Debugger will only be activated when the first breakpoint is triggered.
    • require "debug" will be the same as require "debug/prelude"
  • require "debug/open" and require "debug/open_nonstop"'s behaviour will remain the same.

Why

Side-effects

In apps that have Bundler.require, like most Rails apps, having gem "debug" in Gemfile means the debug gem is always activated when the app is booted.

However, with the debugger's activation, many things will happen in the background:

  • Additional TracePoints would be enabled (example)
  • A new thread will be spawned
  • Additional computation and object allocation that come with the above

To most apps, having them in the background doesn't make a huge difference. But to some, having the debugger activated causes tests to fail.

For example, I tested 5 mid-to-large Rails apps in Shopify, and 3 of them have test failures that only happen when debug is activated:

  • Some tests mock File.readlines and could break when this line is triggered in the background.
  • There's also weird NoMethodError exceptions that only happens with the debugger activated.

Therefore, we always have require: false after gem "debug".

Problem Solved?

However, having require: false means users need to manually type require "debug" before start debugging with console. But in the meantime, byebug can be required by default without the same side-effects. So byebug doesn't need that manual require.

This extra require step is quite inconvenient for people considering switching from byebug to debug.

Other Smaller Issues

  • The activation and forking messages are noisy and disabling them project-by-project is not trivial.
  • require "debug" locks the UI to console, so if users want to use the remote UI by requiring debug/open after the app is booted, require: false is also needed.

Possible Change

diff --git a/lib/debug.rb b/lib/debug.rb
index 15ebccb..3489268 100644
--- a/lib/debug.rb
+++ b/lib/debug.rb
@@ -1,5 +1,3 @@
 # frozen_string_literal: true

 require_relative 'debug/session'
-return unless defined?(DEBUGGER__)
-DEBUGGER__::start no_sigint_hook: true, nonstop: true
diff --git a/lib/debug/session.rb b/lib/debug/session.rb
index a900f89..ffb2604 100644
--- a/lib/debug/session.rb
+++ b/lib/debug/session.rb
@@ -2486,7 +2486,9 @@ end

 module Kernel
   def debugger pre: nil, do: nil, up_level: 0
-    return if !defined?(::DEBUGGER__::SESSION) || !::DEBUGGER__::SESSION.active?
+    if !defined?(::DEBUGGER__::SESSION) || !::DEBUGGER__::SESSION.active?
+      DEBUGGER__::start no_sigint_hook: true, nonstop: true
+    end

     if pre || (do_expr = binding.local_variable_get(:do))
       cmds = ['binding.break', pre, do_expr]
@ko1
Copy link
Collaborator

ko1 commented Nov 17, 2022

Let me think some more time about it.

ko1 added a commit that referenced this issue Dec 2, 2022
With `RUBY_DEBUG_LAZY=1`, `require 'debug'` doesn't start a session
but start when `debugger` method is called (or `require 'debug/start'`
is called).

This feature is experimental.
#797
@ko1
Copy link
Collaborator

ko1 commented Dec 2, 2022

require "debug" won't activate the debugger. Debugger will only be activated when the first breakpoint is triggered.

Your proposal is based on that: user only specify the breakpoint with debugger method.
I agree many people do that but I don't want to remove other possibilities.

To most apps, having them in the background doesn't make a huge difference. But to some, having the debugger activated causes tests to fail.

  • In general, I want to solve these failures by fixing the debugger, not by lazy session starting. So the bug reports are welcome.
    • For example, using from IDE it is same issue.
  • Additional overhead can be an issue and I want to know how it impact on the real usecases. Please file if you find an impact.
    • I want to provide a way to connect to the app via debug port as an backdoor, so I'll provide a way more lightweight version (disable collecting the source code, and so on).
  • If we don't start the session, we couldn't support the following and I'm not sure it should be disabled by default.
      1. opening debug port with RUBY_DEBUG_OPEN.
      1. loading rdbgrc file(s).
      1. multiple forked code entering to the debug console, the input/output will be mixed.
      • it can be avoid with a trick, anyway.
      1. recording evaled code.
      • not so high priority.

BTW requiring debug/prelude doesn't start session, but define debugger (and others) so you may want to use it (but it needs to change Gemfile a bit).
Another idea is to make gem only requires debug/prelude.


I introduced RUBY_DEBUG_LAZY experimentally (so I don't write it on doc), it's require debug/prelude instead of calling start, so please try it and make it official on next version, or consider other possibilities.


The activation and forking messages are noisy and disabling them project-by-project is not trivial.

fixed on #823

require "debug" locks the UI to console, so if users want to use the remote UI by requiring debug/open after the app is booted, require: false is also needed.

I'd supported it (override by requiring debug/open after requiring debug) but it was broken.
fixed on #825

@st0012
Copy link
Member Author

st0012 commented Dec 2, 2022

Your proposal is based on that: user only specify the breakpoint with debugger method.

If users want to set breakpoints with other ways, say with break command, shouldn't rdbg work the same? 🤔

I'm not sure why lazily loading debugger will reduce the possibilities to set breakpoints. Can you elaborate more?

In general, I want to solve these failures by fixing the debugger, not by lazy session starting.

Yes I also believe that's the way to go. But I'm not sure how much the effort it'd take and how it'd be prioritised.

I think lazy-activation to reduce friction is a good first step toward that goal though. And once we made activation lightweight, we can turn it back on and users wouldn't notice a thing.

I'll provide a way more lightweight version (disable collecting the source code, and so on).

I can test if disabling the TracePoint reduce test failures in our apps.

If we don't start the session, we couldn't support the following and I'm not sure it should be disabled by default.

I think the features you listed are useful, but also seem quite specialised. In my experience using and helping my colleagues use the debugger in apps and gems, we rarely need them. I'm not saying that we represent most users, but it may be a shared experience.

So how about we do it in another way:

  • By default we lazily activate the debugger (until it became lightweight enough).
  • In the readme we tell users, if they need the listed features, they need to eagerly load the debugger by setting an environment variable like RUBY_DEBUG_ACTIVATE_AT_START=true.

The idea is, to make the debugger easily accessible to average users, we make it as lightweight as possible by default (lazy-activation is one way). And if some users want more powerful features, we also give them options to change the default behaviour. WDYT?

Another idea is to make gem only requires debug/prelude.

I think that's a good idea too 👍

fixed on #823
fixed on #825

Thank you so much for these fixes 🙏

ko1 added a commit that referenced this issue Dec 2, 2022
With `RUBY_DEBUG_LAZY=1`, `require 'debug'` doesn't start a session
but start when `debugger` method is called (or `require 'debug/start'`
is called).

This feature is experimental.
#797
@ko1
Copy link
Collaborator

ko1 commented Dec 2, 2022

I'm not sure why lazily loading debugger will reduce the possibilities to set breakpoints. Can you elaborate more?

For example, rdbgrc can set breakpoints and editors (vi and so on) can set them.

The idea is, to make the debugger easily accessible to average users, we make it as lightweight as possible by default (lazy-activation is one way). And if some users want more powerful features, we also give them options to change the default behaviour. WDYT?

I believe reducing functionality is for high-level users who can understand what they are doing.

@ko1
Copy link
Collaborator

ko1 commented Dec 2, 2022

for example: "if you want to use Regexp on Ruby, please use --regexp option" is not convenient.

@st0012
Copy link
Member Author

st0012 commented Dec 12, 2022

I believe reducing functionality is for high-level users who can understand what they are doing.

I think it really depends on what functionalities we are reducing and what are most users's usage of them.

For example, I haven't needed to use rdbgrc to set breakpoints. Because it means the breakpoint would be set to all Ruby projects I have, which varies a lot and it simply won't work. I'm not saying nobody uses this feature, but it's likely be a rare case. \

In the contrary, at Shopify we several applications, as well as gems like Tapioca, can benefit from having lazy-activation by default. (Also the app of my prev company, which's tests fail whenever debug is required).

I also want to add an update on the RUBY_DEBUG_LAZY experiment:

I experimented RUBY_DEBUG_LAZY=1 with the removal ofrequire: false on a big monolith we have. I applied it to

  • The project's local development default envs
  • The project's cloud development default envs

And the initial CI seemed to run fine, so we merged the PR.

But it turned out it randomly hanged some of the CI suites from time to time. And it took us a few days to pin down to my change. We don't exactly know what code was causing that, but it's loading the debugger that caused it. So we now added require: false back.

It's possible that we can add RUBY_DEBUG_LAZY=1 to the project's CI setup too. But that project has a dozen of CI suites. If I missed anyone of them, we'll have a flaky CI again.

What I want to say is, we're not requesting any advanced features of the debugger. We simply want to add gem "debug" to the Gemfile without require: false and let users explore it. But the current activation behaviour is causing a lot of issues just to do that.

The RUBY_DEBUG_LAZY experiment is helping, which we appreciate. But it'd still be a lot better if we can make it the default. And if the cost is some advanced and potentially rarely used features would require a flag to work, I think it's a reasonable tradeoff.

@ko1
Copy link
Collaborator

ko1 commented Dec 22, 2022

But it turned out it randomly hanged some of the CI suites from time to time. And it took us a few days to pin down to my change. We don't exactly know what code was causing that, but it's loading the debugger that caused it. So we now added require: false back.

So RUBY_DEBUG_LAZY also have an issue you couldn't figure out, right?

@st0012
Copy link
Member Author

st0012 commented Dec 22, 2022

So RUBY_DEBUG_LAZY also have an issue you couldn't figure out, right?

Sorry I wasn't clear on this: the problem was caused by some CI tests not having RUBY_DEBUG_LAZY:

  • Some test suites ran with RUBY_DEBUG_LAZY and they passed.
  • Some test suites ran without RUBY_DEBUG_LAZY and failed randomly, therefore we didn't discover at the beginning.
    • To solve this with RUBY_DEBUG_LAZY, we need to at least at it to a dozen of CI config files.

So my point is, needing to manually apply RUBY_DEBUG_LAZY in all the places that could use it could be a huge burden to users.

@ko1
Copy link
Collaborator

ko1 commented Dec 22, 2022

BTW why do you use debug.gem on CI?

@st0012
Copy link
Member Author

st0012 commented Dec 22, 2022

In Rails applications, debug gems usually will be put under the development and test group in Gemfile. This is the Gemfile current Rails generates:

group :development, :test do
  # See https://guides.rubyonrails.org/debugging_rails_applications.html#debugging-with-the-debug-gem
  gem "debug", platforms: %i[ mri mingw x64_mingw ]
end

So when running tests, the debug gem could be automatically required and be used to debug tests.
And because CI runs tests with test environment, the debug gem would be required there too.

@ko1
Copy link
Collaborator

ko1 commented Dec 23, 2022

So you mean that

  1. most of programmers don't want to debug by debug.gem, especially on test group
  2. but it is required because Rails inserts it to Gemfile by default
  3. most of programmers don't want to use any debug.gem features until it calls debugger method

So the gem "debug.gem" is not assumed that "require it when debug with it" but introduce debugger command by many people, you say.

@st0012
Copy link
Member Author

st0012 commented Jan 9, 2023

Sorry for the delay.

So the gem "debug.gem" is not assumed that "require it when debug with it" but introduce debugger command by many people, you say.

Yes that's the feedback I got from many devs, also my personal expectation.

I understand that the goal is debug will become a completely built-in tool, which users won't need to add debug in their Gemfile at all. And when that happens, require "debug" could just mean "start debugging" and calling debugger would be redundant (like the most early design of debug did, which would start the console when required).

But at the moment and in near future, debug is still better to be treated as a gem because

  • It's easier to receive bug fix/enhancement through gem releases than upgrading Ruby
  • Old Ruby version always need to receive updated debug as a gem

Therefore, making its activation behaviour similar to other gems (e.g. pry, byebug) can make users adopting it quicker and benefit from its power.

@ko1
Copy link
Collaborator

ko1 commented Mar 7, 2023

I understand that the goal is debug will become a completely built-in tool, which users won't need to add debug in their Gemfile at all.

At least I don't think such goal.

@shioyama
Copy link

shioyama commented Dec 7, 2023

Therefore, making its activation behaviour similar to other gems (e.g. pry, byebug) can make users adopting it quicker and benefit from its power.

I think if you want to make debug become the de-facto standard, this is pretty important.

@eregon
Copy link
Member

eregon commented Dec 7, 2023

Given that Bundler.require exists and is used e.g. by Rails, I think it's a fairly well-known expectation that all gems don't do heavy processing or e.g. add TracePoints while they are being loaded, as that could cause significant overhead or incompatibilities.
So +1 for this.

I guess the main use case for require "debug" to start the debugger is for ruby -rdebug some_script.rb.
Maybe that case can be detected and and so the session would be started eagerly only for that case?

When people add gem "debug" in their Gemfile, I would think none of them want that to immediately start the debugging session when they load their app.

@eregon
Copy link
Member

eregon commented Dec 7, 2023

ruby -rdebug some_script.rb

OTOH using rdbg ... for that use case seems good enough, so probably no need to treat ruby -rdebug any differently.

@peterbrandel
Copy link

Hey 👋

Real world situation where we ran into an issue with how debug is being loaded:
We were testing an app in a staging environment, introducing short lived background workers.
Those background workers spawn a couple of worker processes per pod and execute the jobs asynchronously.
To ensure everything was working as expected we ran a production load against the staging system, processing millions of events.
Eventually the worker pods would run out of memory every time no matter how much memory was provided to the pod (went up to 16Gi and a single worker process per pod).
After several days of scratching our heads we realized that the debug gem was included in the staging grouping in our Gemfile.
Removing the debug gem from the staging environment resolved the issue.

Attaching a graph that shows how memory usage is shooting up as soon as the service gets deployed and is starting to run.
The part where you see the everything drop in the end is when we removed debug from staging.
The zig zag pattern is caused by deploys. While the service was running memory usage never dropped until we remove the gem.

Screenshot 2024-04-30 at 8 40 01 AM

@st0012
Copy link
Member Author

st0012 commented Apr 30, 2024

I also recently compared how the eager and lazy activation options would affect Rails application's test duration on CI, by using different gem declaration in Gemfile:

  1. gem "debug" (activated by default)
  2. gem "debug", require: "debug/prelude" (lazy activation, the proposed behaviour)

I ran full test suites of 3 different Rails apps, and saw between 9.1% to 11.17% extra test duration when debug is activated by default. For this reason, we've made require: "debug/prelude" a recommendation to all Rails apps at Shopify.

I think it's understandable that having a debugger running alongside the application would increase its memory consumption and cause extra overhead. But to me it means the debugger should by default either not be activated, or avoid the activities that would introduce such negative impacts.

And during a conversation with @ko1, he suggested that the overhead I observed may be caused by this line, which my later testings seemed to confirm as the 9~11% overhead largely disappeared. But I didn't do as thorough testing due to short of time, so we may need to verify that with more testings.

Notes

  • The test duration measured only includes time the test command, like bundle exec rspec ..., takes. It doesn't include any environment setup time.

st0012 added a commit to Shopify/rails that referenced this issue Apr 30, 2024
In ruby/debug#797, we found that requiring
`debug` automatically activates it, which could introduce runtime overhead
and cause memory bloat. And I think many users aren't aware of this and
could be taxed by this unnecessarily (e.g. having longer builds on CI).

Therefore, I propose to add `require: "debug/prelude"` after `debug`'s
Gemfile entry in the default Gemfile template. This way, users can
still use breakpoint methods like `debugger`, `binding.break`, and `binding.b`,
but the debugger won't be activated until a breakpoint is hit.
st0012 added a commit to Shopify/rails that referenced this issue Apr 30, 2024
In ruby/debug#797, we found that requiring
`debug` automatically activates it, which could introduce runtime overhead
and cause memory bloat. And I think many users aren't aware of this and
could be taxed by this unnecessarily (e.g. having longer builds on CI).

Therefore, I propose to add `require: "debug/prelude"` after `debug`'s
Gemfile entry in the default Gemfile template. This way, users can
still use breakpoint methods like `debugger`, `binding.break`, and `binding.b`,
but the debugger won't be activated until a breakpoint is hit.
st0012 added a commit to Shopify/rails that referenced this issue Apr 30, 2024
In ruby/debug#797, we found that requiring
`debug` automatically activates it, which could introduce runtime overhead
and cause memory bloat. And I think many users aren't aware of this and
could be taxed by this unnecessarily (e.g. having longer builds on CI).

Therefore, I propose to add `require: "debug/prelude"` after `debug`'s
Gemfile entry in the default Gemfile template. This way, users can
still use breakpoint methods like `debugger`, `binding.break`, and `binding.b`,
but the debugger won't be activated until a breakpoint is hit.
fractaledmind pushed a commit to fractaledmind/rails that referenced this issue May 13, 2024
In ruby/debug#797, we found that requiring
`debug` automatically activates it, which could introduce runtime overhead
and cause memory bloat. And I think many users aren't aware of this and
could be taxed by this unnecessarily (e.g. having longer builds on CI).

Therefore, I propose to add `require: "debug/prelude"` after `debug`'s
Gemfile entry in the default Gemfile template. This way, users can
still use breakpoint methods like `debugger`, `binding.break`, and `binding.b`,
but the debugger won't be activated until a breakpoint is hit.
xjunior pushed a commit to xjunior/rails that referenced this issue Jun 9, 2024
In ruby/debug#797, we found that requiring
`debug` automatically activates it, which could introduce runtime overhead
and cause memory bloat. And I think many users aren't aware of this and
could be taxed by this unnecessarily (e.g. having longer builds on CI).

Therefore, I propose to add `require: "debug/prelude"` after `debug`'s
Gemfile entry in the default Gemfile template. This way, users can
still use breakpoint methods like `debugger`, `binding.break`, and `binding.b`,
but the debugger won't be activated until a breakpoint is hit.
Set2005 pushed a commit to Set2005/fix-association-initialize-order that referenced this issue Jul 8, 2024
In ruby/debug#797, we found that requiring
`debug` automatically activates it, which could introduce runtime overhead
and cause memory bloat. And I think many users aren't aware of this and
could be taxed by this unnecessarily (e.g. having longer builds on CI).

Therefore, I propose to add `require: "debug/prelude"` after `debug`'s
Gemfile entry in the default Gemfile template. This way, users can
still use breakpoint methods like `debugger`, `binding.break`, and `binding.b`,
but the debugger won't be activated until a breakpoint is hit.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

5 participants