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

Race condition in Editor Preview Manager and Editor Manager #9485

Closed
colin-grant-work opened this issue May 14, 2021 · 2 comments · Fixed by #9518
Closed

Race condition in Editor Preview Manager and Editor Manager #9485

colin-grant-work opened this issue May 14, 2021 · 2 comments · Fixed by #9518
Labels
bug bugs found in the application editor issues related to the editor editor-preview issues that are related to the editor-preview

Comments

@colin-grant-work
Copy link
Contributor

colin-grant-work commented May 14, 2021

Bug Description:

If two open requests come in nearly simultaneously, one asking for a preview widget and one asking for a non-preview widget, a race condition in the opening routines of the EditorPreviewManager and EditorManager can lead to an empty editor preview next to an open normal editor and a broken shell / widget tracking system.

Steps to Reproduce:

It's finicky

  1. Install the vscode-clangd plugin
  2. Find an #include directive pointing to an unambiguous filename in your workspace.
  3. Ctrl+Click that filename.
  4. You may (or may not) get bad behavior.

Alternatively:

  1. Add a command like this somewhere in the repo:
        commands.registerCommand({ id: 'bad-opener-behavior', label: 'Try to open an editor two ways.' }, {
            execute: () => {
                const uri = new URI('<path to Theia>/devfile.yaml');
                open(this.openerService, uri, { preview: true });
                open(this.openerService, uri);
            }
        });
  1. Run that command
  2. You may (or may not) see something like this:

image

What appears to be happening is that the EditorPreviewManager checks for the existence of an editor for the file its trying to open (line 124), and if it doesn't find it, it does some other async work (lines 137ff) before trying to open a new editor (line 139 or 146) for that file. If, between the failed check and the actual opening of a new editor, the EditorManager has separately handled a request to open the same file, the two openers can come into conflict.

async open(uri: URI, options: PreviewEditorOpenerOptions = {}): Promise<EditorPreviewWidget | EditorWidget> {
let widget = await this.pinCurrentEditor(uri, options);
if (widget) {
return widget;
}
widget = await this.replaceCurrentPreview(uri, options) || await this.openNewPreview(uri, options);
await this.editorManager.open(uri, options);
return widget;
}
protected async pinCurrentEditor(uri: URI, options: PreviewEditorOpenerOptions): Promise<EditorWidget | EditorPreviewWidget | undefined> {
if (await this.editorManager.getByUri(uri)) {
const editorWidget = await this.editorManager.open(uri, options);
if (editorWidget.parent instanceof EditorPreviewWidget) {
if (!options.preview) {
editorWidget.parent.pinEditorWidget();
}
return editorWidget.parent;
}
return editorWidget;
}
}
protected async replaceCurrentPreview(uri: URI, options: PreviewEditorOpenerOptions): Promise<EditorPreviewWidget | undefined> {
const currentPreview = await this.currentEditorPreview;
if (currentPreview) {
const editorWidget = await this.editorManager.getOrCreateByUri(uri);
currentPreview.replaceEditorWidget(editorWidget);
return currentPreview;
}
}

I've only consistently reproduced the behavior on branches with #9369, but I don't believe that the bug is actually related to that change.

Additional Information

  • Operating System: RHEL7
  • Theia Version: 1.13.0
@colin-grant-work colin-grant-work added bug bugs found in the application editor issues related to the editor editor-preview issues that are related to the editor-preview labels May 14, 2021
@kenneth-marut-work
Copy link
Contributor

I actually noticed this issue rather frequently while working on #9284, which predates #9369. Although I'm not sure the specific steps to reproduce, it seemed to pop up during the development cycle of watching and restarting Theia.

@colin-grant-work
Copy link
Contributor Author

Looks like this duplicates / is related to #8511.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug bugs found in the application editor issues related to the editor editor-preview issues that are related to the editor-preview
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants