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

Fix exit from Python console within reticulate sessions #5056

Merged
merged 5 commits into from
Oct 30, 2024

Conversation

dfalbel
Copy link
Contributor

@dfalbel dfalbel commented Oct 17, 2024

Addresses #5022, #4982 and #5144

Positron detects that the session is closed whenever the zeroMQ sockets are closed, triggering UI updates, etc.
Reticulate Python sessions are managed within the R process, thus when exitting the IPykernel, the process is not necessarily closed - which would make sure the zeroMQ sockets are also closed.

This PR makes sure that the IPyKernel closes the zeroMQ sockets before exitting.

While this PR fixes the initial issue, it uncovers a another issue that I still couldn't figure out how to fix, but have some understanding of what's happening:

If you create a Reticulate Python session, then shut it down (either by typing exit or clicking the shutdown button) and then restart the reticulate python session, the LSP for that second session will never correctly start. It seems it gets stuck in the Starting state.

This is problematic because the next time you click on Shutdown to close the Python session it will be stuck again.
The reason is that the shutdown routine in:

async shutdown(exitReason = positron.RuntimeExitReason.Shutdown): Promise<void> {
if (this._kernel) {
// Stop the LSP client before shutting down the kernel
await this._lsp?.deactivate(true);
return this._kernel.shutdown(exitReason);
} else {
throw new Error('Cannot shutdown; kernel not started');
}
}

Tries to deactivate the LSP, which then blocks in:

// First wait for initialization to complete.
// `stop()` should not be called on a
// partially initialized client.
await this._initializing;

@seeM I suspect that there's still some state that is not completely cleaned up when the IPyKernel exits that's causing
the LSP to not start correctly. I spent a good ammount of time trying to figure out what's happening but could not find yet. Do you have suggestions for where to look at? I'm pretty sure that the sockets are created in:

def on_comm_open(self, comm: BaseComm, msg: Dict[str, Any]) -> None:
"""
Setup positron.lsp comm to receive messages.
"""
self._comm = comm
# Register the comm message handler
comm.on_msg(self._receive_message)
# Parse the host and port from the comm open message
data = msg["content"]["data"]
client_address = data.get("client_address", None)
if client_address is None:
logger.warning(f"No client_address in LSP comm open message: {msg}")
return
host, port = self._split_address(client_address)
if host is None or port is None:
logger.warning(f"Could not parse host and port from client address: {client_address}")
return
# Start the language server thread
POSITRON.start(lsp_host=host, lsp_port=port, shell=self._kernel.shell, comm=comm)

And we are somehow waiting for:

this._client.onDidChangeState((event) => {
const oldState = this._state;
// Convert the state to our own enum
switch (event.newState) {
case State.Starting:
this._state = LspState.starting;
break;
case State.Running:
if (this._initializing) {
traceInfo(`Python (${this._version}) language client init successful`);
this._initializing = undefined;
out.resolve();
}
if (this._client) {
// Register Positron-specific LSP extension methods
this.registerPositronLspExtensions(this._client);
}
this._state = LspState.running;
break;
case State.Stopped:
if (this._initializing) {
traceInfo(`Python (${this._version}) language client init failed`);
out.reject('Python LSP client stopped before initialization');
}
this._state = LspState.stopped;
break;
default:
traceError(`Unexpected language client state: ${event.newState}`);
out.reject('Unexpected language client state');
}
traceInfo(`Python (${this._version}) language client state changed ${oldState} => ${this._state}`);

Because we never reach the State.Running, but we do get State.Starting.

@dfalbel dfalbel requested a review from seeM October 17, 2024 13:12
@dfalbel dfalbel force-pushed the bugfix/exit-reticulate-python branch from a410174 to 0c756ff Compare October 18, 2024 16:44
Copy link
Collaborator

@jmcphers jmcphers left a comment

Choose a reason for hiding this comment

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

This change LGTM and in my testing I was able to exit from Reticulate sessions w/o issue. I'm going to go ahead and merge it for the 2024.11 release; @seeM please let us know if you notice anything post-merge.

@jmcphers jmcphers merged commit 578e92c into main Oct 30, 2024
23 checks passed
@jmcphers jmcphers deleted the bugfix/exit-reticulate-python branch October 30, 2024 22:28
@github-actions github-actions bot locked and limited conversation to collaborators Oct 30, 2024
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.

Code changes LGTM! The workbench.action.positronConsole.focusConsole command might work to focus the console and might be more direct, but not an important change at all.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants