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

Jump to bottom

Refactor URI storage provider #13527

Merged
merged 3 commits into from
May 19, 2023
Merged

Refactor URI storage provider #13527

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3c4517b
Refactor URI storage provider
DonJayamanne May 18, 2023
2a8a7b5
oops
DonJayamanne May 18, 2023
788f679
fix tests
DonJayamanne May 18, 2023
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
2 changes: 0 additions & 2 deletions src/extension.node.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,6 @@ import { IInterpreterPackages } from './platform/interpreter/types';
import { homedir, platform, arch, userInfo } from 'os';
import { getUserHomeDir } from './platform/common/utils/platform.node';
import { homePath } from './platform/common/platform/fs-paths.node';
import { removeOldCachedItems } from './platform/common/cache';

durations.codeLoadingTime = stopWatch.elapsedTime;

Expand All @@ -111,7 +110,6 @@ let activatedServiceContainer: IServiceContainer | undefined;
export async function activate(context: IExtensionContext): Promise<IExtensionApi> {
context.subscriptions.push({ dispose: () => (Exiting.isExiting = true) });
try {
removeOldCachedItems(context.globalState).then(noop, noop);
let api: IExtensionApi;
let ready: Promise<void>;
let serviceContainer: IServiceContainer;
Expand Down
2 changes: 0 additions & 2 deletions src/extension.web.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,6 @@ import { ServiceManager } from './platform/ioc/serviceManager';
import { OutputChannelLogger } from './platform/logging/outputChannelLogger';
import { ConsoleLogger } from './platform/logging/consoleLogger';
import { initializeGlobals as initializeTelemetryGlobals } from './platform/telemetry/telemetry';
import { removeOldCachedItems } from './platform/common/cache';

durations.codeLoadingTime = stopWatch.elapsedTime;

Expand All @@ -116,7 +115,6 @@ let activatedServiceContainer: IServiceContainer | undefined;
export async function activate(context: IExtensionContext): Promise<IExtensionApi> {
context.subscriptions.push({ dispose: () => (Exiting.isExiting = true) });
try {
removeOldCachedItems(context.globalState).then(noop, noop);
let api: IExtensionApi;
let ready: Promise<void>;
let serviceContainer: IServiceContainer;
Expand Down
76 changes: 23 additions & 53 deletions src/kernels/errors/kernelErrorHandler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import {
} from '../../platform/common/types';
import { DataScience, Common } from '../../platform/common/utils/localize';
import { sendTelemetryEvent, Telemetry } from '../../telemetry';
import { Commands, Identifiers } from '../../platform/common/constants';
import { Commands } from '../../platform/common/constants';
import { getDisplayNameOrNameOfKernelConnection } from '../helpers';
import { translateProductToModule } from '../../platform/interpreter/installer/utils';
import { ProductNames } from '../../platform/interpreter/installer/productNames';
Expand All @@ -47,15 +47,12 @@ import { KernelDeadError } from './kernelDeadError';
import { DisplayOptions } from '../displayOptions';
import {
IJupyterInterpreterDependencyManager,
IJupyterServerUriEntry,
IJupyterServerUriStorage,
IJupyterUriProviderRegistration,
JupyterInterpreterDependencyResponse
} from '../jupyter/types';
import {
extractJupyterServerHandleAndId,
handleExpiredCertsError,
handleSelfCertsError
} from '../jupyter/jupyterUtils';
import { handleExpiredCertsError, handleSelfCertsError } from '../jupyter/jupyterUtils';
import { getDisplayPath, getFilePath } from '../../platform/common/platform/fs-paths';
import { isCancellationError } from '../../platform/common/cancellation';
import { JupyterExpiredCertsError } from '../../platform/errors/jupyterExpiredCertsError';
Expand Down Expand Up @@ -237,10 +234,9 @@ export abstract class DataScienceErrorHandler implements IDataScienceErrorHandle
error: RemoteJupyterServerUriProviderError,
errorContext?: KernelAction
) {
const savedList = await this.serverUriStorage.getMRUs();
const server = await this.serverUriStorage.get(error.serverId);
const message = error.originalError?.message || error.message;
const serverId = error.serverId;
const displayName = savedList.find((item) => item.serverId === serverId)?.displayName;
const displayName = server?.displayName;
const idAndHandle = `${error.providerId}:${error.handle}`;
const serverName = displayName || idAndHandle;

Expand All @@ -253,59 +249,37 @@ export abstract class DataScienceErrorHandler implements IDataScienceErrorHandle
error: RemoteJupyterServerConnectionError,
errorContext?: KernelAction
) {
const savedList = await this.serverUriStorage.getMRUs();
const info = await this.serverUriStorage.get(error.serverId);
const message = error.originalError.message || '';
const serverId = error.serverId;
const displayName = savedList.find((item) => item.serverId === serverId)?.displayName;

if (error.baseUrl === Identifiers.REMOTE_URI) {
// 3rd party server uri error
const idAndHandle = extractJupyterServerHandleAndId(error.url);
if (idAndHandle) {
const serverUri = await this.jupyterUriProviderRegistration.getJupyterServerUri(
idAndHandle.id,
idAndHandle.handle
);

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

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

return getUserFriendlyErrorMessage(
DataScience.remoteJupyterConnectionFailedWithServerWithError(serverName, message),
errorContext
);
}
private generateJupyterIdAndHandleErrorName(url: string): string | undefined {
const idAndHandle = extractJupyterServerHandleAndId(url);

return idAndHandle ? `${idAndHandle.id}:${idAndHandle.handle}` : undefined;
}
private async handleJupyterServerProviderConnectionError(uri: string) {
const idAndHandle = extractJupyterServerHandleAndId(uri);

if (!idAndHandle) {
return false;
}

const provider = await this.jupyterUriProviderRegistration.getProvider(idAndHandle.id);
if (!provider || !provider.getHandles) {
private async handleJupyterServerProviderConnectionError(info: IJupyterServerUriEntry) {
const provider = info.provider && (await this.jupyterUriProviderRegistration.getProvider(info.serverId));
if (!info.provider || !provider || !provider.getHandles) {
return false;
}

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

if (!handles.includes(idAndHandle.handle)) {
await this.serverUriStorage.removeMRU(uri);
if (!handles.includes(info.provider.handle)) {
await this.serverUriStorage.remove(info.uri);
}
return true;
} catch (_ex) {
Expand Down Expand Up @@ -366,15 +340,14 @@ export abstract class DataScienceErrorHandler implements IDataScienceErrorHandle
err instanceof InvalidRemoteJupyterServerUriHandleError
) {
this.sendKernelTelemetry(err, errorContext, resource, err.category);
const savedList = await this.serverUriStorage.getMRUs();
const message =
err instanceof InvalidRemoteJupyterServerUriHandleError
? ''
: err instanceof RemoteJupyterServerConnectionError
? err.originalError.message || ''
: err.originalError?.message || err.message;
const serverId = err instanceof RemoteJupyterServerConnectionError ? err.serverId : err.serverId;
const server = savedList.find((item) => item.serverId === serverId);
const server = await this.serverUriStorage.get(serverId);
const displayName = server?.displayName;
const baseUrl = err instanceof RemoteJupyterServerConnectionError ? err.baseUrl : '';
const idAndHandle =
Expand All @@ -386,7 +359,7 @@ export abstract class DataScienceErrorHandler implements IDataScienceErrorHandle
? this.extensions.getExtension(err.extensionId)?.packageJSON.displayName || err.extensionId
: '';
const options = actionSource === 'jupyterExtension' ? [DataScience.selectDifferentKernel] : [];
if (server && (await this.handleJupyterServerProviderConnectionError(server.uri))) {
if (server && (await this.handleJupyterServerProviderConnectionError(server))) {
return KernelInterpreterDependencyResponse.selectDifferentKernel;
}

Expand All @@ -401,13 +374,10 @@ export abstract class DataScienceErrorHandler implements IDataScienceErrorHandle
);
switch (selection) {
case DataScience.removeRemoteJupyterConnectionButtonText: {
// Start with saved list.
const uriList = await this.serverUriStorage.getMRUs();

// Remove this uri if already found (going to add again with a new time)
const item = uriList.find((f) => f.serverId === serverId);
const item = await this.serverUriStorage.get(serverId);
if (item) {
await this.serverUriStorage.removeMRU(item.uri);
await this.serverUriStorage.remove(item.uri);
}
// Wait until all of the remote controllers associated with this server have been removed.
return KernelInterpreterDependencyResponse.cancel;
Expand Down
25 changes: 10 additions & 15 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 @@ -794,7 +794,6 @@ Failed to run jupyter as observable with args notebook --no-browser --notebook-d
},
serverId
});
when(uriStorage.getMRUs()).thenResolve([]);
when(
applicationShell.showErrorMessage(anything(), anything(), anything(), anything(), anything())
).thenResolve();
Expand All @@ -816,7 +815,7 @@ Failed to run jupyter as observable with args notebook --no-browser --notebook-d
DataScience.selectDifferentKernel
)
).once();
verify(uriStorage.removeMRU(uri)).never();
verify(uriStorage.remove(uri)).never();
});
test('Display error when connection to remote jupyter server fails due to 3rd party extension', async () => {
const uri = generateUriFromRemoteProvider('1', 'a');
Expand All @@ -833,7 +832,7 @@ Failed to run jupyter as observable with args notebook --no-browser --notebook-d
},
serverId
});
when(uriStorage.getMRUs()).thenResolve([{ time: 1, uri, serverId, displayName: 'Hello Server' }]);
when(uriStorage.get(serverId)).thenResolve({ time: 1, uri, serverId, displayName: 'Hello Server' });
when(
applicationShell.showErrorMessage(anything(), anything(), anything(), anything(), anything())
).thenResolve();
Expand All @@ -855,7 +854,8 @@ Failed to run jupyter as observable with args notebook --no-browser --notebook-d
DataScience.selectDifferentKernel
)
).once();
verify(uriStorage.removeMRU(uri)).never();
verify(uriStorage.remove(uri)).never();
verify(uriStorage.get(serverId)).atLeast(1);
});
test('Remove remote Uri if user choses to do so, when connection to remote jupyter server fails', async () => {
const uri = 'http://hello:1234/jupyter';
Expand All @@ -875,11 +875,8 @@ Failed to run jupyter as observable with args notebook --no-browser --notebook-d
when(
applicationShell.showErrorMessage(anything(), anything(), anything(), anything(), anything())
).thenResolve(DataScience.removeRemoteJupyterConnectionButtonText as any);
when(uriStorage.removeMRU(anything())).thenResolve();
when(uriStorage.getMRUs()).thenResolve([
{ time: 1, serverId: 'foobar', uri: 'one' },
{ uri, serverId, time: 2 }
]);
when(uriStorage.remove(anything())).thenResolve();
when(uriStorage.get(serverId)).thenResolve({ uri, serverId, time: 2 });
const result = await dataScienceErrorHandler.handleKernelError(
error,
'start',
Expand All @@ -888,8 +885,8 @@ Failed to run jupyter as observable with args notebook --no-browser --notebook-d
'jupyterExtension'
);
assert.strictEqual(result, KernelInterpreterDependencyResponse.cancel);
verify(uriStorage.removeMRU(uri)).once();
verify(uriStorage.getMRUs()).atLeast(1);
verify(uriStorage.remove(uri)).once();
verify(uriStorage.get(serverId)).atLeast(1);
});
test('Change remote Uri if user choses to do so, when connection to remote jupyter server fails', async () => {
const uri = 'http://hello:1234/jupyter';
Expand All @@ -906,7 +903,6 @@ Failed to run jupyter as observable with args notebook --no-browser --notebook-d
},
serverId
});
when(uriStorage.getMRUs()).thenResolve([]);
when(
applicationShell.showErrorMessage(anything(), anything(), anything(), anything(), anything())
).thenResolve(DataScience.changeRemoteJupyterConnectionButtonText as any);
Expand All @@ -919,7 +915,7 @@ Failed to run jupyter as observable with args notebook --no-browser --notebook-d
'jupyterExtension'
);
assert.strictEqual(result, KernelInterpreterDependencyResponse.cancel);
verify(uriStorage.removeMRU(uri)).never();
verify(uriStorage.remove(uri)).never();
});
test('Select different kernel user choses to do so, when connection to remote jupyter server fails', async () => {
const uri = 'http://hello:1234/jupyter';
Expand All @@ -936,7 +932,6 @@ Failed to run jupyter as observable with args notebook --no-browser --notebook-d
},
serverId
});
when(uriStorage.getMRUs()).thenResolve([]);
when(
applicationShell.showErrorMessage(anything(), anything(), anything(), anything(), anything())
).thenResolve(DataScience.selectDifferentKernel as any);
Expand All @@ -948,7 +943,7 @@ Failed to run jupyter as observable with args notebook --no-browser --notebook-d
'jupyterExtension'
);
assert.strictEqual(result, KernelInterpreterDependencyResponse.selectDifferentKernel);
verify(uriStorage.removeMRU(uri)).never();
verify(uriStorage.remove(uri)).never();
});
function verifyErrorMessage(message: string, linkInfo?: string) {
message = message.includes('command:jupyter.viewOutput')
Expand Down
2 changes: 1 addition & 1 deletion src/kernels/jupyter/connection/jupyterConnection.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ export class JupyterConnection {

private async getUriFromServerId(serverId: string) {
// Since there's one server per session, don't use a resource to figure out these settings
const savedList = await this.serverUriStorage.getMRUs();
const savedList = await this.serverUriStorage.getAll();
return savedList.find((item) => item.serverId === serverId)?.uri;
}
private async createConnectionInfoFromUri(uri: string) {
Expand Down
13 changes: 0 additions & 13 deletions src/kernels/jupyter/connection/jupyterConnection.unit.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,19 +191,6 @@ suite('JupyterConnection', () => {
assert.equal(connection.hostName, expectedServerInfo.hostname);
assert.equal(connection.token, expectedServerInfo.token);
});
test('Disconnect event is fired in connection', async () => {
(<any>dsSettings).jupyterLaunchTimeout = 10_000;
const waiter = createConnectionWaiter();
observableOutput.next({ source: 'stderr', out: 'Jupyter listening on http://123.123.123:8888' });
let disconnected = false;

const connection = await waiter.waitForConnection();
connection.disconnected(() => (disconnected = true));

childProc.emit('exit', 999);

assert.isTrue(disconnected);
});
test('Throw timeout error', async () => {
(<any>dsSettings).jupyterLaunchTimeout = 10;
const waiter = createConnectionWaiter();
Expand Down
17 changes: 7 additions & 10 deletions src/kernels/jupyter/connection/jupyterRemoteCachedKernelValidator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
import { inject, injectable } from 'inversify';
import { traceWarning } from '../../../platform/logging';
import { LiveRemoteKernelConnectionMetadata } from '../../types';
import { extractJupyterServerHandleAndId } from '../jupyterUtils';
import {
IJupyterRemoteCachedKernelValidator,
IJupyterServerUriStorage,
Expand All @@ -29,29 +28,27 @@ export class JupyterRemoteCachedKernelValidator implements IJupyterRemoteCachedK
if (!this.liveKernelConnectionTracker.wasKernelUsed(kernel)) {
return false;
}
const providersPromise = this.providerRegistration.getProviders();
const currentList = await this.uriStorage.getMRUs();
const item = currentList.find((item) => item.serverId === kernel.serverId);
const item = await this.uriStorage.get(kernel.serverId);
if (!item) {
// Server has been removed and we have some old cached data.
return false;
}
// Check if this has a provider associated with it.
const info = extractJupyterServerHandleAndId(item.uri);
if (!info) {
if (!item.provider) {
// Could be a regular remote Jupyter Uri entered by the user.
// As its in the list, its still valid.
return true;
}
const providers = await providersPromise;
const provider = providers.find((item) => item.id === info.id);
const provider = await this.providerRegistration.getProvider(item.provider.id);
if (!provider) {
traceWarning(`Extension may have been uninstalled for provider ${info.id}, handle ${info.handle}`);
traceWarning(
`Extension may have been uninstalled for provider ${item.provider.id}, handle ${item.provider.handle}`
);
return false;
}
if (provider.getHandles) {
const handles = await provider.getHandles();
if (handles.includes(info.handle)) {
if (handles.includes(item.provider.handle)) {
return true;
} else {
traceWarning(
Expand Down
Loading