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

VS Code breakpoints skipped after Rails reloads related code #870

Closed
st0012 opened this issue Dec 16, 2022 · 6 comments · Fixed by #944
Closed

VS Code breakpoints skipped after Rails reloads related code #870

st0012 opened this issue Dec 16, 2022 · 6 comments · Fixed by #944
Labels
bug Something isn't working

Comments

@st0012
Copy link
Member

st0012 commented Dec 16, 2022

Your environment

  • ruby -v: 3.1.2
  • rdbg -v: 1.7.0

Describe the bug

Breakpoints added via VS Code seems to be lost after surrounding code is modified and reloaded by Rails.

For example, if we set a breakpoint at

def index
  @posts = Post.all # set the breakpoint at this line
end

And then we insert a new line to the method:

def index
  puts "foo"
  @posts = Post.all # VS Code would move the breakpoint here after update
end

The breakpoint would not be triggered anymore.

To Reproduce

  1. Clone this repo.

  2. Run $ bundle install.

  3. Run $ bundle exec bin/rails db:migrate RAILS_ENV=development.

  4. Run $ bundle exec rdbg -O -n -c -- bundle exec bin/rails s to run the server with debugger in server mode.

  5. Press F5 to attach VS Code to the debugger.

  6. Go to app/controllers/posts_controller.rb and place an VS Code breakpoint at line 6.

    def index
      @posts = Post.all # set the breakpoint at this line
    end
  7. Verify the breakpoint can be hit:

    1. In browser, visit http://localhost:3000.
    2. See the breakpoint being hit.
    3. Press F5 again to continue the program.
  8. Add puts "foo" at app/controllers/posts_controller.rb:6

    def index
      puts "foo"
      @posts = Post.all
    end
  9. Refresh the page in browser.

Expected behavior

VS Code breakpoints can still trigger after the file is updated.

Additional context

  • When updating other methods, like adding something to the new method (10 lines away), the breakpoint still works. So I suspect it's related to the breakpoint's iseq being outdated?

  • From DAP server logs, it looks like the debugger does receive updated setBreakpoints request:

#25986:[>] {"command":"setBreakpoints","arguments":{"source":{"path":"/Users/hung-wulo/src/github.com/st0012/debug-issue/app/controllers/posts_controller.rb","name":"posts_controller.rb","sourceReference":0},"lines":[6],"breakpoints":[{"line":6}],"sourceModified":true},"type":"request","seq":13}
#25986:[<] {"type":"response","command":"setBreakpoints","request_seq":13,"success":true,"message":"Success","body":{"breakpoints":[{"verified":true}]},"seq":977}

But the breakpoint wasn't actually added/activated.

@st0012
Copy link
Member Author

st0012 commented Dec 16, 2022

I think it's related to LineBreakpoint's @pending state:

After the first breakpoint hit, the later breakpoint would has @iseq like

<RubyVM::InstructionSequence:index@/Users/hung-wulo/src/github.com/st0012/debug-issue/app/controllers/posts_controller.rb:5>

And this means its @pending state would be false.

So when the :script_compiled event is triggered

  1. bps_pending_until_load? would also be false
  2. Thus ThreadClient.current.on_load wouldn't be triggered for the updated code
  3. Which could have reloaded related LineBreakpoints

But I'm not sure which part is the root cause of the issue:

  • Should the later breakpoint's @iseq still be nil?
  • Should bps_pending_until_load? also take the bps' file path into consideration?
  • Or it could be something else?

Workaround

As a quick workaround, changing bps_pending_until_load? to just do @bps.any? seems to work.

st0012 added a commit to st0012/debug that referenced this issue Dec 21, 2022
@ko1 ko1 added the bug Something isn't working label Dec 22, 2022
st0012 added a commit to st0012/debug that referenced this issue Dec 22, 2022
st0012 added a commit to st0012/debug that referenced this issue Jan 10, 2023
@crobbo
Copy link

crobbo commented Jan 13, 2023

Nothing to add other than to comment I have also been experiencing this bug. For the breakpoints to start registering after changing the code I have restart the server and attach the debug session again.

A fix for this would be great!

@ko1
Copy link
Collaborator

ko1 commented Mar 25, 2023

# repro code
File.write("l.rb", <<RUBY)
  # l.rb
  def foo # 2
    p 1   # 3
    p 2
  end
RUBY
load 'l.rb'
foo

debugger do: 'b l.rb:3'

p :loeaded

