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.

Sign up for GitHub

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

Do not use Uris to uniquely identify Server Handles (1) #13540

Closed
wants to merge 1 commit into from
Closed

Do not use Uris to uniquely identify Server Handles (1) #13540

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions src/kernels/errors/invalidRemoteJupyterServerUriHandleError.ts
View file
Open in desktop
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 },
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Handle Id and Handle are not unique, two extensions could have the same two values.

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
Open in desktop
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);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Consistently use Serverhandle everwhere instead of generating an id called serverId and passing that around.

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
Open in desktop
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
Open in desktop
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
5 changes: 0 additions & 5 deletions src/kernels/execution/helpers.unit.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@ suite(`UpdateNotebookMetadata`, () => {
test('Update Language', async () => {
const notebookMetadata = { orig_nbformat: 4, language_info: { name: 'JUNK' } };
const kernelConnection = PythonKernelConnectionMetadata.create({
id: 'python36',
interpreter: python36Global,
kernelSpec: pythonDefaultKernelSpec
});
Expand All @@ -68,7 +67,6 @@ suite(`UpdateNotebookMetadata`, () => {
test('Update Python Version', async () => {
const notebookMetadata = { orig_nbformat: 4, language_info: { name: 'python', version: '3.6.0' } };
const kernelConnection = PythonKernelConnectionMetadata.create({
id: 'python36',
interpreter: python37Global,
kernelSpec: pythonDefaultKernelSpec
});
Expand All @@ -90,7 +88,6 @@ suite(`UpdateNotebookMetadata`, () => {
language_info: { name: 'python', version: '3.6.0' }
};
const kernelConnection = PythonKernelConnectionMetadata.create({
id: 'python36',
interpreter: python36Global,
kernelSpec: pythonDefaultKernelSpec
});
Expand Down Expand Up @@ -122,7 +119,6 @@ suite(`UpdateNotebookMetadata`, () => {
language_info: { name: 'python', version: '3.6.0' }
};
const kernelConnection = PythonKernelConnectionMetadata.create({
id: 'python36',
interpreter: python36Global,
kernelSpec: pythonDefaultKernelSpec
});
Expand Down Expand Up @@ -170,7 +166,6 @@ suite(`UpdateNotebookMetadata`, () => {
language_info: { name: 'python', version: '3.6.0' }
};
const kernelConnection = PythonKernelConnectionMetadata.create({
id: 'python36',
interpreter: python36Global,
kernelSpec: pythonDefaultKernelSpec
});
Expand Down
Loading