fix(notebooks): fix race condition in view registration (#13106)

[bvenreply]

What it does

Fixes #13106

Fixes a race condition that occurred when event handlers for onDidRegisterNotebookSerializer were registered after said event was fired. This prevented the correct rendering of a notebook the first time one was opened during the lifetime of a page.

To solve this, we're storing the events that are fired before any event handler is registered and replay them for the first such handler only. Review comments highlighted this was not necessary and that there was a simpler solution

How to test

• Install the necessary vscode plugins:

  • ms-python.python (https://open-vsx.org/extension/ms-python/python)
  • ms-toolsai.jupyter (https://open-vsx.org/extension/ms-toolsai/jupyter)
  • vscode builtins (https://open-vsx.org/extension/eclipse-theia/builtin-extension-pack)

• Create a new file with extension .ipynb

• Open the file from the explorer

• Observe the notebook UI is shown immediately, as opposed to an empty editor before the patch.

Follow-ups

Review checklist

• As an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

• As a reviewer, I agree to behave in accordance with the review guidelines

[msujew]

This fix looks a bit overengineered to me. The only thing that is actually incorrect is the order in which we wait for things in waitForNotebookProvider. We can fix the issue by just reordering what we're doing in there:

protected async waitForNotebookProvider(type: string): Promise<NotebookProviderInfo | undefined> {
    if (this.notebookProviders.has(type)) {
        return this.notebookProviders.get(type);
    }

    const deferred = new Deferred<NotebookProviderInfo | undefined>();
    // 20 seconds of timeout
    const timeoutDuration = 20_000;

    const disposable = this.onDidRegisterNotebookSerializer(viewType => {
        if (viewType === type) {
            clearTimeout(timeout);
            disposable.dispose();
            deferred.resolve(this.notebookProviders.get(type));
        }
    });
    const timeout = setTimeout(() => {
        clearTimeout(timeout);
        disposable.dispose();
        deferred.reject(new Error(`Timed out while waiting for notebook serializer for type ${type} to be registered`));
    }, timeoutDuration);

    await Promise.all(this.willUseNotebookSerializerEmitter.fire(type));
    return deferred.promise;
}

The main issue seems to be that the provider is registered during the await Promise.all(this.willUseNotebookSerializerEmitter.fire(type)); call. We basically have a blind spot there. If we register the callback before that, we don't need the replay mechanism.

[bvenreply]

@msujew I can give that a try. Though those closure captures still need to be fixed.

[bvenreply]

@msujew You were right, that approach also works and it's a lot cleaner. Thank you. I force-pushed a new commit.

[msujew]

Alright, looks good to me 👍