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.

Sign up for GitHub

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

Jump to bottom

introduce debug session model #80

Merged
merged 30 commits into from
Dec 26, 2024
Merged

introduce debug session model #80

merged 30 commits into from
Dec 26, 2024

Conversation

RemcoSmitsDev
Copy link
Owner

@RemcoSmitsDev RemcoSmitsDev commented Dec 22, 2024 •
edited
Loading

Intro

This pr introduces a new layer called DebugSession the debug session is responsible for holding the clients, because we can have more clients for one debug session. Some debug adapter tell us to spawn a new client that should reconnect to the current already spawned debug adapter (first spawned client). This allows us also to show clients under a debug session inside the dap log, so it's more clear for users that we did not start 2 debug session, but we spawned 2 clients.

The reason behind this change is that a user had an issue with reverse request, not always both clients are shutting down. So instead of only shutting down the client that the adapter tells us to shut down, we also shut down the main client that spawned the adapter itself.

This PR also fixes a race issue with sending breakpoints and the configuration done request. See bf293f9.

TODO's

  • Show debug sessions inside dap logs, with nested child clients
  • Make reverse requests work again
  • Rework shutdown client, so we also remove the session if there are no clients left anymore
    • Add tests to validate we can handle shutdown of multiple clients
  • Make collab capabilities work again
  • Move ignore breakpoints to debug session
  • Move sessions to local store
  • Style dap log menu, so session and clients are better visible
Screenshot 2024-12-25 at 21 42 52

@RemcoSmitsDev RemcoSmitsDev force-pushed the introduce-debug-session-model branch from 094fffc to f3808da Compare December 25, 2024 12:21
@RemcoSmitsDev RemcoSmitsDev force-pushed the introduce-debug-session-model branch from 65adba8 to 92b653a Compare December 25, 2024 19:07
@RemcoSmitsDev
Move next session/client id to local store
c92ac33
@RemcoSmitsDev
Remove duplicated test support only method
fc11c31
@RemcoSmitsDev
Show client id first in dap log menu entry
e7fcd8e
@RemcoSmitsDev
Show sub menu better
c6902c0
@RemcoSmitsDev
Sort clients by their id
637ddb3
@RemcoSmitsDev
Sort sessions by their id
8ef58c4
@RemcoSmitsDev
Fix configuration done race condition with sending breakpoints
bf293f9 
@Anthony-Eid I think this fixed the race condition you noticed with the configuration done request.

So the issue is when the task is created it directly start the execution of the task itself.
So the `configuration_done` request is most of the times finished before the breakpoints are send
if you don't have a lot of breakpoints.

So instead of creating both tasks before the task is created we now create the 2 tasks right after eachother,
so we can be sure that the `configuration_done` request is send after the breakpoints are send.

```
[2024-12-25T21:51:09+01:00 DEBUG dap::client] Client 0 received event `Initialized`
[2024-12-25T21:51:09+01:00 DEBUG dap::client] Client 0 send `setBreakpoints` request with sequence_id: 3
[2024-12-25T21:51:09+01:00 DEBUG dap::client] Client 0 send `setBreakpoints` request with sequence_id: 4
[2024-12-25T21:51:09+01:00 DEBUG dap::client] Client 0 send `configurationDone` request with sequence_id: 8
[2024-12-25T21:51:09+01:00 DEBUG dap::client] Client 0 send `setBreakpoints` request with sequence_id: 9
[2024-12-25T21:51:09+01:00 DEBUG dap::client] Client 0 send `setBreakpoints` request with sequence_id: 7
[2024-12-25T21:51:09+01:00 DEBUG dap::client] Client 0 send `setBreakpoints` request with sequence_id: 5
[2024-12-25T21:51:09+01:00 DEBUG dap::client] Client 0 received response for: `setBreakpoints` sequence_id: 3
[2024-12-25T21:51:09+01:00 DEBUG dap::client] Client 0 received response for: `setBreakpoints` sequence_id: 4
[2024-12-25T21:51:09+01:00 DEBUG dap::client] Client 0 send `setBreakpoints` request with sequence_id: 6
[2024-12-25T21:51:09+01:00 DEBUG dap::client] Client 0 received response for: `configurationDone` sequence_id: 8
[2024-12-25T21:51:09+01:00 DEBUG dap::client] Client 0 received response for: `setBreakpoints` sequence_id: 9
[2024-12-25T21:51:09+01:00 DEBUG dap::client] Client 0 received response for: `setBreakpoints` sequence_id: 7
[2024-12-25T21:51:09+01:00 DEBUG dap::client] Client 0 received response for: `setBreakpoints` sequence_id: 5
[2024-12-25T21:51:09+01:00 DEBUG dap::client] Client 0 received response for: `setBreakpoints` sequence_id: 6
```

```
[2024-12-25T21:49:51+01:00 DEBUG dap::client] Client 0 received event `Initialized`
[2024-12-25T21:49:51+01:00 DEBUG dap::client] Client 0 send `setBreakpoints` request with sequence_id: 4
[2024-12-25T21:49:51+01:00 DEBUG dap::client] Client 0 send `setBreakpoints` request with sequence_id: 5
[2024-12-25T21:49:51+01:00 DEBUG dap::client] Client 0 send `setBreakpoints` request with sequence_id: 6
[2024-12-25T21:49:51+01:00 DEBUG dap::client] Client 0 send `setBreakpoints` request with sequence_id: 7
[2024-12-25T21:49:51+01:00 DEBUG dap::client] Client 0 send `setBreakpoints` request with sequence_id: 8
[2024-12-25T21:49:51+01:00 DEBUG dap::client] Client 0 send `setBreakpoints` request with sequence_id: 3
[2024-12-25T21:49:51+01:00 DEBUG dap::client] Client 0 received response for: `setBreakpoints` sequence_id: 4
[2024-12-25T21:49:51+01:00 DEBUG dap::client] Client 0 received response for: `setBreakpoints` sequence_id: 5
[2024-12-25T21:49:51+01:00 DEBUG dap::client] Client 0 received response for: `setBreakpoints` sequence_id: 6
[2024-12-25T21:49:51+01:00 DEBUG dap::client] Client 0 received response for: `setBreakpoints` sequence_id: 7
[2024-12-25T21:49:51+01:00 DEBUG dap::client] Client 0 received response for: `setBreakpoints` sequence_id: 8
[2024-12-25T21:49:51+01:00 DEBUG dap::client] Client 0 received response for: `setBreakpoints` sequence_id: 3
[2024-12-25T21:49:51+01:00 DEBUG dap::client] Client 0 send `configurationDone` request with sequence_id: 9
[2024-12-25T21:49:51+01:00 DEBUG dap::client] Client 0 received response for: `configurationDone` sequence_id: 9
```
@RemcoSmitsDev
Move capabilities back to dapstore itself
4cea4e8
@RemcoSmitsDev RemcoSmitsDev force-pushed the introduce-debug-session-model branch from 4730d13 to 4cea4e8 Compare December 25, 2024 22:13
@RemcoSmitsDev RemcoSmitsDev marked this pull request as ready for review December 26, 2024 13:09
@RemcoSmitsDev
Notify errors when start client to user
b7a21cc
@RemcoSmitsDev
Notify reconnect error to user
601dd66
@RemcoSmitsDev
Always shutdown all clients
1f893e1 
We should always shutdown all clients  from a single debug session when one is shutdown/terminated.
@RemcoSmitsDev RemcoSmitsDev changed the title WIP introduce debug session model introduce debug session model Dec 26, 2024
@RemcoSmitsDev
Copy link
Owner Author

Hey @gaauwe, could you test if you still have the issue with the port not always being available? This PR should fix this issue, because we shut down now all clients that belong to a single debug session.

@RemcoSmitsDev RemcoSmitsDev merged commit ccacce9 into debugger Dec 26, 2024
10 checks passed
@gaauwe
Copy link

gaauwe commented Jan 6, 2025

Hi @RemcoSmitsDev! Bit later due to holidays (hope you had a great time as well!), but this seems to work really great. Played a bit around with restarting / closing processes and debugging sessions and it all seems to go well now :D

Not sure if this info is helpful; but if I omit the urlFilter in the config that attaches to an existing browser process, it will start a debug session for every webpage that is open (as expected). Stopping the debug session and starting it again after adding urlFilter now also correctly only shows the active sessions in the DAP Logs (the UI update is also very good looking and way clearer!):

image

Showing the task name in the debugger tab itself works also really great:

image

All in all great work again. Will keep playing around with it as always, but so far so good :D

@RemcoSmitsDev
Copy link
Owner Author

@gaauwe No worries, yeah I also had a great time, I also took some time to fix this issue. Great to hear it fixes your issues, and that the UI now is a bit more self explaining.

Thanks for the kind words🙂 and for testing it once again!!

@RemcoSmitsDev RemcoSmitsDev deleted the introduce-debug-session-model branch January 6, 2025 21:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@RemcoSmitsDev @gaauwe @Anthony-Eid