Skip to content

Commit

Permalink
Minor refactoring and comments
Browse files Browse the repository at this point in the history
  • Loading branch information
DonJayamanne committed May 18, 2023
1 parent 81f7e9b commit 52d006c
Show file tree
Hide file tree
Showing 44 changed files with 257 additions and 270 deletions.
13 changes: 13 additions & 0 deletions src/gdpr.ts
Original file line number Diff line number Diff line change
Expand Up @@ -449,6 +449,18 @@
"${include}": [
"${F1}"
]
}
*/
//Telemetry.JupyterServerProviderResponseApi
/* __GDPR__
"DATASCIENCE.JUPYTER_SERVER_PROVIDER_RESPONSE_API" : {
"providerId": {"classification":"PublicNonPersonalData","purpose":"FeatureInsight","comment":"","owner":"donjayamanne"},
"extensionId": {"classification":"PublicNonPersonalData","purpose":"FeatureInsight","comment":"Extension Id that's attempting to use the API.","owner":"donjayamanne"},
"pemUsed": {"classification":"PublicNonPersonalData","purpose":"FeatureInsight","comment":"Name of the API member used.","owner":"donjayamanne"},
"${include}": [
"${F1}"
]
}
*/
Expand Down Expand Up @@ -1458,6 +1470,7 @@
"distro_version_id": {"classification":"SystemMetaData","purpose":"FeatureInsight","comment":"Linux distro version id.","owner":"donjayamanne"},
"errorMessage": {"classification":"CallstackOrException","purpose":"FeatureInsight","comment":"Error message when azure build module fails to load.","owner":"donjayamanne"},
"fallbackErrorMessage": {"classification":"CallstackOrException","purpose":"FeatureInsight","comment":"Error message when fallback module fails to load.","owner":"donjayamanne"},
"zmqBinaries": {"classification":"CallstackOrException","purpose":"FeatureInsight","comment":"List of binaries found on disc.","owner":"donjayamanne"},
"${include}": [
"${F1}"
Expand Down
12 changes: 6 additions & 6 deletions src/kernels/errors/kernelErrorHandler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,7 @@ export abstract class DataScienceErrorHandler implements IDataScienceErrorHandle
error: RemoteJupyterServerUriProviderError,
errorContext?: KernelAction
) {
const savedList = await this.serverUriStorage.getSavedUriList();
const savedList = await this.serverUriStorage.getMRUs();
const message = error.originalError?.message || error.message;
const serverId = error.serverId;
const displayName = savedList.find((item) => item.serverId === serverId)?.displayName;
Expand All @@ -253,7 +253,7 @@ export abstract class DataScienceErrorHandler implements IDataScienceErrorHandle
error: RemoteJupyterServerConnectionError,
errorContext?: KernelAction
) {
const savedList = await this.serverUriStorage.getSavedUriList();
const savedList = await this.serverUriStorage.getMRUs();
const message = error.originalError.message || '';
const serverId = error.serverId;
const displayName = savedList.find((item) => item.serverId === serverId)?.displayName;
Expand Down Expand Up @@ -305,7 +305,7 @@ export abstract class DataScienceErrorHandler implements IDataScienceErrorHandle
const handles = await provider.getHandles();

if (!handles.includes(idAndHandle.handle)) {
await this.serverUriStorage.removeUri(uri);
await this.serverUriStorage.removeMRU(uri);
}
return true;
} catch (_ex) {
Expand Down Expand Up @@ -366,7 +366,7 @@ export abstract class DataScienceErrorHandler implements IDataScienceErrorHandle
err instanceof InvalidRemoteJupyterServerUriHandleError
) {
this.sendKernelTelemetry(err, errorContext, resource, err.category);
const savedList = await this.serverUriStorage.getSavedUriList();
const savedList = await this.serverUriStorage.getMRUs();
const message =
err instanceof InvalidRemoteJupyterServerUriHandleError
? ''
Expand Down Expand Up @@ -402,12 +402,12 @@ export abstract class DataScienceErrorHandler implements IDataScienceErrorHandle
switch (selection) {
case DataScience.removeRemoteJupyterConnectionButtonText: {
// Start with saved list.
const uriList = await this.serverUriStorage.getSavedUriList();
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);
if (item) {
await this.serverUriStorage.removeUri(item.uri);
await this.serverUriStorage.removeMRU(item.uri);
}
// Wait until all of the remote controllers associated with this server have been removed.
return KernelInterpreterDependencyResponse.cancel;
Expand Down
24 changes: 12 additions & 12 deletions src/kernels/errors/kernelErrorHandler.unit.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -794,7 +794,7 @@ Failed to run jupyter as observable with args notebook --no-browser --notebook-d
},
serverId
});
when(uriStorage.getSavedUriList()).thenResolve([]);
when(uriStorage.getMRUs()).thenResolve([]);
when(
applicationShell.showErrorMessage(anything(), anything(), anything(), anything(), anything())
).thenResolve();
Expand All @@ -816,7 +816,7 @@ Failed to run jupyter as observable with args notebook --no-browser --notebook-d
DataScience.selectDifferentKernel
)
).once();
verify(uriStorage.removeUri(uri)).never();
verify(uriStorage.removeMRU(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 +833,7 @@ Failed to run jupyter as observable with args notebook --no-browser --notebook-d
},
serverId
});
when(uriStorage.getSavedUriList()).thenResolve([{ time: 1, uri, serverId, displayName: 'Hello Server' }]);
when(uriStorage.getMRUs()).thenResolve([{ time: 1, uri, serverId, displayName: 'Hello Server' }]);
when(
applicationShell.showErrorMessage(anything(), anything(), anything(), anything(), anything())
).thenResolve();
Expand All @@ -855,7 +855,7 @@ Failed to run jupyter as observable with args notebook --no-browser --notebook-d
DataScience.selectDifferentKernel
)
).once();
verify(uriStorage.removeUri(uri)).never();
verify(uriStorage.removeMRU(uri)).never();
});
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,8 +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.removeUri(anything())).thenResolve();
when(uriStorage.getSavedUriList()).thenResolve([
when(uriStorage.removeMRU(anything())).thenResolve();
when(uriStorage.getMRUs()).thenResolve([
{ time: 1, serverId: 'foobar', uri: 'one' },
{ uri, serverId, time: 2 }
]);
Expand All @@ -888,8 +888,8 @@ Failed to run jupyter as observable with args notebook --no-browser --notebook-d
'jupyterExtension'
);
assert.strictEqual(result, KernelInterpreterDependencyResponse.cancel);
verify(uriStorage.removeUri(uri)).once();
verify(uriStorage.getSavedUriList()).atLeast(1);
verify(uriStorage.removeMRU(uri)).once();
verify(uriStorage.getMRUs()).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 +906,7 @@ Failed to run jupyter as observable with args notebook --no-browser --notebook-d
},
serverId
});
when(uriStorage.getSavedUriList()).thenResolve([]);
when(uriStorage.getMRUs()).thenResolve([]);
when(
applicationShell.showErrorMessage(anything(), anything(), anything(), anything(), anything())
).thenResolve(DataScience.changeRemoteJupyterConnectionButtonText as any);
Expand All @@ -919,7 +919,7 @@ Failed to run jupyter as observable with args notebook --no-browser --notebook-d
'jupyterExtension'
);
assert.strictEqual(result, KernelInterpreterDependencyResponse.cancel);
verify(uriStorage.removeUri(uri)).never();
verify(uriStorage.removeMRU(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 +936,7 @@ Failed to run jupyter as observable with args notebook --no-browser --notebook-d
},
serverId
});
when(uriStorage.getSavedUriList()).thenResolve([]);
when(uriStorage.getMRUs()).thenResolve([]);
when(
applicationShell.showErrorMessage(anything(), anything(), anything(), anything(), anything())
).thenResolve(DataScience.selectDifferentKernel as any);
Expand All @@ -948,7 +948,7 @@ Failed to run jupyter as observable with args notebook --no-browser --notebook-d
'jupyterExtension'
);
assert.strictEqual(result, KernelInterpreterDependencyResponse.selectDifferentKernel);
verify(uriStorage.removeUri(uri)).never();
verify(uriStorage.removeMRU(uri)).never();
});
function verifyErrorMessage(message: string, linkInfo?: string) {
message = message.includes('command:jupyter.viewOutput')
Expand Down
2 changes: 1 addition & 1 deletion src/kernels/execution/lastCellExecutionTracker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ export class LastCellExecutionTracker extends Disposables implements IExtensionS
disposables.push(this);
}
public activate(): void {
this.serverStorage.onDidRemoveUris(this.onDidRemoveServerUris, this, this.disposables);
this.serverStorage.onDidRemove(this.onDidRemoveServerUris, this, this.disposables);
}
private getStateKey(serverId: string) {
return `LAST_EXECUTED_CELL_${serverId}`;
Expand Down
72 changes: 15 additions & 57 deletions src/kernels/jupyter/connection/jupyterConnection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@
// Licensed under the MIT License.

import { inject, injectable } from 'inversify';
import { IExtensionSyncActivationService } from '../../../platform/activation/types';
import { IDisposableRegistry } from '../../../platform/common/types';
import { noop } from '../../../platform/common/utils/misc';
import { RemoteJupyterServerUriProviderError } from '../../errors/remoteJupyterServerUriProviderError';
import { BaseError } from '../../../platform/errors/types';
Expand All @@ -15,7 +13,6 @@ import {
generateUriFromRemoteProvider
} from '../jupyterUtils';
import {
IJupyterServerUri,
IJupyterServerUriStorage,
IJupyterSessionManager,
IJupyterSessionManagerFactory,
Expand All @@ -26,37 +23,14 @@ import {
* Creates IJupyterConnection objects for URIs and 3rd party handles/ids.
*/
@injectable()
export class JupyterConnection implements IExtensionSyncActivationService {
private uriToJupyterServerUri = new Map<string, IJupyterServerUri>();
private pendingTimeouts: (NodeJS.Timeout | number)[] = [];
export class JupyterConnection {
constructor(
@inject(IJupyterUriProviderRegistration)
private readonly jupyterPickerRegistration: IJupyterUriProviderRegistration,
@inject(IJupyterSessionManagerFactory)
private readonly jupyterSessionManagerFactory: IJupyterSessionManagerFactory,
@inject(IDisposableRegistry)
private readonly disposables: IDisposableRegistry,
@inject(IJupyterServerUriStorage) private readonly serverUriStorage: IJupyterServerUriStorage
) {
disposables.push(this);
}
public activate() {
this.serverUriStorage.onDidChangeUri(
() =>
// When server URI changes, clear our pending URI timeouts
this.clearTimeouts(),
this,
this.disposables
);
}
public dispose() {
this.clearTimeouts();
}
private clearTimeouts() {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
this.pendingTimeouts.forEach((t) => clearTimeout(t as any));
this.pendingTimeouts = [];
}
) {}

public async createConnectionInfo(options: { serverId: string } | { uri: string }) {
const uri = 'uri' in options ? options.uri : await this.getUriFromServerId(options.serverId);
Expand All @@ -71,15 +45,12 @@ export class JupyterConnection implements IExtensionSyncActivationService {

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.getSavedUriList();
const savedList = await this.serverUriStorage.getMRUs();
return savedList.find((item) => item.serverId === serverId)?.uri;
}
private async createConnectionInfoFromUri(uri: string) {
// Prepare our map of server URIs
await this.updateServerUri(uri);

const server = await this.getJupyterServerUri(uri);
const idAndHandle = extractJupyterServerHandleAndId(uri);
const server = this.uriToJupyterServerUri.get(uri);
return createRemoteConnectionInfo(uri, server, idAndHandle?.id);
}

Expand All @@ -99,32 +70,19 @@ export class JupyterConnection implements IExtensionSyncActivationService {
}
}

private async updateServerUri(uri: string): Promise<void> {
private async getJupyterServerUri(uri: string) {
const idAndHandle = extractJupyterServerHandleAndId(uri);
if (idAndHandle) {
try {
const serverUri = await this.jupyterPickerRegistration.getJupyterServerUri(
idAndHandle.id,
idAndHandle.handle
);
this.uriToJupyterServerUri.set(uri, serverUri);
// See if there's an expiration date
if (serverUri.expiration) {
const timeoutInMS = serverUri.expiration.getTime() - Date.now();
// Week seems long enough (in case the expiration is ridiculous)
if (timeoutInMS > 0 && timeoutInMS < 604800000) {
this.pendingTimeouts.push(setTimeout(() => this.updateServerUri(uri).catch(noop), timeoutInMS));
}
}
} catch (ex) {
if (ex instanceof BaseError) {
throw ex;
}
const serverId = await computeServerId(
generateUriFromRemoteProvider(idAndHandle.id, idAndHandle.handle)
);
throw new RemoteJupyterServerUriProviderError(idAndHandle.id, idAndHandle.handle, ex, serverId);
if (!idAndHandle) {
return;
}
try {
return await this.jupyterPickerRegistration.getJupyterServerUri(idAndHandle.id, idAndHandle.handle);
} catch (ex) {
if (ex instanceof BaseError) {
throw ex;
}
const serverId = await computeServerId(generateUriFromRemoteProvider(idAndHandle.id, idAndHandle.handle));
throw new RemoteJupyterServerUriProviderError(idAndHandle.id, idAndHandle.handle, ex, serverId);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,6 @@ suite('Jupyter Connection', async () => {
jupyterConnection = new JupyterConnection(
instance(registrationPicker),
instance(sessionManagerFactory),
disposables,
instance(serverUriStorage)
);

Expand All @@ -56,17 +55,12 @@ suite('Jupyter Connection', async () => {
const serverConnectionChangeEvent = new EventEmitter<void>();
disposables.push(serverConnectionChangeEvent);

when(serverUriStorage.onDidChangeUri).thenReturn(serverConnectionChangeEvent.event);

jupyterConnection.activate();
when(serverUriStorage.onDidChange).thenReturn(serverConnectionChangeEvent.event);
});
teardown(() => {
disposeAllDisposables(disposables);
});

test('Ensure event handler is added', () => {
verify(serverUriStorage.onDidChangeUri).once();
});
test('Validation will result in fetching kernels and kernelspecs', async () => {
const uri = 'http://localhost:8888/?token=1234';
when(sessionManager.dispose()).thenResolve();
Expand Down
2 changes: 1 addition & 1 deletion src/kernels/jupyter/connection/jupyterPasswordConnect.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ export class JupyterPasswordConnect implements IJupyterPasswordConnect {
@inject(IDisposableRegistry) private readonly disposables: IDisposableRegistry
) {
// Sign up to see if servers are removed from our uri storage list
this.serverUriStorage.onDidRemoveUris(this.onDidRemoveUris, this, this.disposables);
this.serverUriStorage.onDidRemove(this.onDidRemoveUris, this, this.disposables);
}
private static _prompt?: Deferred<void>;
public static get prompt(): Promise<void> | undefined {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ export class JupyterRemoteCachedKernelValidator implements IJupyterRemoteCachedK
return false;
}
const providersPromise = this.providerRegistration.getProviders();
const currentList = await this.uriStorage.getSavedUriList();
const currentList = await this.uriStorage.getMRUs();
const item = currentList.find((item) => item.serverId === kernel.serverId);
if (!item) {
// Server has been removed and we have some old cached data.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,20 +33,12 @@ class MockProvider implements IJupyterUriProvider {
}
public async getServerUri(handle: string): Promise<IJupyterServerUri> {
if (handle === '1') {
const currentDate = new Date();
return {
// eslint-disable-next-line
baseUrl: 'http://foobar:3000',
token: '',
displayName: 'dummy',
authorizationHeader: { Bearer: this.currentBearer.toString() },
expiration: new Date(
currentDate.getFullYear(),
currentDate.getMonth(),
undefined,
currentDate.getHours(),
currentDate.getMinutes() + 1 // Expire after one minute
)
authorizationHeader: { Bearer: this.currentBearer.toString() }
};
}

Expand Down
Loading

0 comments on commit 52d006c

Please sign in to comment.