Fixed race condition on monaco editor initialization #8563
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
federicobozzini
force-pushed
the
monaco-race-condition
branch
2 times, most recently
from
45cc46e to
ff354d6
Compare
Thanks, I've fixed it. It seems there is still some issue with the dependencies but I don't think it's related to this PR. |
Indeed - it's because of |
|
@federicobozzini For the sake of verification, could you provide us with some pseudo stack trace illustrating the race condition? I'd like to understand the chain of events that lead to the issue, and I am having trouble finding it on my end. |
|
The stacktrace of the files creation on the frontend side is: Click to expandThis is not too easy to debug because as far as I understand most of this process is not started explicitly but by a chain of object creations and inizialitazions managed by The source of this error seems to come from the fact that the |
|
I think I get a general sense of the issue thanks to your explanation. I don't like this lock (deferred) to be exposed and waiting for some other component to release. It works, but I was looking for a different way to address the issue: Is it possible to move the code on which the |
federicobozzini
force-pushed
the
monaco-race-condition
branch
from
ff354d6 to
e773f8d
Compare
|
@marechal-p I agree with you. I've changed the code to move the |
There was a problem hiding this comment.
LGTM.
@akosyakov you have more experience with the monaco integration, are we missing anything?
paul-marechal
force-pushed
the
monaco-race-condition
branch
from
9bb2437 to
b3ca800
Compare
paul-marechal
previously approved these changes
Oct 6, 2020
|
@marechal-p I've done some investigation and the problem with plugins doesn't seem to happen anymore. This PR should be now safe to merge. |
|
Please squash your commits, we can merge multi-commit PRs but the history has to stay clean from merge commits :) |
Signed-off-by: Federico Bozzini <[email protected]>
federicobozzini
force-pushed
the
monaco-race-condition
branch
from
c44111e to
ea8da32
Compare
|
@marechal-p It should now be ready. |
paul-marechal
approved these changes
Oct 14, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What it does
This PR fixes #8562. I think it was caused by a race condition in the Monaco Editor inizialization and this is one possible way to fix it.
How to test
To test try to run the browser version of Theia on windows, with a slow frontend contribution, and access Theia with a windows machine.
This fix prevents
MonacoTextModelServiceto run untilMonacoEditorProvideris properly initialized, preventing the issue from happening.Review checklist
Reminder for reviewers