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

Improve dap command execution #920

Merged
merged 2 commits into from
Mar 24, 2023
Merged

Conversation

st0012
Copy link
Member

@st0012 st0012 commented Mar 8, 2023

Fixes #900

Description

From investigating #900 with @andyw8, I found that the current implementation has 2 potential problems:

  1. The command sent via evaluate request (e.g. ,b 5) is actually executed AFTER the next session command (e.g. enqueued request hashes). This is because:
    1. wait_command would usually be waiting for @ui.readline when the request arrives. Therefore, the @preset_command added through debugger do: won't be picked up right away.
    2. Now, when the later request (e.g. stackTrace) comes in, it'll be picked up by @ui.readline and be executed right away.
    3. Only after the current wait_command call finishes and we enter the next one, the b 5 command would be picked up.
  2. The debugger do:'s behaviour is not deterministic, at least in the evaluate request's case. Sometimes the do command would be added through SESSION.add_preset_commands and sometimes through DEBUGGER__.add_line_breakpoint. We believe this is one of the causes of First breakpoint is skipped on a remote development environment #900.

Implementation

To address these 2 problems I think we can directly enqueue the sent command to @q_msg:

  • It solves problem 1 because the command would be picked up by @ui.readline too so the execution order would be correct. The test added in the PR covers this case.
  • It also avoids problem 2 because the behaviour won't be affected by whether we're in a subsession or not.

Questions and Concerns

  • I assume there was a reason that this feature was implemented with debugger do: in the first place, but we couldn't figure it out ourselves. @ko1 would you mind elaborating it?
  • Is it safe for wait_command to take the command from both synchronous state change (@preset_command) and asynchronously through @ui.readline?
  • I think problem 1 could also be addressed by improving wait_command. But I don't know the design decisions behind the current implementation + I think the impact radius is too big. So I didn't pursue it.

@st0012 st0012 force-pushed the improve-dap-command-execution branch from af558c3 to 34182df Compare March 9, 2023 11:04
@st0012 st0012 force-pushed the improve-dap-command-execution branch from 34182df to cacaef2 Compare March 9, 2023 11:09
@ko1
Copy link
Collaborator

ko1 commented Mar 24, 2023

All right, maybe I consider something but I couldn't remember the reason why I haven't choose direct manipulation of @q_msg.
Let's try.

@ko1 ko1 merged commit 211b319 into ruby:master Mar 24, 2023
@st0012 st0012 deleted the improve-dap-command-execution branch March 25, 2023 07:09
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.

First breakpoint is skipped on a remote development environment
3 participants