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

multiple console session: start and stop LSPs #6714

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

dhruvisompura
Copy link
Contributor

@dhruvisompura dhruvisompura commented Mar 10, 2025

Addresses #6310

To get LSP features working in a sensible manner for multiple console sessions we need to ensure the LSP for the active session is the only one providing diagnostics/code completion/etc.

The simplest approach to achieve the above is to only allow the LSP for the active session to be running, and shutdown all other LSPs. Positron does not know about the LSPs, these are managed by the extensions right now. To keep with this pattern, we need to let the language pack know when the active session changes so it can gracefully start/stop the LSPs for each session.

When a user changes the active session, we fire an onDidChangeForegroundSession event in Positron. By exposing this event to the extension host, the language packs can listen for changes to the active session and start/stop the LSPs.

We can notify the extension host when this event fires and the extension host can mirror the event and emit when its notified the active session changed. The mirrored event on the extension host side can be exposed in the positron API so that an extension can listen to the event. The extensions can listen to this event and start the LSP for the active session, and stop all other LSPs.

Release Notes

New Features

  • N/A

Bug Fixes

  • N/A

QA Notes

@:console @:sessions @:web @:editor

Copy link

github-actions bot commented Mar 10, 2025

E2E Tests 🚀
This PR will run tests tagged with: @:critical @:console @:sessions @:web @:editor

readme  valid tags

@dhruvisompura dhruvisompura changed the title Feature/mulitple console session diagnostics mulitple console session: diagnostics and completion Mar 11, 2025
@dhruvisompura dhruvisompura force-pushed the feature/mulitple-console-session-diagnostics branch from 64e4c46 to 30160e3 Compare March 11, 2025 19:32
Copy link
Contributor

@lionel- lionel- left a comment

Choose a reason for hiding this comment

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

I like that the event emitted by the frontend is agnostic to whether the LSP lives in kernels and needs to be stopped and started as the selected runtime changes, or if there is one LSP for all runtimes and we simply need to update it with the state of the selected runtime (as currently planned for Air).

I'm a bit worried about the rapid-fire case where a user quickly changes sessions or perhaps some extension code does it programmatically via a command. We should make sure that each language pack handles this gracefully.

On the positron-r side we have a queue for starting and stopping the LSP in our state change handler. See