File.write("l.rb", <<RUBY)
  # l.rb
  def foo
    p 0
    p 1
    p 2
  end
RUBY

load 'l.rb'

foo

@ko1
Copy link
Collaborator

ko1 commented Mar 25, 2023

Sorry it is not a correct repro-code.
It needs VSCode interaction.

@ko1
Copy link
Collaborator

ko1 commented Mar 25, 2023

All right, I repro it.

File.write("l.rb", <<RUBY)
  # l.rb
  def foo
    p 1
  end
RUBY

load 'l.rb'
p :loeaded
debugger # set braekpoint at "l.rb:3" here

File.write("l.rb", <<RUBY)
  def foo
    p 0
    p 1
  end
RUBY

load 'l.rb'

foo

and next foo doesn't stop.

@ko1
Copy link
Collaborator

ko1 commented Mar 25, 2023

File.write("l.rb", <<RUBY)
  def foo
    p 1
  end
RUBY

load 'l.rb'
p :loeaded

debugger do: 'b l.rb:2'

foo

File.write("l.rb", <<RUBY)
  def foo
    p 0
    p 1
  end
RUBY

load 'l.rb'

foo

This is repro-code.

ko1 added a commit that referenced this issue Mar 25, 2023
Breakpoints should be remained on reloaded files. To make sure
maintaining loaded file names.

fix #870
@ko1 ko1 closed this as completed in #944 Mar 25, 2023
ko1 added a commit that referenced this issue Mar 25, 2023
Breakpoints should be remained on reloaded files. To make sure
maintaining loaded file names.

fix #870
st0012 added a commit to Shopify/debug that referenced this issue Mar 30, 2023
* DAP: use Object#__send__ to avoid name conflicts

* DAP: allow custom request extension in Session class

* Fix bug that "trace line" does not work after stopping trace once

* Increase timeout in debug_code

* Fix warning about unused variable

* Avoid memberless Struct

Related to Ruby bug 19416.

* Remove redundant raw detach test

The test's subject is the `disconnect` request with `restart: false,
terminateDebuggee: <true|false>` arguments, which has been covered by the
tests in `disconnect_dap_test.rb`. So this test case has become obsolete.

Co-authored-by: Andy Waite <[email protected]>

* Always propagate keyword arguments in WebSocketServer

* Fix `::Process::daemon` patch

As discussed here:
ruby@699a31e#r94469355

* Add test

* Use local variables

* Add test for info consts with an expression

Co-authored-by: Andy Waite <[email protected]>

* Fix typo in README

* ⬆️ Update ci ruby versions

* ⏪ Revert Ruby 2.6

* ✏️ Fix typo

* CDP: Support evaluating expression on non-current frame

* CDP: support reattaching in Chrome DevTools

Close ruby#800

* Improved stability for chrome debugging

- Display the greeting message regardless of the status of invocation of
  chrome.

  This allows coming back to the debugger on a new tab when the window
  process by `UI_CDP.run_new_chrome` is killed.

- Handle `Errno::ESRCH` in `UI_CDP.cleanup_reader`.

  When the process by `UI_CDP.run_new_chrome` is killed, re-killing it
  breaks your debugging session and in turn the "debugee".

* Make `UI_ServerBase#puts` to behave like `STDERR#puts`

`UI_ServerBase#greeting` calls `#puts` with no arguments, this
breaks reconnections to the debugger.

* Handle not existing $FILENAME in `Session#process_protocol_request`

`Session#process_protocol_request` gracefully handles `Errno::ENOENT`
when `eval`-ing `$FILENAME`, and it doens't exist.

* free terminated ThreadClients

fix ruby#899

* DAP: use Mutex when sending response

When debug.gem tries to send response from multiple threads, the socket connection is closed. I confirmed this bug when using custom request from vscode-rdbg

* Make sure to fail when remote debuggee does not exit after scenarios

Because Errno::EPIPE is rescued while sending message to socket, protocol_test_case_test.rb does not pass. protocol_test_case_test.rb had been passed because ReaderThreadError was occurred and the debuggee process was still alive. Here is a scenario. After closing socket, terminated event was sent. However socket was closed, so debuggee process raised Errno::EPIPE and debugggee process was still alive. The test framework detected the status and failed.

Thus I fixed so that the test framework does not kill the debuggee process unexpectedly.

* Alias Session send methods in WebSocketServer 

Methods ``respond``, ``respond_fail`` and ``fire_event`` can be aliased to ``send_response``, ``send_fail_response`` and ``send_event``, respectively.

