posit-dev / positron

Positron, a next-generation data science IDE
Other
2.36k stars 69 forks source link

Comm messages emitted on open are ignored by the front end (e.g. `server_started`) #1611

Open jmcphers opened 11 months ago

jmcphers commented 11 months ago

To reproduce, just start Positron and look in the Developer Tools console. You'll see these warnings and many like them:

 WARN Client instance 'positron-lsp-python-2-c2057dd6' not found; dropping message: 

The messages look like this (line breaks added for clarity):

{
"event_clock":14,
"id":"e3777dcb-ffb54d609453e67fe36b36db_34366_61",
"parent_id":"0dcd4a4a-45e2-422c-ab2d-f826fd49db9d",
"when":"2023-10-17T15:40:22.834695Z",
"type":"comm_data",
"comm_id":"positron-lsp-python-2-c2057dd6",
"data":{"msg_type":"server_started","content":{}}
}

This happens because we don't add the comm (client instance) on the Positron side until the createClient promise has resolved. However, on the Python/R side, the comm sends this message immediately after it is opened, and that message reaches Positron before the client instance is finished opening.

There are a bunch of ways we could solve this:

This warning, while annoying, doesn't result in user-facing bugs. However, it shouldn't be simply silenced since it does correctly flag a real issue: messages emitted by comms on startup don't make it to the front end.

jmcphers commented 6 months ago

We should fix this for RC since otherwise a lot of people will think these "errors" are the source of problems they're seeing.

lionel- commented 6 months ago

I think the problem is that the LSP and DAP comms are created from the positron-r extension and the main thread is never notified of this creation. This is in contrast to:

To verify this, drop into browser() and step with the controls. You'll see drop warnings for DAP comm messages like start_debug or execute, even though the DAP comm is fully initialized and the messages are correctly handled by the R extension.

Here are the solutions I can think of:

  1. Notify the main thread when runtime.createClient() is called from an extension. This may be a bit awkward when that method is called from the main thread.

  2. Add a type of comms that are not exposed as clients to the main thread. In principle it makes sense that the main thread does not need to be aware of comms fully managed by a language pack. If I'm not mistaken we do not need the Lamport clock in this case since the communication with the backend is confined to the extension host.

    Perhaps we could have a list of client types known to be managed by the main thread and everything else would be treated as unmanaged.

  3. Silently ignore messages emitted for unknown clients. This is easiest but we could be missing out on useful warnings when a message has been received for a client supposed to be managed by the main thread that doesn't exist.

I'm currently leaning towards (2). WDYT @jmcphers?

(I was also wondering if we could avoid wrapping the LSP and DAP in comms altogether. It should be possible for the LSP as it currently does not use the comm once started. LSP has good support for custom requests and events over the TCP connection and this is a better fit than Jupyter comms because we need those messages to be concurrent with R (whereas comm messages from backend to frontend are queued on Shell and only treated when R is idle). The situation is different for the DAP which relies on Jupyter comm messages to send events. DAP doesn't have good support for custom requests and events so there is no easy way to change that.)