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

Add a DAP server to Ark #79

Merged
merged 49 commits into from
Sep 6, 2023
Merged

Add a DAP server to Ark #79

merged 49 commits into from
Sep 6, 2023

Conversation

lionel-
Copy link
Contributor

@lionel- lionel- commented Aug 23, 2023

This PR adds a comm that wraps a DAP server.

Dependencies

Event flow

  • On startup the positron-r extension requests Ark to create a DAP server comm. The Shell socket thread then starts the comm via the ServerHandler::start() method (previously called LspHandler::start() and now shared for the LSP and DAP). The DAP implementation of this method creates the ark-dap thread that manages the TCP connection and runs the DAP server when connected.

    This is currently the only way to create this thread but in the future we could provide other ways to connect to the DAP without a Jupyter comm.

  • The ark-dap thread accepts connections in a loop. When a new connection is accepted, it creates an ark-dap-events thread that listens to backend events and a new server object. The thread then serves the client until the connection is disconnected, at which point ark-dap-events is terminated, the server is dropped, and we loop back into waiting the next connection.

  • The ReadConsole() method of the R thread interacts with the frontend and the DAP thread by calling Dap::start_debug() and Dap::stop_debug() when it detects we've entered or left a browser() prompt. These send Stopped and Terminated events respectively. A Continued event is also emitted when a resume command is sent to R (n, f, etc).

  • When start_debug() is called for the first time, we send a start_debug message to the frontend via the DAP comm. The frontend then starts a debug session and connects via TCP to the client. This is what makes the experience smooth for the user. We start the debug session automatically for them.

    If not called for the first time, start_debug() sends a Stopped event to the DAP client via the TCP connection, if we are connected.

  • stop_debug() sends a Terminated DAP event if connected to a frontend. This causes the client to disconnect and terminate the debug session. If not connected to a frontend (currently untested), we just remain connected. A Continued event has already been sent to the client after detecting a resume command like n.

Concurrency

Dap is owned by the main R thread and by the DAP server thread (also by the Shell socket thread via DapHandler but only at startup).

Channels:

  • To the ReadConsole() method via the R Request channel. This is used to send commands like n or Q when Ark is not connected to the Positron frontend.

  • From the ReadConsole() method via a DapBackendEvent channel. This is used to let the DAP and the frontend know that we've entered or leaved a debugging state.

  • To the Positron frontend via the comm_tx channel. This is used to update the state of the frontend and to ask it to perform commands like executing n or Q on our behalf.

State:

  • is_connected: Whether the ark-dap thread is connected to a DAP client. When this is true, the event channel is set (a new one is created for each new DAP connection). Otherwise it is None.

  • is_debugging: Whether the REPL is in a browser() context or not.

Copy link
Contributor

@DavisVaughan DavisVaughan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have done a partial read through, I'll read through more tomorrow and do some actual testing then too

crates/amalthea/src/language/lsp_handler.rs Outdated Show resolved Hide resolved
crates/amalthea/src/comm/server_comm.rs Outdated Show resolved Hide resolved
crates/amalthea/src/socket/shell.rs Outdated Show resolved Hide resolved
Comment on lines +91 to +94
let msg = CommChannelMsg::Data(json!({
"msg_type": "start_debug",
"content": {}
}));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe worth creating a formal comm message type for this?

crates/ark/src/dap/dap.rs Show resolved Hide resolved
crates/ark/src/dap/dap.rs Outdated Show resolved Hide resolved
crates/ark/src/dap/dap.rs Outdated Show resolved Hide resolved
self.is_debugging = false;

if self.is_connected {
if let Some(_) = &self.comm_tx {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't quite tell why we need this extra if let, can you help me understand that part?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is in case we add support for connections outside of a Jupyter comm.

crates/ark/src/dap/dap.rs Outdated Show resolved Hide resolved
@lionel- lionel- force-pushed the feature/dap branch 2 times, most recently from baa85ab to e0f173e Compare August 24, 2023 12:49
Copy link
Contributor

@DavisVaughan DavisVaughan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few more comments as I finish my read through, now going to do some actual local testing

crates/ark/src/dap/dap_server.rs Outdated Show resolved Hide resolved
crates/ark/src/interface.rs Show resolved Hide resolved
}

let line = INTEGER_ELT(srcref, 0);
let column = INTEGER_ELT(srcref, 4);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if this ever comes up in practice, but ?srcref has this to say about the length of a srcref object (i.e. could be 4 elements or shorter sometimes)

Bytes (elements 2, 4) and columns (elements 5, 6) may be different due to multibyte characters. If only four values are given, the columns and bytes are assumed to match. Lines (elements 1, 3) and parsed lines (elements 7, 8) may differ if a #line directive is used in code: the former will respect the directive, the latter will just count lines. If only 4 or 6 elements are given, the parsed lines will be assumed to match the lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting! Here is where the line directive is coming from: r-devel/r-svn@ead62cd

IIUC, this allows generated or expanded code to refer to original files, which seems neat. The right thing to do in this case is to use the directive-aware field, which we do.

Taking the column value (5th element) rather than the bytes is more likely to be correct (if encoding match). However your excerpt suggests that we might be provided with fewer elements if the bytes and columns values match. I don't think this has come up with rlang srcref handling, but to be safe I've changed the code to account for this possibility.

@DavisVaughan
Copy link
Contributor

DavisVaughan commented Aug 24, 2023

@lionel- and I have been discussing how globalCallingHandlers() aren't run while debugging. That can be mostly captured with this screenshot:

Screenshot 2023-08-24 at 11 52 32 AM

Which we think is due to
https://github.com/wch/r-source/blob/08df654faa01e9f7818da5a4214f78412caa0310/src/main/errors.c#L1883

In practice for Positron this means any errors seen while debugging seem to get "swallowed" because our global handler that relays the message to the Console isn't run.


To be clear this doesn't seem to be the fault of this PR, so it isn't a blocker. Just documenting this somewhere for now.

@lionel- lionel- force-pushed the feature/dap branch 2 times, most recently from f8caf81 to d024c45 Compare September 5, 2023 08:51
@lionel-
Copy link
Contributor Author

lionel- commented Sep 5, 2023

To work around the issue discovered by Davis, we're now turning off the global handling while in the debugger. During this time, error messages are sent to stderr rather than published to positron. The usual setup is restored when we detect a non-browser prompt.

@lionel-
Copy link
Contributor Author

lionel- commented Sep 5, 2023

Should we merge this for private alpha @jmcphers?

@lionel-
Copy link
Contributor Author

lionel- commented Sep 6, 2023

I've filed the global handling within debugger issue at https://bugs.r-project.org/show_bug.cgi?id=18590, with a proposed patch.

@lionel- lionel- merged commit 2aaebaf into main Sep 6, 2023
1 check passed
@lionel- lionel- deleted the feature/dap branch September 6, 2023 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants