Skip to content

Commit

Permalink
Remove serverId and URIs to identify servers
Browse files Browse the repository at this point in the history
  • Loading branch information
@DonJayamanne
DonJayamanne committed May 29, 2023
1 parent 8e8dc1d commit a8d5c1a
Show file tree
Hide file tree
Showing 86 changed files with 1,377 additions and 1,186 deletions.
6 changes: 2 additions & 4 deletions src/kernels/errors/invalidRemoteJupyterServerUriHandleError.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,8 @@ import { BaseError } from '../../platform/errors/types';
*/
export class InvalidRemoteJupyterServerUriHandleError extends BaseError {
constructor(
public readonly providerId: string,
public readonly handle: string,
public readonly extensionId: string,
public readonly serverId: string
public readonly serverHandle: { extensionId: string; id: string; handle: string },
public readonly extensionId: string
) {
super('invalidremotejupyterserverurihandle', 'Server handle not in list of known handles');
}
Expand Down
35 changes: 19 additions & 16 deletions src/kernels/errors/kernelErrorHandler.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -234,10 +234,10 @@ export abstract class DataScienceErrorHandler implements IDataScienceErrorHandle
error: RemoteJupyterServerUriProviderError,
errorContext?: KernelAction
) {
const server = await this.serverUriStorage.get(error.serverId);
const server = await this.serverUriStorage.get(error.serverHandle);
const message = error.originalError?.message || error.message;
const displayName = server?.displayName;
const idAndHandle = `${error.providerId}:${error.handle}`;
const idAndHandle = `${error.serverHandle.id}:${error.serverHandle.handle}`;
const serverName = displayName || idAndHandle;

return getUserFriendlyErrorMessage(
Expand All @@ -249,37 +249,38 @@ export abstract class DataScienceErrorHandler implements IDataScienceErrorHandle
error: RemoteJupyterServerConnectionError,
errorContext?: KernelAction
) {
const info = await this.serverUriStorage.get(error.serverId);
const info = await this.serverUriStorage.get(error.serverHandle);
const message = error.originalError.message || '';

if (info?.provider) {
const serverName = info.displayName ?? error.url;
if (info?.serverHandle) {
const serverName = info.displayName ?? error.baseUrl;
return getUserFriendlyErrorMessage(
DataScience.remoteJupyterConnectionFailedWithServerWithError(serverName, message),
errorContext
);
}

const baseUrl = error.baseUrl;
const serverName =
info?.displayName && baseUrl ? `${info.displayName} (${baseUrl})` : info?.displayName || baseUrl;
info?.displayName && error.baseUrl
? `${info.displayName} (${error.baseUrl})`
: info?.displayName || error.baseUrl;

return getUserFriendlyErrorMessage(
DataScience.remoteJupyterConnectionFailedWithServerWithError(serverName, message),
errorContext
);
}
private async handleJupyterServerProviderConnectionError(info: IJupyterServerUriEntry) {
const provider = await this.jupyterUriProviderRegistration.getProvider(info.serverId);
const provider = await this.jupyterUriProviderRegistration.getProvider(info.serverHandle.id);
if (!provider || !provider.getHandles) {
return false;
}

try {
const handles = await provider.getHandles();

if (!handles.includes(info.provider.handle)) {
await this.serverUriStorage.remove(info.serverId);
if (!handles.includes(info.serverHandle.handle)) {
await this.serverUriStorage.remove(info.serverHandle);
}
return true;
} catch (_ex) {
Expand Down Expand Up @@ -346,12 +347,14 @@ export abstract class DataScienceErrorHandler implements IDataScienceErrorHandle
: err instanceof RemoteJupyterServerConnectionError
? err.originalError.message || ''
: err.originalError?.message || err.message;
const serverId = err instanceof RemoteJupyterServerConnectionError ? err.serverId : err.serverId;
const server = await this.serverUriStorage.get(serverId);
const provider = err.serverHandle;
const server = await this.serverUriStorage.get(provider);
const displayName = server?.displayName;
const baseUrl = err instanceof RemoteJupyterServerConnectionError ? err.baseUrl : '';
const idAndHandle =
err instanceof RemoteJupyterServerUriProviderError ? `${err.providerId}:${err.handle}` : '';
err instanceof RemoteJupyterServerUriProviderError
? `${err.serverHandle.id}:${err.serverHandle.handle}`
: '';
const serverName =
displayName && baseUrl ? `${displayName} (${baseUrl})` : displayName || baseUrl || idAndHandle;
const extensionName =
Expand All @@ -375,9 +378,9 @@ export abstract class DataScienceErrorHandler implements IDataScienceErrorHandle
switch (selection) {
case DataScience.removeRemoteJupyterConnectionButtonText: {
// Remove this uri if already found (going to add again with a new time)
const item = await this.serverUriStorage.get(serverId);
if (item) {
await this.serverUriStorage.remove(item.serverId);
const item = provider ? await this.serverUriStorage.get(provider) : undefined;
if (item && provider) {
await this.serverUriStorage.remove(provider);
}
// Wait until all of the remote controllers associated with this server have been removed.
return KernelInterpreterDependencyResponse.cancel;
Expand Down
89 changes: 49 additions & 40 deletions src/kernels/errors/kernelErrorHandler.unit.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,13 @@ import {
IJupyterInterpreterDependencyManager,
IJupyterServerUriStorage,
IJupyterUriProviderRegistration,
JupyterInterpreterDependencyResponse
JupyterInterpreterDependencyResponse,
JupyterServerProviderHandle
} from '../jupyter/types';
import { getDisplayNameOrNameOfKernelConnection } from '../helpers';
import { getOSType, OSType } from '../../platform/common/utils/platform';
import { RemoteJupyterServerConnectionError } from '../../platform/errors/remoteJupyterServerConnectionError';
import { computeServerId, generateUriFromRemoteProvider } from '../jupyter/jupyterUtils';
import { jupyterServerHandleToString } from '../jupyter/jupyterUtils';
import { RemoteJupyterServerUriProviderError } from './remoteJupyterServerUriProviderError';
import { IReservedPythonNamedProvider } from '../../platform/interpreter/types';
import { DataScienceErrorHandlerNode } from './kernelErrorHandler.node';
Expand Down Expand Up @@ -151,15 +152,11 @@ suite('Error Handler Unit Tests', () => {

suite('Kernel startup errors', () => {
let kernelConnection: KernelConnectionMetadata;
const uri = generateUriFromRemoteProvider('1', 'a');
let serverId: string;
suiteSetup(async () => {
serverId = await computeServerId(uri);
});
const serverHandle: JupyterServerProviderHandle = { extensionId: 'ext', id: '1', handle: 'a' };
const serverHandleId = jupyterServerHandleToString(serverHandle);
setup(() => {
when(applicationShell.showErrorMessage(anything(), Common.learnMore)).thenResolve(Common.learnMore as any);
kernelConnection = PythonKernelConnectionMetadata.create({
id: '',
interpreter: {
uri: Uri.file('Hello There'),
id: Uri.file('Hello There').fsPath,
Expand Down Expand Up @@ -785,17 +782,20 @@ Failed to run jupyter as observable with args notebook --no-browser --notebook-d
);
});
test('Display error when connection to remote jupyter server fails', async () => {
const error = new RemoteJupyterServerConnectionError(uri, serverId, new Error('ECONNRESET error'));
const connection = RemoteKernelSpecConnectionMetadata.create({
const error = new RemoteJupyterServerConnectionError(
serverHandleId,
serverHandle,
new Error('ECONNRESET error')
);
const connection = await RemoteKernelSpecConnectionMetadata.create({
baseUrl: 'http://hello:1234/',
id: '1',
kernelSpec: {
argv: [],
display_name: '',
name: '',
executable: ''
},
serverId
serverHandle
});
when(
applicationShell.showErrorMessage(anything(), anything(), anything(), anything(), anything())
Expand All @@ -818,27 +818,24 @@ Failed to run jupyter as observable with args notebook --no-browser --notebook-d
DataScience.selectDifferentKernel
)
).once();
verify(uriStorage.remove(serverId)).never();
verify(uriStorage.remove(anything())).never();
});
test('Display error when connection to remote jupyter server fails due to 3rd party extension', async () => {
const error = new RemoteJupyterServerUriProviderError('1', 'a', new Error('invalid handle'), serverId);
const connection = RemoteKernelSpecConnectionMetadata.create({
const error = new RemoteJupyterServerUriProviderError(serverHandle, new Error('invalid handle'));
const connection = await RemoteKernelSpecConnectionMetadata.create({
baseUrl: 'http://hello:1234/',
id: '1',
kernelSpec: {
argv: [],
display_name: '',
name: '',
executable: ''
},
serverId
serverHandle: serverHandle
});
when(uriStorage.get(serverId)).thenResolve({
when(uriStorage.get(anything())).thenResolve({
time: 1,
uri,
serverId,
displayName: 'Hello Server',
provider: { id: '1', handle: 'a' }
serverHandle: serverHandle
});
when(
applicationShell.showErrorMessage(anything(), anything(), anything(), anything(), anything())
Expand All @@ -861,27 +858,33 @@ Failed to run jupyter as observable with args notebook --no-browser --notebook-d
DataScience.selectDifferentKernel
)
).once();
verify(uriStorage.remove(serverId)).never();
verify(uriStorage.get(serverId)).atLeast(1);
verify(uriStorage.remove(anything())).never();
verify(uriStorage.get(anything())).atLeast(1);
});
test('Remove remote Uri if user choses to do so, when connection to remote jupyter server fails', async () => {
const error = new RemoteJupyterServerConnectionError(uri, serverId, new Error('ECONNRESET error'));
const connection = RemoteKernelSpecConnectionMetadata.create({
const error = new RemoteJupyterServerConnectionError(
serverHandleId,
serverHandle,
new Error('ECONNRESET error')
);
const connection = await RemoteKernelSpecConnectionMetadata.create({
baseUrl: 'http://hello:1234/',
id: '1',
kernelSpec: {
argv: [],
display_name: '',
name: '',
executable: '' // Send nothing for argv[0]
},
serverId
serverHandle
});
when(
applicationShell.showErrorMessage(anything(), anything(), anything(), anything(), anything())
).thenResolve(DataScience.removeRemoteJupyterConnectionButtonText as any);
when(uriStorage.remove(anything())).thenResolve();
when(uriStorage.get(serverId)).thenResolve({ uri, serverId, time: 2, provider: { id: '1', handle: 'a' } });
when(uriStorage.get(anything())).thenResolve({
time: 2,
serverHandle
});
const result = await dataScienceErrorHandler.handleKernelError(
error,
'start',
Expand All @@ -890,21 +893,24 @@ Failed to run jupyter as observable with args notebook --no-browser --notebook-d
'jupyterExtension'
);
assert.strictEqual(result, KernelInterpreterDependencyResponse.cancel);
verify(uriStorage.remove(serverId)).once();
verify(uriStorage.get(serverId)).atLeast(1);
verify(uriStorage.remove(anything())).once();
verify(uriStorage.get(anything())).atLeast(1);
});
test('Change remote Uri if user choses to do so, when connection to remote jupyter server fails', async () => {
const error = new RemoteJupyterServerConnectionError(uri, serverId, new Error('ECONNRESET error'));
const connection = RemoteKernelSpecConnectionMetadata.create({
const error = new RemoteJupyterServerConnectionError(
serverHandleId,
serverHandle,
new Error('ECONNRESET error')
);
const connection = await RemoteKernelSpecConnectionMetadata.create({
baseUrl: 'http://hello:1234/',
id: '1',
kernelSpec: {
argv: [],
display_name: '',
name: '',
executable: ''
},
serverId
serverHandle
});
when(
applicationShell.showErrorMessage(anything(), anything(), anything(), anything(), anything())
Expand All @@ -918,20 +924,23 @@ Failed to run jupyter as observable with args notebook --no-browser --notebook-d
'jupyterExtension'
);
assert.strictEqual(result, KernelInterpreterDependencyResponse.cancel);
verify(uriStorage.remove(serverId)).never();
verify(uriStorage.remove(anything())).never();
});
test('Select different kernel user choses to do so, when connection to remote jupyter server fails', async () => {
const error = new RemoteJupyterServerConnectionError(uri, serverId, new Error('ECONNRESET error'));
const connection = RemoteKernelSpecConnectionMetadata.create({
const error = new RemoteJupyterServerConnectionError(
serverHandleId,
serverHandle,
new Error('ECONNRESET error')
);
const connection = await RemoteKernelSpecConnectionMetadata.create({
baseUrl: 'http://hello:1234/',
id: '1',
kernelSpec: {
argv: [],
display_name: '',
name: '',
executable: ''
},
serverId
serverHandle
});
when(
applicationShell.showErrorMessage(anything(), anything(), anything(), anything(), anything())
Expand All @@ -944,7 +953,7 @@ Failed to run jupyter as observable with args notebook --no-browser --notebook-d
'jupyterExtension'
);
assert.strictEqual(result, KernelInterpreterDependencyResponse.selectDifferentKernel);
verify(uriStorage.remove(serverId)).never();
verify(uriStorage.remove(anything())).never();
});
function verifyErrorMessage(message: string, linkInfo?: string) {
message = message.includes('command:jupyter.viewOutput')
Expand Down
6 changes: 2 additions & 4 deletions src/kernels/errors/remoteJupyterServerUriProviderError.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,8 @@ import { BaseError } from '../../platform/errors/types';
*/
export class RemoteJupyterServerUriProviderError extends BaseError {
constructor(
public readonly providerId: string,
public readonly handle: string,
public readonly originalError: Error,
public serverId: string
public readonly serverHandle: { extensionId: string; id: string; handle: string },
public readonly originalError: Error
) {
super('remotejupyterserveruriprovider', originalError.message || originalError.toString());
}
Expand Down
2 changes: 1 addition & 1 deletion src/kernels/execution/cellExecution.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -414,7 +414,7 @@ export class CellExecution implements IDisposable {
try {
// At this point we're about to ACTUALLY execute some code. Fire an event to indicate that
this._preExecuteEmitter.fire(this.cell);

traceVerbose(`Execution Request Sent to Kernel for cell ${this.cell.index}`);
// For Jupyter requests, silent === don't output, while store_history === don't update execution count
// https://jupyter-client.readthedocs.io/en/stable/api/client.html#jupyter_client.KernelClient.execute
this.request = session.requestExecute(
Expand Down
4 changes: 2 additions & 2 deletions src/kernels/execution/cellExecutionCreator.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import {
NotebookCellOutputItem,
TextDocument
} from 'vscode';
import { traceVerbose } from '../../platform/logging';
import { traceInfo, traceVerbose } from '../../platform/logging';
import { sendTelemetryEvent, Telemetry } from '../../telemetry';
import { IKernelController } from '../types';
import { noop } from '../../platform/common/utils/misc';
Expand Down Expand Up @@ -70,7 +70,7 @@ export class NotebookCellExecutionWrapper implements NotebookCellExecution {
if (this._endCallback) {
try {
this._impl.end(success, endTime);
traceVerbose(
traceInfo(
`End cell ${this.cell.index} execution @ ${endTime}, started @ ${this._startTime}, elapsed time = ${
((endTime || 0) - (this._startTime || 0)) / 1000
}s`
Expand Down
3 changes: 2 additions & 1 deletion src/kernels/execution/cellExecutionMessageHandler.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ import type { Kernel } from '@jupyterlab/services';
import { CellExecutionCreator } from './cellExecutionCreator';
import { IApplicationShell } from '../../platform/common/application/types';
import { disposeAllDisposables } from '../../platform/common/helpers';
import { traceError, traceInfoIfCI, traceVerbose, traceWarning } from '../../platform/logging';
import { traceError, traceInfo, traceInfoIfCI, traceVerbose, traceWarning } from '../../platform/logging';
import { IDisposable, IExtensionContext } from '../../platform/common/types';
import { concatMultilineString, formatStreamText, isJupyterNotebook } from '../../platform/common/utils';
import {
Expand Down Expand Up @@ -485,6 +485,7 @@ export class CellExecutionMessageHandler implements IDisposable {
//
}
this.execution?.start(this.startTime);
traceInfo(`Kernel acknowledged execution of cell ${this.cell.index} @ ${this.startTime}`);
}

// eslint-disable-next-line @typescript-eslint/no-require-imports
Expand Down
Loading

0 comments on commit a8d5c1a

Please sign in to comment.