-
Notifications
You must be signed in to change notification settings - Fork 299
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
Verify remote Jupyter Sever connections upon connecting to remote kernels #9941
Changes from 12 commits
3a34293
f068232
3b0942b
0f2551e
04d6513
4afff5d
bdbb177
3196ed0
b2405b2
3aa7c6f
f8723af
79ac6f1
a4b48ab
caf5914
60858b1
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 @@ | ||
Validate remote Jupyter Server connections when attempting to start a kernel. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Notify failures in connection to remote Jupyter Server only when connecting to those kernels. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -35,6 +35,7 @@ import { Cancellation } from '../../../../platform/common/cancellation'; | |
import { getDisplayPath } from '../../../../platform/common/platform/fs-paths'; | ||
import { INotebookServer } from '../../types'; | ||
import { Uri } from 'vscode'; | ||
import { RemoteJupyterServerConnectionError } from '../../../../platform/errors/remoteJupyterServerConnectionError'; | ||
/* eslint-disable @typescript-eslint/no-explicit-any */ | ||
|
||
export class HostJupyterServer implements INotebookServer { | ||
|
@@ -150,6 +151,22 @@ export class HostJupyterServer implements INotebookServer { | |
if (!this.sessionManager || this.isDisposed) { | ||
throw new SessionDisposedError(); | ||
} | ||
if ( | ||
this.sessionManager && | ||
!this.isDisposed && | ||
(kernelConnection.kind === 'connectToLiveRemoteKernel' || | ||
kernelConnection.kind === 'startUsingRemoteKernelSpec') | ||
) { | ||
try { | ||
await Promise.all([this.sessionManager.getRunningKernels(), this.sessionManager.getKernelSpecs()]); | ||
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. Ensure we try to fetch the kernels (validate the remote jupyter server connection) before creating a session. 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. As well as the current issue #9167 |
||
} catch (ex) { | ||
traceError( | ||
'Failed to fetch running kernels from remote server, connection may be outdated or remote server may be unreachable', | ||
ex | ||
); | ||
throw new RemoteJupyterServerConnectionError(kernelConnection.baseUrl, kernelConnection.serverId, ex); | ||
} | ||
} | ||
const stopWatch = new StopWatch(); | ||
// Create a session and return it. | ||
try { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -48,7 +48,6 @@ export class RemoteKernelFinder implements IRemoteKernelFinder { | |
): Promise<KernelConnectionMetadata[]> { | ||
// Get a jupyter session manager to talk to | ||
let sessionManager: IJupyterSessionManager | undefined; | ||
|
||
// This should only be used when doing remote. | ||
if (connInfo.type === 'jupyter') { | ||
try { | ||
|
@@ -113,6 +112,7 @@ export class RemoteKernelFinder implements IRemoteKernelFinder { | |
return items; | ||
} catch (ex) { | ||
traceError(`Error fetching remote kernels:`, ex); | ||
throw ex; | ||
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. Previously we'd swallow exceptions and return an empty list, now we throw the error and higher up when we have errors we just fallback to the cache (this behaviour is documented in the calling code) |
||
} finally { | ||
if (sessionManager) { | ||
await sessionManager.dispose(); | ||
|
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.
cerateConnectionInfo
will ensure the server Uri has been updated, calling code need to know about such internals