-
Notifications
You must be signed in to change notification settings - Fork 222
Fix race condition in language client visibility notifications #4256
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
Conversation
Add isRunning() check before sending notifications to the language client to prevent "Client is not running" errors in test environments where the client may not have fully started yet. Co-authored-by: cklin <1418580+cklin@users.noreply.github.com>
|
This is not the right solution. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes an intermittent race condition in the CodeQL language client where visibility notifications were being sent before the language client finished starting, causing "Client is not running" errors in Windows tests.
Changes:
- Added an
isRunning()guard inCodeQLLanguageClient.notifyVisibilityChange()to prevent sending notifications before the client is fully started
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
I thought about it more and explored a different approach through #4274. Ideally, we should get to the bottom of the underlying race condition. But the editor visibility change is not really under our control, and there is not much point for the extension to notify itself of the visibility change if it (the extension) had already stopped running. So I think this is the best way to mitigate the flaky test. |
The
SourceMap › should jump to QL codetest was failing intermittently on Windows with "Client is not running" errors. ThenotifyVisibilityChange()method was sending notifications before the language client finished starting.Changes
Added an
isRunning()guard inCodeQLLanguageClient.notifyVisibilityChange():This matches the pattern used elsewhere in the codebase (e.g.,
extension.ts:201).Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.