-
Notifications
You must be signed in to change notification settings - Fork 76
auth: Better than the old fix #2111
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
|
@copilot can you review this? |
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 refactors the event handling mechanism in VSCodeAzureSubscriptionProvider to improve concurrency safety and prevent infinite event loops. The changes implement a lazy listener pattern and add proper synchronization for account state checking.
- Introduces event emitters for sign-in/sign-out events with lazy listener initialization
- Adds promise-based synchronization to prevent race conditions in
accountsRemoved() - Consolidates duplicate event suppression flags into a single
suppressEventsvariable
Reviewed Changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| auth/src/VSCodeAzureSubscriptionProvider.ts | Refactored event handling with lazy listener initialization, added concurrency-safe accountsRemoved() method, and consolidated event suppression logic |
| auth/package.json | Bumped version from 5.1.0 to 5.1.1 |
| auth/package-lock.json | Updated lockfile to reflect new version |
| auth/CHANGELOG.md | Added release notes for version 5.1.1 |
Files not reviewed (1)
- auth/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@bwateratmsft I've opened a new pull request, #2112, to work on those changes. Once the pull request is ready, I'll request review from you. |
| private lastEventFired: number = 0; | ||
| private suppressEvents: boolean = false; |
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.
I ended up having to combine the timers/suppression. I can't make it work with two separate debounce timers--there's always some way things go wrong. However, I think it's fine--it's not likely the user is simultaneously signing in and signing out.
Same as #2110 but a better fix.
This is an easier way to understand what's happened, because it skips the added/reverted #2110:
27e1388...ece2793