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

Avoid validation of Jupyter Server providers #14081

Merged
merged 6 commits into from
Aug 8, 2023
Merged

Avoid validation of Jupyter Server providers #14081

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
aef31e2
Move display names into Jupyter Server Uri storage
DonJayamanne Aug 8, 2023
27acfd5
fix formatting
DonJayamanne Aug 8, 2023
5e4d1d7
Avoid validation of Jupyter Server providers
DonJayamanne Aug 8, 2023
647b05a
Stop using display name from Uri storage
DonJayamanne Aug 8, 2023
81ad41b
add error logging
DonJayamanne Aug 8, 2023
2a516b2
log error
DonJayamanne Aug 8, 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
51 changes: 13 additions & 38 deletions src/kernels/jupyter/connection/serverUriStorage.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,7 @@ import {
generateIdFromRemoteProvider,
getOwnerExtensionOfProviderHandle
} from '../jupyterUtils';
import {
IJupyterServerUriEntry,
IJupyterServerUriStorage,
IJupyterUriProviderRegistration,
JupyterServerProviderHandle
} from '../types';
import { IJupyterServerUriEntry, IJupyterServerUriStorage, JupyterServerProviderHandle } from '../types';
import { IFileSystem } from '../../../platform/common/platform/types';
import * as path from '../../../platform/vscode-path/resources';
import { noop } from '../../../platform/common/utils/misc';
Expand Down Expand Up @@ -65,8 +60,6 @@ export class JupyterServerUriStorage extends Disposables implements IJupyterServ
constructor(
@inject(IEncryptedStorage) encryptedStorage: IEncryptedStorage,
@inject(IMemento) @named(GLOBAL_MEMENTO) globalMemento: Memento,
@inject(IJupyterUriProviderRegistration)
private readonly jupyterPickerRegistration: IJupyterUriProviderRegistration,
@inject(IFileSystem)
fs: IFileSystem,
@inject(IExtensionContext)
Expand All @@ -80,7 +73,7 @@ export class JupyterServerUriStorage extends Disposables implements IJupyterServ
// eslint-disable-next-line @typescript-eslint/no-use-before-define
this.oldStorage = new OldStorage(encryptedStorage, globalMemento);
// eslint-disable-next-line @typescript-eslint/no-use-before-define
this.newStorage = new NewStorage(jupyterPickerRegistration, fs, storageFile, this.oldStorage);
this.newStorage = new NewStorage(fs, storageFile, this.oldStorage);
this.disposables.push(this._onDidAddUri);
this.disposables.push(this._onDidChangeUri);
this.disposables.push(this._onDidRemoveUris);
Expand All @@ -95,33 +88,26 @@ export class JupyterServerUriStorage extends Disposables implements IJupyterServ
this.newStorage.onDidChange((e) => this._onDidChangeUri.fire(e), this, this.disposables);
this.newStorage.onDidRemove((e) => this._onDidRemoveUris.fire(e), this, this.disposables);
}
public async getAll(skipValidation?: boolean): Promise<IJupyterServerUriEntry[]> {
public async getAll(): Promise<IJupyterServerUriEntry[]> {
this.hookupStorageEvents();
await this.newStorage.migrateMRU();
return this.newStorage.getAll(!skipValidation);
return this.newStorage.getAll();
}
public async clear(): Promise<void> {
this.hookupStorageEvents();
await this.newStorage.migrateMRU();
await Promise.all([this.oldStorage.clear(), this.newStorage.clear()]);
}
public async add(
jupyterHandle: JupyterServerProviderHandle,
options?: { time: number; displayName: string }
): Promise<void> {
public async add(jupyterHandle: JupyterServerProviderHandle, options?: { time: number }): Promise<void> {
this.hookupStorageEvents();
await this.newStorage.migrateMRU();
traceInfoIfCI(`setUri: ${jupyterHandle.id}.${jupyterHandle.handle}`);
const entry: IJupyterServerUriEntry = {
time: options?.time ?? Date.now(),
displayName: options?.displayName,
displayName: '',
provider: jupyterHandle
};

if (!options) {
const server = await this.jupyterPickerRegistration.getJupyterServerUri(jupyterHandle, true);
entry.displayName = server.displayName;
}
await this.newStorage.add(entry);
}
public async update(server: JupyterServerProviderHandle) {
Expand Down Expand Up @@ -217,8 +203,6 @@ class NewStorage {
private migration: Promise<void> | undefined;
private updatePromise = Promise.resolve();
constructor(
@inject(IJupyterUriProviderRegistration)
private readonly jupyterPickerRegistration: IJupyterUriProviderRegistration,
private readonly fs: IFileSystem,
private readonly storageFile: Uri,
private readonly oldStorage: OldStorage
Expand Down Expand Up @@ -305,7 +289,7 @@ class NewStorage {
.catch(noop));
}
public async update(server: JupyterServerProviderHandle) {
const uriList = await this.getAllImpl(false);
const uriList = await this.getAllImpl();

const existingEntry = uriList.find(
(entry) => entry.provider.id === server.id && entry.provider.handle === server.handle
Expand All @@ -323,7 +307,7 @@ class NewStorage {
public async remove(server: JupyterServerProviderHandle) {
await (this.updatePromise = this.updatePromise
.then(async () => {
const all = await this.getAllImpl(false);
const all = await this.getAllImpl();
if (all.length === 0) {
return;
}
Expand All @@ -350,17 +334,17 @@ class NewStorage {
})
.catch(noop));
}
public async getAll(validate = true): Promise<IJupyterServerUriEntry[]> {
return this.getAllImpl(validate).then((items) => items.sort((a, b) => b.time - a.time));
public async getAll(): Promise<IJupyterServerUriEntry[]> {
return this.getAllImpl().then((items) => items.sort((a, b) => b.time - a.time));
}
public async clear(): Promise<void> {
const all = await this.getAllImpl(false);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

false is the only value supported now.

const all = await this.getAllImpl();
await this.fs.delete(this.storageFile);
if (all.length) {
this._onDidRemoveUris.fire(all);
}
}
private async getAllImpl(validate = true): Promise<IJupyterServerUriEntry[]> {
private async getAllImpl(): Promise<IJupyterServerUriEntry[]> {
const data = await this.getAllRaw();
const entries: IJupyterServerUriEntry[] = [];

Expand All @@ -372,16 +356,7 @@ class NewStorage {
displayName: item.displayName || uri,
provider: item.serverHandle
};
if (!validate) {
entries.push(server);
return;
}
try {
await this.jupyterPickerRegistration.getJupyterServerUri(item.serverHandle, true);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is only required in one calling place, removed this and move the validation to the calling code.
Thereby removing a cycling reference and more layers.
This also makes listing of Jupyter Server much faster, as we no longer validate each server, which in turn waits for all related extensions to completely activate.

entries.push(server);
} catch {
//
}
entries.push(server);
})
);
return entries;
Expand Down
48 changes: 12 additions & 36 deletions src/kernels/jupyter/connection/serverUriStorage.unit.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,6 @@ suite('Server Uri Storage', async () => {
serverUriStorage = new JupyterServerUriStorage(
instance(encryptedStorage),
instance(memento),
instance(jupyterPickerRegistration),
instance(fs),
instance(context),
disposables
Expand Down Expand Up @@ -219,35 +218,30 @@ suite('Server Uri Storage', async () => {
.sort((a, b) => a.time - b.time)
.map((a) => {
return {
displayName: a.displayName,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

displayName is deprecated

uri: generateIdFromRemoteProvider(a.provider)
};
})
.sort((a, b) => (a.displayName || '').localeCompare(b.displayName || '')),
}),
itemsInNewStorage
.sort((a, b) => a.time - b.time)
.map((a) => {
return {
displayName: a.displayName,
uri: generateIdFromRemoteProvider(a.serverHandle)
};
})
.concat({
displayName: 'NewDisplayName1',
uri: generateIdFromRemoteProvider({
id: 'NewId1',
handle: 'NewHandle1',
extensionId: JVSC_EXTENSION_ID
})
})
.sort((a, b) => (a.displayName || '').localeCompare(b.displayName || ''))
);

assert.equal(onDidRemoveEvent.count, 0, 'Event should not be fired');
assert.equal(onDidAddEvent.count, 1, 'Event should be fired once');
assert.equal(onDidChangeEvent.count, 1, 'Event should be fired once');
});
test('Add new entry with time and display name', async () => {
test('Add new entry with time', async () => {
generateDummyData(2, true);
when(fs.exists(anything())).thenResolve(true);
when(fs.exists(uriEquals(globalStorageUri))).thenResolve(true);
Expand All @@ -263,12 +257,12 @@ suite('Server Uri Storage', async () => {

await serverUriStorage.add(
{ handle: 'NewHandle1', id: 'NewId1', extensionId: JVSC_EXTENSION_ID },
{ time: 1234, displayName: 'Sample Name' }
{ time: 1234 }
);
const all = await serverUriStorage.getAll();

verify(fs.writeFile(anything(), anything())).once();
assert.strictEqual(all.find((a) => a.displayName === 'Sample Name')?.time, 1234, 'Incorrect time');
assert.strictEqual(all.find((a) => a.provider.handle === 'NewHandle1')?.time, 1234, 'Incorrect time');
});
test('Add three new entries', async () => {
const itemsInNewStorage = generateDummyData(2, true);
Expand Down Expand Up @@ -314,43 +308,38 @@ suite('Server Uri Storage', async () => {
all
.map((a) => {
return {
displayName: a.displayName,
uri: generateIdFromRemoteProvider(a.provider)
};
})
.sort((a, b) => (a.displayName || '').localeCompare(b.displayName || '')),
.sort((a, b) => a.uri.localeCompare(b.uri)),
itemsInNewStorage
.map((a) => {
return {
displayName: a.displayName,
uri: generateIdFromRemoteProvider(a.serverHandle)
};
})
.concat({
displayName: 'NewDisplayName1',
uri: generateIdFromRemoteProvider({
id: 'NewId1',
handle: 'NewHandle1',
extensionId: JVSC_EXTENSION_ID
})
})
.concat({
displayName: 'NewDisplayName2',
uri: generateIdFromRemoteProvider({
id: 'NewId2',
handle: 'NewHandle2',
extensionId: JVSC_EXTENSION_ID
})
})
.concat({
displayName: 'NewDisplayName3',
uri: generateIdFromRemoteProvider({
id: 'NewId3',
handle: 'NewHandle3',
extensionId: JVSC_EXTENSION_ID
})
})
.sort((a, b) => a.displayName.localeCompare(b.displayName))
.sort((a, b) => a.uri.localeCompare(b.uri))
);

assert.equal(onDidRemoveEvent.count, 0, 'Event should not be fired');
Expand Down Expand Up @@ -403,43 +392,38 @@ suite('Server Uri Storage', async () => {
all
.map((a) => {
return {
displayName: a.displayName,
uri: generateIdFromRemoteProvider(a.provider)
};
})
.sort((a, b) => (a.displayName || '').localeCompare(b.displayName || '')),
.sort((a, b) => a.uri.localeCompare(b.uri)),
itemsInNewStorage
.map((a) => {
return {
displayName: a.displayName,
uri: generateIdFromRemoteProvider(a.serverHandle)
};
})
.concat({
displayName: 'NewDisplayName1',
uri: generateIdFromRemoteProvider({
id: 'NewId1',
handle: 'NewHandle1',
extensionId: JVSC_EXTENSION_ID
})
})
.concat({
displayName: 'NewDisplayName2',
uri: generateIdFromRemoteProvider({
id: 'NewId2',
handle: 'NewHandle2',
extensionId: JVSC_EXTENSION_ID
})
})
.concat({
displayName: 'NewDisplayName3',
uri: generateIdFromRemoteProvider({
id: 'NewId3',
handle: 'NewHandle3',
extensionId: JVSC_EXTENSION_ID
})
})
.sort((a, b) => a.displayName.localeCompare(b.displayName))
.sort((a, b) => a.uri.localeCompare(b.uri))
);

assert.equal(onDidRemoveEvent.count, 0, 'Event should not be fired');
Expand Down Expand Up @@ -493,35 +477,31 @@ suite('Server Uri Storage', async () => {
all
.map((a) => {
return {
displayName: a.displayName,
uri: generateIdFromRemoteProvider(a.provider)
};
})
.sort((a, b) => (a.displayName || '').localeCompare(b.displayName || '')),
.sort((a, b) => a.uri.localeCompare(b.uri)),
itemsInNewStorage
.map((a) => {
return {
displayName: a.displayName,
uri: generateIdFromRemoteProvider(a.serverHandle)
};
})
.concat({
displayName: 'NewDisplayName1',
uri: generateIdFromRemoteProvider({
id: 'NewId1',
handle: 'NewHandle1',
extensionId: JVSC_EXTENSION_ID
})
})
.concat({
displayName: 'NewDisplayName3',
uri: generateIdFromRemoteProvider({
id: 'NewId3',
handle: 'NewHandle3',
extensionId: JVSC_EXTENSION_ID
})
})
.sort((a, b) => a.displayName.localeCompare(b.displayName))
.sort((a, b) => a.uri.localeCompare(b.uri))
);

assert.equal(onDidRemoveEvent.count, 1, 'Event should be fired once');
Expand Down Expand Up @@ -573,35 +553,31 @@ suite('Server Uri Storage', async () => {
all
.map((a) => {
return {
displayName: a.displayName,
uri: generateIdFromRemoteProvider(a.provider)
};
})
.sort((a, b) => (a.displayName || '').localeCompare(b.displayName || '')),
.sort((a, b) => a.uri.localeCompare(b.uri)),
itemsInNewStorage
.map((a) => {
return {
displayName: a.displayName,
uri: generateIdFromRemoteProvider(a.serverHandle)
};
})
.concat({
displayName: 'NewDisplayName1',
uri: generateIdFromRemoteProvider({
id: 'NewId1',
handle: 'NewHandle1',
extensionId: JVSC_EXTENSION_ID
})
})
.concat({
displayName: 'NewDisplayName3',
uri: generateIdFromRemoteProvider({
id: 'NewId3',
handle: 'NewHandle3',
extensionId: JVSC_EXTENSION_ID
})
})
.sort((a, b) => a.displayName.localeCompare(b.displayName))
.sort((a, b) => a.uri.localeCompare(b.uri))
);

assert.equal(onDidRemoveEvent.count, 1, 'Event should be fired once');
Expand Down
8 changes: 4 additions & 4 deletions src/kernels/jupyter/finder/remoteKernelFinder.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import {
IOldJupyterSessionManagerFactory,
IJupyterRemoteCachedKernelValidator,
IRemoteKernelFinder,
IJupyterServerUriEntry
JupyterServerProviderHandle
} from '../types';
import { sendKernelSpecTelemetry } from '../../raw/finder/helper';
import { traceError, traceWarning, traceInfoIfCI, traceVerbose } from '../../../platform/logging';
Expand Down Expand Up @@ -95,13 +95,13 @@ export class RemoteKernelFinder implements IRemoteKernelFinder, IDisposable {
private readonly cachedRemoteKernelValidator: IJupyterRemoteCachedKernelValidator,
kernelFinder: KernelFinder,
private readonly kernelProvider: IKernelProvider,
readonly serverUri: IJupyterServerUriEntry,
readonly serverProviderHandle: JupyterServerProviderHandle,
private readonly jupyterConnection: JupyterConnection,
private readonly fs: IFileSystem,
private readonly context: IExtensionContext
) {
this.cacheFile = Uri.joinPath(context.globalStorageUri, RemoteKernelSpecCacheFileName);
this.cacheKey = generateIdFromRemoteProvider(serverUri.provider);
this.cacheKey = generateIdFromRemoteProvider(serverProviderHandle);
// When we register, add a disposable to clean ourselves up from the main kernel finder list
// Unlike the Local kernel finder universal remote kernel finders will be added on the fly
this.disposables.push(kernelFinder.registerKernelFinder(this));
Expand Down Expand Up @@ -260,7 +260,7 @@ export class RemoteKernelFinder implements IRemoteKernelFinder, IDisposable {
disposables.push(KernelProgressReporter.createProgressReporter(undefined, DataScience.connectingToJupyter));
}
return this.jupyterConnection
.createConnectionInfo(this.serverUri.provider)
.createConnectionInfo(this.serverProviderHandle)
.finally(() => disposeAllDisposables(disposables));
}

Expand Down
Loading
Loading