Skip to content

Commit

Permalink
Fix exit from Python console within reticulate sessions (#5056)
Browse files Browse the repository at this point in the history
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:


https://github.com/posit-dev/positron/blob/66c701594d7eba0d57bf40c8d04cb488b4464f47/extensions/positron-python/src/client/positron/session.ts#L386-L394

Tries to `deactivate` the LSP, which then blocks in:


https://github.com/posit-dev/positron/blob/66c701594d7eba0d57bf40c8d04cb488b4464f47/extensions/positron-python/src/client/positron/lsp.ts#L183-L187

@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:


https://github.com/posit-dev/positron/blob/a4101740ce6806cbf95ad2aaf45dacd963c2f11d/extensions/positron-python/python_files/positron/positron_ipykernel/lsp.py#L30-L52

And we are somehow waiting for: 


https://github.com/posit-dev/positron/blob/a4101740ce6806cbf95ad2aaf45dacd963c2f11d/extensions/positron-python/src/client/positron/lsp.ts#L125-L155

Because we never reach the `State.Running`, but we do get
`State.Starting`.
  • Loading branch information
dfalbel authored Oct 30, 2024
1 parent af48f1a commit 578e92c
Show file tree
Hide file tree
Showing 3 changed files with 14 additions and 10 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,11 @@ def start(self, lsp_host: str, lsp_port: int, shell: "PositronShell", comm: Base
# Give the LSP server access to the kernel to enhance completions with live variables
self.shell = shell

# If self.lsp has been used previously in this process and sucessfully exited, it will be
# marked with a shutdown flag, which makes it ignore all messages.
# We reset it here, so we allow the server to start again.
self.lsp._shutdown = False

if self._server_thread is not None:
logger.warning("LSP server thread was not properly shutdown")
return
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,11 @@
import os
import sys

from positron_ipykernel.positron_ipkernel import PositronIPKernelApp, PositronIPyKernel
from positron_ipykernel.positron_ipkernel import (
PositronIPKernelApp,
PositronIPyKernel,
PositronShell,
)
from positron_ipykernel.positron_jedilsp import POSITRON
from positron_ipykernel.session_mode import SessionMode

Expand Down Expand Up @@ -151,8 +155,10 @@ def parse_args() -> argparse.Namespace:
# When the app is gone, it should be safe to clear singleton instances.
# This allows re-starting the ipykernel in the same process, using different
# connection strings, etc.
PositronShell.clear_instance()
PositronIPyKernel.clear_instance()
PositronIPKernelApp.clear_instance()
app.close()

logger.info(f"Exiting process with status {exit_status}")
sys.exit(exit_status)
11 changes: 2 additions & 9 deletions extensions/positron-reticulate/src/extension.ts
Original file line number Diff line number Diff line change
Expand Up @@ -523,15 +523,8 @@ class ReticulateRuntimeSession implements positron.LanguageRuntimeSession {

public async shutdown(exitReason: positron.RuntimeExitReason) {
await this.pythonSession.shutdown(exitReason);
// Tell Positron that the kernel has exit. When launching IPykernel from a standalone
// process, when the kernel exits, then all of it's threads, specially the IOPub thread
// holding the ZeroMQ sockets will cease to exist, and thus Positron identifies that the
// kernel has successfuly closed. However, since we launch positron from a different thread,
// when the kernel exits, the thread exits, but all other dangling threads are still alive,
// thus Positron never identifies that the kernel exited. We must then manually fire exit event.
// We rely on an implementation detail of the jupyter adapter, that allows us to force the
// kernels to disconnect.
(this.pythonSession as any)._kernel._kernel._allSockets.forEach((socket: any) => socket.disconnect());
// Execute some dummy code in the R session to shift focus to it.
await positron.runtime.executeCode('r', '', true, true);
return;
}

Expand Down

0 comments on commit 578e92c

Please sign in to comment.