-
Notifications
You must be signed in to change notification settings - Fork 127
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
Fix compatibility with Fiber Scheduler. #987
Conversation
fe573fd
to
34b7e16
Compare
The patch is mentioned in the following issue: ruby#987 (comment)
The patch is mentioned in the following issue: ruby#987 (comment)
Hello! First, thank you @ioquatix for this patch. We are running ruby 3.1 and encountered the bug mentioned in #486. Since this patch is for ruby 3.2+, I went ahead an applied your suggestion to make it work with Ruby 3.1. Here's the branch, rebased on master and with the patch you mentioned. It fixed our problem 🎉 . I'd be happy to help get this in the debug gem itself. What would be the way forward to make this happen? @ko1, I'd be happy to help in any way I can to get this merged 🚀 . |
Seems like I spoke too soon 🙃. @ioquatix using |
@louim can you try with Ruby 3.1.3? There were some bug fixes which I hope were backported, it might be responsible. |
@ioquatix sorry, I didn’t mention it, but we are in fact using ruby 3.1.3. |
d403e22
to
a1375fc
Compare
@louim Can you test with this? if ::Fiber.respond_to?(:blocking)
private def fiber_blocking
::Fiber.blocking{yield}
end
else
private def fiber_blocking
::Fiber.new(blocking: true){yield}.resume
end
end |
1322696
to
951ffe3
Compare
With the latest change, the test failures on < 3.2 are stack trace related, as we can't control the stack when using |
951ffe3
to
754ade2
Compare
@@ -63,7 +63,7 @@ def connect_to_cdp_server | |||
MSG | |||
end | |||
|
|||
TIMEOUT_SEC = (ENV['RUBY_DEBUG_TIMEOUT_SEC'] || 10).to_i | |||
TIMEOUT_SEC = (ENV['RUBY_DEBUG_TIMEOUT_SEC'] || 30).to_i |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do you extend the timeout?
this extension will introduce huge slowdown on some cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found the test runner was timing out on GitHub actions even in HEAD.
This reverts commit 6716762. #987 (comment)
[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [debug](https://togithub.com/ruby/debug) | `1.8.0` -> `1.9.0` | [![age](https://developer.mend.io/api/mc/badges/age/rubygems/debug/1.9.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/rubygems/debug/1.9.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/rubygems/debug/1.8.0/1.9.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/rubygems/debug/1.8.0/1.9.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>ruby/debug (debug)</summary> ### [`v1.9.0`](https://togithub.com/ruby/debug/releases/tag/v1.9.0) [Compare Source](https://togithub.com/ruby/debug/compare/v1.8.0...v1.9.0) #### What's Changed - Configuration - `session_name` config by [@​ko1](https://togithub.com/ko1) in [https://github.com/ruby/debug/pull/1036](https://togithub.com/ruby/debug/pull/1036) - CLI - Require Reline 0.3.8+ to avoid frozen issue by [@​st0012](https://togithub.com/st0012) in [https://github.com/ruby/debug/pull/1020](https://togithub.com/ruby/debug/pull/1020) - Fix prompt list size and colorized code line size to match input line size passed to Reline by [@​tompng](https://togithub.com/tompng) in [https://github.com/ruby/debug/pull/1010](https://togithub.com/ruby/debug/pull/1010) - Fix broken command history when executing debugger on irb by [@​takatea](https://togithub.com/takatea) in [https://github.com/ruby/debug/pull/997](https://togithub.com/ruby/debug/pull/997) - Drop patch for Reline 0.2.7 by [@​st0012](https://togithub.com/st0012) in [https://github.com/ruby/debug/pull/1022](https://togithub.com/ruby/debug/pull/1022) - Support IRB console by [@​st0012](https://togithub.com/st0012) in [https://github.com/ruby/debug/pull/1024](https://togithub.com/ruby/debug/pull/1024) - Remote - Allow TracePoint reentry during DAP's evaluation by [@​st0012](https://togithub.com/st0012) in [https://github.com/ruby/debug/pull/1026](https://togithub.com/ruby/debug/pull/1026) - CDP: Add debuggerId field in the RETURN OBJECT of "Debugger.enable" by [@​ono-max](https://togithub.com/ono-max) in [https://github.com/ruby/debug/pull/1028](https://togithub.com/ruby/debug/pull/1028) - CDP: disable JavaScript engine based autocompletion by [@​ono-max](https://togithub.com/ono-max) in [https://github.com/ruby/debug/pull/1029](https://togithub.com/ruby/debug/pull/1029) - Do not use HEAD request if 1 port by [@​ko1](https://togithub.com/ko1) in [https://github.com/ruby/debug/pull/1035](https://togithub.com/ruby/debug/pull/1035) - Show session_name on connection by [@​ko1](https://togithub.com/ko1) in [https://github.com/ruby/debug/pull/1037](https://togithub.com/ruby/debug/pull/1037) - Internal - Stop assuming Integer#times is written in C by [@​k0kubun](https://togithub.com/k0kubun) in [https://github.com/ruby/debug/pull/1015](https://togithub.com/ruby/debug/pull/1015) - Disable cloned breakpoints trace point events by [@​vinistock](https://togithub.com/vinistock) in [https://github.com/ruby/debug/pull/1008](https://togithub.com/ruby/debug/pull/1008) - Unfreeze threads for some object-evaluating commands by [@​st0012](https://togithub.com/st0012) in [https://github.com/ruby/debug/pull/1030](https://togithub.com/ruby/debug/pull/1030) - Prevent backtrace from hanging if objects in the backtrace use Thread in inspect by [@​vinistock](https://togithub.com/vinistock) in [https://github.com/ruby/debug/pull/1038](https://togithub.com/ruby/debug/pull/1038) - Compile iseq_collector.c logic only on CRuby by [@​eregon](https://togithub.com/eregon) in [https://github.com/ruby/debug/pull/1039](https://togithub.com/ruby/debug/pull/1039) - Fix compatibility with Fiber Scheduler. by [@​ioquatix](https://togithub.com/ioquatix) in [https://github.com/ruby/debug/pull/987](https://togithub.com/ruby/debug/pull/987) - Do not make a Fiber for commands by [@​ko1](https://togithub.com/ko1) in [https://github.com/ruby/debug/pull/1044](https://togithub.com/ruby/debug/pull/1044) - support Ruby 3.3 by [@​ko1](https://togithub.com/ko1) in [https://github.com/ruby/debug/pull/1045](https://togithub.com/ruby/debug/pull/1045) - Misc/Doc - Fix ruby warnings by [@​y-yagi](https://togithub.com/y-yagi) in [https://github.com/ruby/debug/pull/993](https://togithub.com/ruby/debug/pull/993) - Fix a typo by [@​makenowjust](https://togithub.com/makenowjust) in [https://github.com/ruby/debug/pull/988](https://togithub.com/ruby/debug/pull/988) - Update `TrapInterceptor` to avoid assigning to an unused variable by [@​DavidZivk](https://togithub.com/DavidZivk) in [https://github.com/ruby/debug/pull/985](https://togithub.com/ruby/debug/pull/985) - remove debug print by [@​ko1](https://togithub.com/ko1) in [https://github.com/ruby/debug/pull/1043](https://togithub.com/ruby/debug/pull/1043) - Minor punctuation and grammar fixes by [@​ahangarha](https://togithub.com/ahangarha) in [https://github.com/ruby/debug/pull/1041](https://togithub.com/ruby/debug/pull/1041) - Tests - Bump actions/checkout from 3 to 4 by [@​dependabot](https://togithub.com/dependabot) in [https://github.com/ruby/debug/pull/1014](https://togithub.com/ruby/debug/pull/1014) #### New Contributors - [@​dependabot](https://togithub.com/dependabot) made their first contribution in [https://github.com/ruby/debug/pull/1014](https://togithub.com/ruby/debug/pull/1014) - [@​tompng](https://togithub.com/tompng) made their first contribution in [https://github.com/ruby/debug/pull/1010](https://togithub.com/ruby/debug/pull/1010) - [@​takatea](https://togithub.com/takatea) made their first contribution in [https://github.com/ruby/debug/pull/997](https://togithub.com/ruby/debug/pull/997) - [@​y-yagi](https://togithub.com/y-yagi) made their first contribution in [https://github.com/ruby/debug/pull/993](https://togithub.com/ruby/debug/pull/993) - [@​DavidZivk](https://togithub.com/DavidZivk) made their first contribution in [https://github.com/ruby/debug/pull/985](https://togithub.com/ruby/debug/pull/985) - [@​eregon](https://togithub.com/eregon) made their first contribution in [https://github.com/ruby/debug/pull/1039](https://togithub.com/ruby/debug/pull/1039) - [@​ahangarha](https://togithub.com/ahangarha) made their first contribution in [https://github.com/ruby/debug/pull/1041](https://togithub.com/ruby/debug/pull/1041) - [@​ioquatix](https://togithub.com/ioquatix) made their first contribution in [https://github.com/ruby/debug/pull/987](https://togithub.com/ruby/debug/pull/987) **Full Changelog**: ruby/debug@v1.8.0...v1.9.0 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/powerhome/audiences). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy44Ny4yIiwidXBkYXRlZEluVmVyIjoiMzcuODcuMiIsInRhcmdldEJyYW5jaCI6Im1haW4ifQ==--> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
This is a first pass at improving the compatibility with the fiber scheduler and contains two changes:
ThreadClient
is actually per-thread and not per-fiber.It only works on 3.2 but could be made to work on 3.0 and 3.1 if some tests were changed.
Fiber.blocking{...}
was introduced in Ruby 3.2. It can be emulated byFiber.new(blocking: true) {...}
but the backtrace is changed. This causes some test failures.My personal opinion is that we can ignore 3.0 and 3.1 but there is an option to support them with the compatibility wrapper outlined above, if we are willing to change some of the tests that check backtraces.
I cannot see any other obvious issues with the
debug
code. If there are other places where thedebug
code is executing blocking operations, we should add moreFiber.blocking{...}
to avoid entering the fiber scheduler.