* Fix timing bug on session_server creation

`@session_server` should be assigned at first.

`@session_server = Thread.current` in the session thread does not
work because the creator thread can access to `@session_server`
before it.

* Fix useless assignments

  Found by running `rubocop --only=Lint/UselessAssignment`

* Remove Tracer#puts as it's not used

* Avoid raising debuggee not finished error if assertions failed

When assertions failed, we should let the AssertionFailedError surface
and not calling flunk so we can get correct failure messages.

* Fix test builder

* DAP: allow custom request extension in ThreadClient class

* DAP: introduce Rdbg Record Inspector

* Revert "DAP: introduce Rdbg Record Inspector"

This reverts commit e29faba.

* relax authority check

to pass on the Windows.
fix ruby/vscode-rdbg#169

* Add test for global variable support in DAP and CDP

* Add test for instance variable ordering in DAP

PR ruby#806 sorts instance variables by name before returning them. This
commit adds a test that verifies this functionality under the DAP
protocol.

* Fix incorrect method name

* dap/cdp_result -> protocol_result

From ThreadClient it retunrs :dap_result or :cdp_result to the SESSION.
This patch renames it to `:protocol_result` and it will be handled
by `process_protocol_result`.

* separate 'test_console' and 'test_test'

* `test_all` should also run `test_test`

* Omit slow tests for healty CI

* add workflow

* DAP: rename the method name of custom request

Follow up for ruby#939

Change the name from "request_..." to "custom_dap_request_" in UI_DAP and ThreadClient for consistency

* DAP: support custom request in session class

UI_DAP -> Session: custom_dap_request_...

Session -> ThreadClient: custom_dap_request_event_...

Add "request_event" prefix to clarify it is a response (not Events in DAP)

* Add test for DAP's command execution through evaluate request

* Enqueue DAP's evaluate command right away

* DAP: echo back the given command

on `, debug_command` form.

* remain breakpoints on reloaded files

Breakpoints should be remained on reloaded files. To make sure
maintaining loaded file names.

fix ruby#870

* Use better approach to signal remote process' exit status

1. `remote_info.failed_process` stores symbol but we only use the value's
   presence to check if the process is force-killed.
2. The force-killed status is directly controlled by `kill_safely` through
    `kill_remote_debuggee`, which is directly called right before we check
    the status with `remote_info.failed_process`.

Combining the two, we can just let `kill_safely` and `kill_remote_debuggee` to
return the force-killed status and not storing it in `remote_info`, which
already contains a bunch of information.

This also eliminates the need to pass `test_info` to `kill_safely`, which
makes related code easier to understand and maintain.

* Remove unused failed_process attribute

* Add "result" as an argument to custom_dap_request_event method

The generated result in ThreadClient is passed as "result". We usually use it when returning responses to VS Code in Session class

* restart all threads on eval

When a thread keeps a lock, and REPL runs a code which needs the
lock, other threads should make a progress to release the lock.

fix ruby#877

* CDP: support remote debugging in different environment

From investigation by @ko1-san, the path to set breakpoints seems to be sent in "url" field when debugging in different environment such as WSL(running debuggee) and Windows(running Chrome DevTools). I supported "url" field in this PR.

* `-v` prints version and do something

Without this patch, `rdbg -v target.rb` prints version and terminates
the process. With this pach, `-v` prints version and starts debugging
for `target.rb`.

If no filename is given, terminates the process.

* v1.7.2

* Revert "Workaround VS Code's breakpoint skipping issue"

This reverts commit 4313d91.

* Revert "Avoid locking all threads on debugger suspension"

This reverts commit 1e0f45c.

* Revert "Always insert preset commands when executing commands from DAP (#4)"

This reverts commit 8eea4c9.

---------

Co-authored-by: Naoto Ono <[email protected]>
Co-authored-by: Takashi Kokubun <[email protected]>
Co-authored-by: Andy Waite <[email protected]>
Co-authored-by: Jeremy Evans <[email protected]>
Co-authored-by: Andy Waite <[email protected]>
Co-authored-by: Mau Magnaguagno <[email protected]>
Co-authored-by: Vinicius Stock <[email protected]>
Co-authored-by: yamashush <[email protected]>
Co-authored-by: Jose D. Gomez R <[email protected]>
Co-authored-by: Koichi Sasada <[email protected]>
Co-authored-by: Emily Samp <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

Successfully merging a pull request may close this issue.

3 participants