refactor(webview): avoid race conditions in constructors. #5961
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
This comment was marked as resolved.
This comment was marked as resolved.
Hweinstock
changed the title
jpinkney-aws
reviewed
Nov 8, 2024
| export class ApplicationComposer { | ||
| public readonly documentUri: vscode.Uri | ||
| public webviewPanel: vscode.WebviewPanel | ||
| public webviewPanel: vscode.WebviewPanel = {} as vscode.WebviewPanel |
There was a problem hiding this comment.
is there any reason this can't be public webviewPanel: vscode.WebviewPanel | undefined? If the webview panel is an empty object does that make some assumptions down the line that it might be defined, even though it isn't?
There was a problem hiding this comment.
Yeah, I wasn't sure if there was a better way to handle this. Since the constructor is private, the only way to instantiate ApplicationComposer is through the create method. Thus, this field will always be initiated. Perhaps there is a better way to enforce this invariant?
The advantage of an empty object is that I don't have to do an undefined check on each usage, but maybe if we can't enforce create being the sole entry point then this is necessary?
justinmk3
approved these changes
Nov 8, 2024
justinmk3
pushed a commit
that referenced
this pull request
Nov 8, 2024
## Problem It appears that older versions of VSCode are causing this test to be flaky, which appears to be fixed in v1.88. See findings section for more information. ## Solution - Skip the test if vscode version is pre-1.88. - Add tech-debt test to remove this check once min is 1.88+. ## Findings - Ran 1000 times in CI, and failed 29 times. (2.9% failure rate). If we re-reran 3 times, ci would pass ~ 99.998% of the time (assuming failures independent). - Only fails on VSCode minimum (1.83.0). - Originally thought it was related to activating Q extension or fetching it from CDN, but appears mocking this still results in flakiness. - Every failure comes with a `ERROR:gles2_command_buffer_stub.cc(364)] ContextResult::kFatalFailure Failed to create context.` which appears to be from OpenGL. This means it is coming from very far down the stack since VSCode is built on electron, which uses chromium, which is then using OpenGL. (ai suggests this is a GPU problem?). - Thought to be from race condition with async constructor hack, fixed by #5961. - Evidence is suggesting that this could be a bug in a prior version of VSCode, electron, or OpenGL, and should go away once we bump minimum VSCode version. - Works on VSCode 1.90+, 1.87- fails, 1.88 is the minimum version it is not flaky. (verified using 1000 attempts). - Failures are NOT independent since retrying 10x times only reduced failure rate to ~1%. Therefore retries are not a viable option.
JLargent42
approved these changes
Nov 8, 2024
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.
Problem
In the constructor for
ApplicationComposerManager, there is an async call (non-awaited). This call is modifying state within the object and therefore causes a race condition with any other function relying on this state. Seeaws-toolkit-vscode/packages/core/src/applicationcomposer/webviewManager.ts
Lines 30 to 33 in 8113288
This is potentially related to a flaky test.The test is still failing, but this refactor is still a win.Solution
createmethod with a private constructor. Move this pattern upward to enable async calls.License: I confirm that my contribution is made under the terms of the Apache 2.0 license.