Handle delayed Notebook provider registration (#3526)

* Handle delayed Notebook provider registration
- Fixes #3197 Notebooks: builtin provider always used on reopen with notebook file visible
- Fixes #3414 Can't refresh kernel after connect to big data cluster

There are 3 parts to this fix:
- If no notebook provider other than the default is installed, we warn users and prompt to install the SQL2019 extension
- We wait on the extension host registration to complete before determining which provider to use
- We know that the extension registration of the provider instance will be after package.json is read, so if we wait after registration for 10 seconds to give this a chance to happen before returning a provider to the front end

* Remove launch.json change that was added accidentally

* Fix timeout not being the expected value

* Removed console log left in during debugging

* Remove unnecessary whitespace

* Fix unit test failure

* Name the registration better, and remove outdated comments
This commit is contained in:
Kevin Cunnane
2018-12-07 17:56:21 -08:00
committed by GitHub
parent 96fb618390
commit e3bce7172c
7 changed files with 301 additions and 111 deletions

View File

@@ -9,12 +9,13 @@ import { IJSONSchema } from 'vs/base/common/jsonSchema';
import { ExtensionsRegistry, IExtensionPointUser } from 'vs/workbench/services/extensions/common/extensionsRegistry';
import { localize } from 'vs/nls';
import * as platform from 'vs/platform/registry/common/platform';
import { Event, Emitter } from 'vs/base/common/event';
export const Extensions = {
NotebookProviderContribution: 'notebook.providers'
};
export interface NotebookProviderDescription {
export interface NotebookProviderRegistration {
provider: string;
fileExtensions: string | string[];
}
@@ -54,42 +55,28 @@ let notebookContrib: IJSONSchema = {
};
export interface INotebookProviderRegistry {
registerNotebookProvider(provider: NotebookProviderDescription): void;
getSupportedFileExtensions(): string[];
getProviderForFileType(fileType: string): string;
readonly registrations: NotebookProviderRegistration[];
readonly onNewRegistration: Event<{ id: string, registration: NotebookProviderRegistration }>;
registerNotebookProvider(registration: NotebookProviderRegistration): void;
}
class NotebookProviderRegistry implements INotebookProviderRegistry {
private providerIdToProviders = new Map<string, NotebookProviderDescription>();
private fileToProviders = new Map<string, NotebookProviderDescription>();
private providerIdToRegistration = new Map<string, NotebookProviderRegistration>();
private _onNewRegistration = new Emitter<{ id: string, registration: NotebookProviderRegistration }>();
public readonly onNewRegistration: Event<{ id: string, registration: NotebookProviderRegistration }> = this._onNewRegistration.event;
registerNotebookProvider(provider: NotebookProviderDescription): void {
registerNotebookProvider(registration: NotebookProviderRegistration): void {
// Note: this method intentionally overrides default provider for a file type.
// This means that any built-in provider will be overridden by registered extensions
this.providerIdToProviders.set(provider.provider, provider);
if (provider.fileExtensions) {
if (Array.isArray<string>(provider.fileExtensions)) {
for (let fileType of provider.fileExtensions) {
this.addFileProvider(fileType, provider);
}
} else {
this.addFileProvider(provider.fileExtensions, provider);
}
}
this.providerIdToRegistration.set(registration.provider, registration);
this._onNewRegistration.fire( { id: registration.provider, registration: registration });
}
private addFileProvider(fileType: string, provider: NotebookProviderDescription) {
this.fileToProviders.set(fileType.toUpperCase(), provider);
}
getSupportedFileExtensions(): string[] {
return Array.from(this.fileToProviders.keys());
}
getProviderForFileType(fileType: string): string {
fileType = fileType.toUpperCase();
let provider = this.fileToProviders.get(fileType);
return provider ? provider.provider : undefined;
public get registrations(): NotebookProviderRegistration[] {
let registrationArray: NotebookProviderRegistration[] = [];
this.providerIdToRegistration.forEach(p => registrationArray.push(p));
return registrationArray;
}
}
@@ -97,15 +84,15 @@ const notebookProviderRegistry = new NotebookProviderRegistry();
platform.Registry.add(Extensions.NotebookProviderContribution, notebookProviderRegistry);
ExtensionsRegistry.registerExtensionPoint<NotebookProviderDescription | NotebookProviderDescription[]>(Extensions.NotebookProviderContribution, [], notebookContrib).setHandler(extensions => {
ExtensionsRegistry.registerExtensionPoint<NotebookProviderRegistration | NotebookProviderRegistration[]>(Extensions.NotebookProviderContribution, [], notebookContrib).setHandler(extensions => {
function handleExtension(contrib: NotebookProviderDescription, extension: IExtensionPointUser<any>) {
function handleExtension(contrib: NotebookProviderRegistration, extension: IExtensionPointUser<any>) {
notebookProviderRegistry.registerNotebookProvider(contrib);
}
for (let extension of extensions) {
const { value } = extension;
if (Array.isArray<NotebookProviderDescription>(value)) {
if (Array.isArray<NotebookProviderRegistration>(value)) {
for (let command of value) {
handleExtension(command, extension);
}

View File

@@ -26,10 +26,12 @@ export const DEFAULT_NOTEBOOK_FILETYPE = 'IPYNB';
export interface INotebookService {
_serviceBrand: any;
onNotebookEditorAdd: Event<INotebookEditor>;
onNotebookEditorRemove: Event<INotebookEditor>;
readonly onNotebookEditorAdd: Event<INotebookEditor>;
readonly onNotebookEditorRemove: Event<INotebookEditor>;
onNotebookEditorRename: Event<INotebookEditor>;
readonly isRegistrationComplete: boolean;
readonly registrationComplete: Promise<void>;
/**
* Register a metadata provider
*/
@@ -40,6 +42,10 @@ export interface INotebookService {
*/
unregisterProvider(providerId: string): void;
getSupportedFileExtensions(): string[];
getProviderForFileType(fileType: string): string;
/**
* Initializes and returns a Notebook manager that can handle all important calls to open, display, and
* run cells in a notebook.

View File

@@ -18,42 +18,142 @@ import { RenderMimeRegistry } from 'sql/parts/notebook/outputs/registry';
import { standardRendererFactories } from 'sql/parts/notebook/outputs/factories';
import { LocalContentManager } from 'sql/services/notebook/localContentManager';
import { SessionManager } from 'sql/services/notebook/sessionManager';
import { Extensions, INotebookProviderRegistry } from 'sql/services/notebook/notebookRegistry';
import { Extensions, INotebookProviderRegistry, NotebookProviderRegistration } from 'sql/services/notebook/notebookRegistry';
import { Emitter, Event } from 'vs/base/common/event';
import { Memento } from 'vs/workbench/common/memento';
import { IStorageService } from 'vs/platform/storage/common/storage';
import { IExtensionService } from 'vs/workbench/services/extensions/common/extensions';
import { IExtensionManagementService, IExtensionIdentifier } from 'vs/platform/extensionManagement/common/extensionManagement';
import { Disposable } from 'vs/base/common/lifecycle';
import { getIdFromLocalExtensionId } from 'vs/platform/extensionManagement/common/extensionManagementUtil';
import { Deferred } from 'sql/base/common/promise';
export interface NotebookProviderProperties {
provider: string;
fileExtensions: string[];
}
export class NotebookService implements INotebookService {
interface NotebookProviderCache {
[id: string]: NotebookProviderProperties;
}
interface NotebookProvidersMemento {
notebookProviderCache: NotebookProviderCache;
}
const notebookRegistry = Registry.as<INotebookProviderRegistry>(Extensions.NotebookProviderContribution);
class ProviderDescriptor {
private _instanceReady = new Deferred<INotebookProvider>();
constructor(private providerId: string, private _instance?: INotebookProvider) {
if (_instance) {
this._instanceReady.resolve(_instance);
}
}
public get instanceReady(): Promise<INotebookProvider> {
return this._instanceReady.promise;
}
public get instance(): INotebookProvider {
return this._instance;
}
public set instance(value: INotebookProvider) {
this._instance = value;
this._instanceReady.resolve(value);
}
}
export class NotebookService extends Disposable implements INotebookService {
_serviceBrand: any;
private _memento = new Memento('notebookProviders');
private _mimeRegistry: RenderMimeRegistry;
private _providers: Map<string, INotebookProvider> = new Map();
private _providers: Map<string, ProviderDescriptor> = new Map();
private _managers: Map<string, INotebookManager> = new Map();
private _onNotebookEditorAdd = new Emitter<INotebookEditor>();
private _onNotebookEditorRemove = new Emitter<INotebookEditor>();
private _onNotebookEditorRename = new Emitter<INotebookEditor>();
private _editors = new Map<string, INotebookEditor>();
private _fileToProviders = new Map<string, NotebookProviderRegistration>();
private _registrationComplete = new Deferred<void>();
private _isRegistrationComplete = false;
constructor() {
constructor(
@IStorageService private _storageService: IStorageService,
@IExtensionService extensionService: IExtensionService,
@IExtensionManagementService extensionManagementService: IExtensionManagementService
) {
super();
this._register(notebookRegistry.onNewRegistration(this.updateRegisteredProviders, this));
this.registerDefaultProvider();
if (extensionService) {
extensionService.whenInstalledExtensionsRegistered().then(() => {
this.cleanupProviders();
this._isRegistrationComplete = true;
this._registrationComplete.resolve();
});
}
if (extensionManagementService) {
this._register(extensionManagementService.onDidUninstallExtension(({ identifier }) => this.removeContributedProvidersFromCache(identifier, extensionService)));
}
}
private registerDefaultProvider() {
let defaultProvider = new BuiltinProvider();
this.registerProvider(defaultProvider.providerId, defaultProvider);
let registry = Registry.as<INotebookProviderRegistry>(Extensions.NotebookProviderContribution);
registry.registerNotebookProvider({
provider: defaultProvider.providerId,
fileExtensions: DEFAULT_NOTEBOOK_FILETYPE
});
private updateRegisteredProviders(p: { id: string; registration: NotebookProviderRegistration; }) {
let registration = p.registration;
if (!this._providers.has(p.id)) {
this._providers.set(p.id, new ProviderDescriptor(p.id));
}
if (registration.fileExtensions) {
if (Array.isArray<string>(registration.fileExtensions)) {
for (let fileType of registration.fileExtensions) {
this.addFileProvider(fileType, registration);
}
}
else {
this.addFileProvider(registration.fileExtensions, registration);
}
}
}
registerProvider(providerId: string, provider: INotebookProvider): void {
this._providers.set(providerId, provider);
registerProvider(providerId: string, instance: INotebookProvider): void {
let providerDescriptor = this._providers.get(providerId);
if (providerDescriptor) {
// Update, which will resolve the promise for anyone waiting on the instance to be registered
providerDescriptor.instance = instance;
} else {
this._providers.set(providerId, new ProviderDescriptor(providerId, instance));
}
}
unregisterProvider(providerId: string): void {
this._providers.delete(providerId);
}
get isRegistrationComplete(): boolean {
return this._isRegistrationComplete;
}
get registrationComplete(): Promise<void> {
return this._registrationComplete.promise;
}
private addFileProvider(fileType: string, provider: NotebookProviderRegistration) {
this._fileToProviders.set(fileType.toUpperCase(), provider);
}
getSupportedFileExtensions(): string[] {
return Array.from(this._fileToProviders.keys());
}
getProviderForFileType(fileType: string): string {
fileType = fileType.toUpperCase();
let provider = this._fileToProviders.get(fileType);
return provider ? provider.provider : undefined;
}
public shutdown(): void {
this._managers.forEach(manager => {
if (manager.serverManager) {
@@ -119,32 +219,59 @@ export class NotebookService implements INotebookService {
}
}
private sendNotebookCloseToProvider(editor: INotebookEditor) {
private sendNotebookCloseToProvider(editor: INotebookEditor): void {
let notebookUri = editor.notebookParams.notebookUri;
let uriString = notebookUri.toString();
let manager = this._managers.get(uriString);
if (manager) {
// As we have a manager, we can assume provider is ready
this._managers.delete(uriString);
let provider = this._providers.get(manager.providerId);
provider.handleNotebookClosed(notebookUri);
provider.instance.handleNotebookClosed(notebookUri);
}
}
// PRIVATE HELPERS /////////////////////////////////////////////////////
private doWithProvider<T>(providerId: string, op: (provider: INotebookProvider) => Thenable<T>): Thenable<T> {
private async doWithProvider<T>(providerId: string, op: (provider: INotebookProvider) => Thenable<T>): Promise<T> {
// Make sure the provider exists before attempting to retrieve accounts
let provider: INotebookProvider;
if (this._providers.has(providerId)) {
provider = this._providers.get(providerId);
}
else {
provider = this._providers.get(DEFAULT_NOTEBOOK_PROVIDER);
let provider: INotebookProvider = await this.getProviderInstance(providerId);
return op(provider);
}
private async getProviderInstance(providerId: string, timeout?: number): Promise<INotebookProvider> {
let providerDescriptor = this._providers.get(providerId);
let instance: INotebookProvider;
// Try get from actual provider, waiting on its registration
if (providerDescriptor) {
if (!providerDescriptor.instance) {
instance = await this.waitOnProviderAvailability(providerDescriptor);
} else {
instance = providerDescriptor.instance;
}
}
if (!provider) {
return Promise.reject(new Error(localize('notebookServiceNoProvider', 'Notebook provider does not exist'))).then();
// Fall back to default if this failed
if (!instance) {
providerDescriptor = this._providers.get(DEFAULT_NOTEBOOK_PROVIDER);
instance = providerDescriptor ? providerDescriptor.instance : undefined;
}
return op(provider);
// Should never happen, but if default wasn't registered we should throw
if (!instance) {
throw new Error(localize('notebookServiceNoProvider', 'Notebook provider does not exist'));
}
return instance;
}
private waitOnProviderAvailability(providerDescriptor: ProviderDescriptor, timeout?: number): Promise<INotebookProvider> {
// Wait up to 10 seconds for the provider to be registered
timeout = timeout || 10000;
let promises: Promise<INotebookProvider>[] = [
providerDescriptor.instanceReady,
new Promise<INotebookProvider>((resolve, reject) => setTimeout(() => resolve(), timeout))
];
return Promise.race(promises);
}
//Returns an instantiation of RenderMimeRegistry class
@@ -156,6 +283,41 @@ export class NotebookService implements INotebookService {
}
return this._mimeRegistry;
}
private get providersMemento(): NotebookProvidersMemento {
return this._memento.getMemento(this._storageService) as NotebookProvidersMemento;
}
private cleanupProviders(): void {
let knownProviders = Object.keys(notebookRegistry.registrations);
let cache = this.providersMemento.notebookProviderCache;
for (let key in cache) {
if (!knownProviders.includes(key)) {
this._providers.delete(key);
delete cache[key];
}
}
}
private registerDefaultProvider() {
let defaultProvider = new BuiltinProvider();
this.registerProvider(defaultProvider.providerId, defaultProvider);
notebookRegistry.registerNotebookProvider({
provider: defaultProvider.providerId,
fileExtensions: DEFAULT_NOTEBOOK_FILETYPE
});
}
private removeContributedProvidersFromCache(identifier: IExtensionIdentifier, extensionService: IExtensionService) {
let extensionid = getIdFromLocalExtensionId(identifier.id);
extensionService.getExtensions().then(i => {
let extension = i.find(c => c.id === extensionid);
if (extension && extension.contributes['notebookProvider']) {
let id = extension.contributes['notebookProvider'].providerId;
delete this.providersMemento.notebookProviderCache[id];
}
});
}
}
export class BuiltinProvider implements INotebookProvider {