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.

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

Minor refactoring and comments #13519

Merged
merged 2 commits into from
May 18, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
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