Skip to content

Commit

Permalink
Refactor URI storage provider (#13527)
Browse files Browse the repository at this point in the history 
* Refactor URI storage provider

* oops

* fix tests
  • Loading branch information
@DonJayamanne
DonJayamanne authored May 19, 2023
1 parent a089f78 commit 8e89c53
Show file tree
Hide file tree
Showing 45 changed files with 435 additions and 666 deletions.
2 changes: 0 additions & 2 deletions src/extension.node.ts
View file
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
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
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
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
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
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
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

0 comments on commit 8e89c53

Please sign in to comment.