-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Wait for ready on raw session start and respect timeout on kernel change #11768
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
When switching to an invalid kernel (one that is registered but cannot start) in raw mode respect the launch timeout that is passed in. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,6 +22,13 @@ import { ReportableAction } from '../progress/types'; | |
import { RawSession } from '../raw-kernel/rawSession'; | ||
import { IJupyterKernelSpec, ISessionWithSocket } from '../types'; | ||
|
||
// Error thrown when we are unable to start a raw kernel session | ||
export class RawKernelSessionStartError extends Error { | ||
constructor(kernelTitle: string) { | ||
super(localize.DataScience.rawKernelSessionFailed().format(kernelTitle)); | ||
} | ||
} | ||
|
||
/* | ||
RawJupyterSession is the implementation of IJupyterSession that instead of | ||
connecting to JupyterLab services it instead connects to a kernel directly | ||
|
@@ -91,7 +98,7 @@ export class RawJupyterSession extends BaseJupyterSession { | |
} else if (newSession === null) { | ||
sendTelemetryEvent(Telemetry.RawKernelSessionStartTimeout); | ||
traceError('Raw session failed to start in given timeout'); | ||
throw new Error(localize.DataScience.sessionDisposed()); | ||
throw new RawKernelSessionStartError(kernelSpec.display_name || kernelSpec.name); | ||
} else { | ||
sendTelemetryEvent(Telemetry.RawKernelSessionStartSuccess); | ||
traceInfo('Raw session started and connected'); | ||
|
@@ -121,7 +128,7 @@ export class RawJupyterSession extends BaseJupyterSession { | |
|
||
public async createNewKernelSession( | ||
kernel: IJupyterKernelSpec | LiveKernelModel, | ||
_timeoutMS: number, | ||
timeoutMS: number, | ||
interpreter?: PythonInterpreter | ||
): Promise<ISessionWithSocket> { | ||
if (!kernel || 'session' in kernel) { | ||
|
@@ -131,7 +138,13 @@ export class RawJupyterSession extends BaseJupyterSession { | |
|
||
this.outputChannel.appendLine(localize.DataScience.kernelStarted().format(kernel.display_name || kernel.name)); | ||
|
||
return this.startRawSession(kernel, interpreter); | ||
const newSession = await waitForPromise(this.startRawSession(kernel, interpreter), timeoutMS); | ||
|
||
if (!newSession) { | ||
throw new RawKernelSessionStartError(kernel.display_name || kernel.name); | ||
} | ||
|
||
return newSession; | ||
} | ||
|
||
protected shutdownSession( | ||
|
@@ -209,6 +222,9 @@ export class RawJupyterSession extends BaseJupyterSession { | |
// Create our raw session, it will own the process lifetime | ||
const result = new RawSession(process); | ||
|
||
// When our kernel connects and gets a status message it triggers the ready promise | ||
await result.kernel.ready; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was actually the more interesting part of the fix. When we were having those initial windows launch fixes I examined this promise, but it didn't actually help with the case where the ports were not open yet. But we do need it here. Ready waits for that initial kernel_status message to be returned. So without this here we might have a bad kernel that can't actually start (regardless of if the process exits or not) but we would leak outside of the start timeout check. So the start or switch timeout check would not trigger, but we'd be stuck with a hung kernel. |
||
|
||
// So that we don't have problems with ipywidgets, always register the default ipywidgets comm target. | ||
// Restart sessions and retries might make this hard to do correctly otherwise. | ||
result.kernel.registerCommTarget(Identifiers.DefaultCommTarget, noop); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needed to respect the restart timeout that was getting passed in here.