this._queue.add(async () => {
. This queue should probably be shared with the runtime activation events.

In general language pack extensions need to manage for each session a queue of LSP activation/deactivation events in any case. But since it's their responsibility to implement that correctly, and since that can be tricky, it would be good if the session classes were allowed to implement a very simple queuing strategy that fully stops and starts the service for each event emitted, in order. That's what we did in Air and this queueing is probably more robust than the one currently in positron-r: https://github.com/posit-dev/air/blob/af84b665d8ec794bfba51c406866b6c266eeaf7e/editors/code/src/lsp.ts#L76.

I'm wondering about having a helper class provided by Positron that could be registered by language packs as handler for the session changes. It would have access to the current state of the LSP for each session managed by this language pack (stopped, starting, started, stopping), and would be smarter about starting and stopping LSPs only when needed. This would have the following benefits:

  • Backend sessions would be allowed to implement a very simple and bullet proof queueing strategy for their LSP lifecycle changes.

  • We'd have nice performance benefits from avoiding unnecessary work, e.g. in the case where a runtime is unselected, reselected, unselected, and reselected again (a realistic use case if the user is comparing outputs).

The other thing I noted is that currently we select a free port on each activation event and pass that to the activation routine. We know from our experience with Jupyter that this setup causes race conditions regarding port availability. Ideally the LSP would pick the port itself and communicate it to the frontend. That's a bigger change but we should at least make sure that ports are not picked ahead of time, the port picking should be queued along with the LSP start call to reduce the amount of time between the port selection and the actual startup.

if (this._lsp.state === LspState.running) {
this._lsp.deactivate(true);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

And @lionel- you think ark needs the tower-lsp patch so this._client!.stop() over in lsp.ts actually ends up stopping the LSP on the ark side?

@DavisVaughan
Copy link
Contributor

we select a free port on each activation event

I would not be surprised if we see reports that "the lsp wasn't reactivated after switching consoles" due to the race condition here. It came up decently often in CI, and allowing the lsp to be turned on and off is going to increase the chances of us hitting the race condition.

I'm not actually sure we can move the port picker any closer to the actual server startup, because both _kernel and _lsp need to know about the port, and the divergence between them happens where port picking already occurs.

@dhruvisompura dhruvisompura marked this pull request as ready for review March 12, 2025 16:41
@DavisVaughan
Copy link
Contributor

DavisVaughan commented Mar 12, 2025

LOL just ran into the race condition today when I restarted R, it looks like this and shows up in your console

Notably, you see Address already in use because the client (positron) picks the ports but does not bind to them, so in the short amount of time between when positron picked the port and the server (ark) gets the ports and binds to them, someone else can bind to that same port preventing ark from doing so as we see here.

I expect that having to pick ports a lot more often increases the chances of this occurring a lot

R 4.4.1 failed to start up (exit code -1)

The process exited abnormally (signal: 6 (SIGABRT))
thread 'main' panicked at crates/ark/src/start.rs:111:9:
Couldn't connect to frontend: SocketBindError("IOPub", "tcp://127.0.0.1:17344", Address already in use)
stack backtrace:
   0: rust_begin_unwind
             at /rustc/90b35a6239c3d8bdabc530a6a0816f7ff89a0aaf/library/std/src/panicking.rs:665:5
   1: core::panicking::panic_fmt
             at /rustc/90b35a6239c3d8bdabc530a6a0816f7ff89a0aaf/library/core/src/panicking.rs:74:14
   2: ark::start::start_kernel
             at /Users/davis/files/programming/positron/ark/crates/ark/src/start.rs:111:9
   3: ark::main
             at /Users/davis/files/programming/positron/ark/crates/ark/src/main.rs:370:5
   4: core::ops::function::FnOnce::call_once
             at /Users/davis/.rustup/toolchains/1.83-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/ops/function.rs:250:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

Copy link
Contributor

@seeM seeM left a comment

Choose a reason for hiding this comment

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

When I started a second Python console, diagnostics were immediately cleared, and then replaced with the second LS after 1 second (that's a debounce applied in the Python LS which I think is fine for now).

Switching back to the first console, I saw the same behavior.

Switching to the second console again, I ended up with two LSs.

Screen.Recording.2025-03-13.at.09.12.05.mov

@@ -114,6 +114,20 @@ export class PythonRuntimeSession implements positron.LanguageRuntimeSession, vs
this.onDidChangeRuntimeState = this._stateEmitter.event;
this.onDidEndSession = this._exitEmitter.event;

positron.runtime.onDidChangeForegroundSession((sessionId) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably dispose this. I see that we're not disposing the other event listener disposables but I think that may just have been a mistake. Doesn't need to be in this PR though.

Copy link
Contributor

Choose a reason for hiding this comment

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

@seeM can you open a new issue to outline what we need to look into, in terms of disposing which listeners here?

dhruvisompura added a commit that referenced this pull request Mar 13, 2025
Addresses #6719 

This fixes a regression introduced to `$selectLanguageRuntime` when
multiple console sessions is enabled.

The `$selectLanguageRuntime` function exposed to the extension host by
Positron is meant to be used by a language pack to signal to Positron
that the interpreter/runtime has changed on their end and that Positron
should take appropriate action. Right now that means Positron should
change the foreground session to a session for that runtime.

The foreground session is not updated by Positron when
`$selectLanguageRuntime` is being called which is causing the user to
see mismatched information in the UI.

The fix is to update the foreground session in Positron when
`$selectLanguageRuntime` is called.

Note: This doesn't address the inverse path. When the foreground session
changes, the Python extension does not currently change the active
interpreter. That can be handled after
#6714 which provides a way for
extensions to be notified the foreground session has changed.

### Release Notes


#### New Features

- N/A

#### Bug Fixes

- N/A


### QA Notes

@:sessions @:console @:web

### Screenshots

https://github.com/user-attachments/assets/59b878a1-87e1-48d7-9307-b8a1ce7421e2
@dhruvisompura dhruvisompura force-pushed the feature/mulitple-console-session-diagnostics branch from 06041e0 to 983414a Compare March 13, 2025 18:52
@dhruvisompura dhruvisompura changed the title mulitple console session: diagnostics and completion multiple console session: diagnostics and completion Mar 13, 2025
@juliasilge juliasilge changed the title multiple console session: diagnostics and completion multiple console session: start and stop LSPs Mar 14, 2025
@juliasilge juliasilge mentioned this pull request Mar 14, 2025
*
* @param sessionId The ID of the new foreground session
*/
@debounce(1000)
Copy link
Contributor

@juliasilge juliasilge Mar 14, 2025

Choose a reason for hiding this comment

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

Adding this debounce solves a LOT of the problems we are seeing around infinite completions and such. Looking at the outlines is very helpful to see how this works now; for a file that generates an R or Python outline, an additional outline will flash when you start an additional console (so there are two for a moment), but then it will update to only have one. You do not get additional outlines now when you switch between consoles.

A full second might be a little slow for this debounce.

@juliasilge
Copy link
Contributor

I am really happy with how multiple consoles are acting with the last few changes I pushed. I just kicked off the full suite of tests here, to see how things go in terms of general impact:

https://github.com/posit-dev/positron/actions/runs/13867728020

@juliasilge
Copy link
Contributor

juliasilge commented Mar 15, 2025

Update: tests are all green! ✅

What I would like to advocate for next is to review/merge this PR, because it improves the experience of our early users of this feature dramatically. I think multisession consoles are close to usable for real work now (at least for early adopters). Then we can deal with #6532 and #6533 as followups, to improve the behavior further (do a better job of stopping the LSP right away, deal with race condition for ports).

@juliasilge juliasilge requested review from jmcphers and removed request for jmcphers March 15, 2025 23:40
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.

6 participants