fix: add timeout protection to ProviderSettingsManager initialization #8463
Conversation
This file contains hidden or 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
- Add ensureInitialized() method with configurable timeout (default 10s) - Implement initialization tracking to prevent multiple init attempts - Handle initialization errors gracefully to prevent infinite retries - Update constructor to call ensureInitialized() with error handling - Update tests to handle async initialization properly This prevents the extension from freezing indefinitely during startup when the ProviderSettingsManager initialization gets stuck. Fixes #8462
dosubot
bot
added
size:L
Closed
hannesrudolph
added
the
Issue/PR - Triage
![roomote[bot]](https://avatars.githubusercontent.com/in/1546624?s=60&v=4){kind=small}
Comment on lines
+121
to
+138
| const timeoutPromise = new Promise<never>((_, reject) => { | ||
| setTimeout(() => { | ||
| reject(new Error(`ProviderSettingsManager initialization timed out after ${timeoutMs}ms`)) | ||
| }, timeoutMs) | ||
| }) | ||
|
|
||
| const initPromise = this.initialize() | ||
|
|
||
| try { | ||
| await Promise.race([initPromise, timeoutPromise]) | ||
| this.isInitialized = true | ||
| } catch (error) { | ||
| console.error("ProviderSettingsManager initialization failed:", error) | ||
| // Set to initialized even on error to prevent infinite retries | ||
| this.isInitialized = true | ||
| throw error | ||
| } | ||
| } |
Contributor
Author
There was a problem hiding this comment.
[P1] Timeout promise can cause unhandled rejection and isInitialized is set true on error, allowing calls to bypass init. Suggest clearing the timer and not marking initialized on failure.
Suggested change
| const timeoutPromise = new Promise<never>((_, reject) => { | |
| setTimeout(() => { | |
| reject(new Error(`ProviderSettingsManager initialization timed out after ${timeoutMs}ms`)) | |
| }, timeoutMs) | |
| }) | |
| const initPromise = this.initialize() | |
| try { | |
| await Promise.race([initPromise, timeoutPromise]) | |
| this.isInitialized = true | |
| } catch (error) { | |
| console.error("ProviderSettingsManager initialization failed:", error) | |
| // Set to initialized even on error to prevent infinite retries | |
| this.isInitialized = true | |
| throw error | |
| } | |
| } | |
| let timer: ReturnType<typeof setTimeout> | undefined | |
| const timeoutPromise = new Promise<never>((_, reject) => { | |
| timer = setTimeout(() => { | |
| reject(new Error(`ProviderSettingsManager initialization timed out after ${timeoutMs}ms`)) | |
| }, timeoutMs) | |
| }) | |
| const initPromise = this.initialize() | |
| try { | |
| await Promise.race([initPromise, timeoutPromise]) | |
| this.isInitialized = true | |
| } catch (error) { | |
| console.error("ProviderSettingsManager initialization failed:", error) | |
| throw error | |
| } finally { | |
| if (timer) clearTimeout(timer) | |
| this.initializationPromise = null | |
| } |
| this.initialize().catch(console.error) | ||
| // Initialize asynchronously with timeout protection | ||
| this.ensureInitialized().catch((error) => { | ||
| console.error("ProviderSettingsManager initialization failed:", error) |
Contributor
Author
There was a problem hiding this comment.
[P2] Duplicate error logging and swallowed rejection. initializeWithTimeout already logs the failure; catching here logs again and swallows the rejection. Consider no-op catch to avoid duplicate logs while preventing unhandled rejection warnings.
Suggested change
| console.error("ProviderSettingsManager initialization failed:", error) | |
| this.ensureInitialized().catch(() => { | |
| // already logged inside initializeWithTimeout | |
| }) |
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.
Description
This PR fixes the extension freeze issue reported in #8462 where the extension would hang indefinitely during initialization, particularly affecting Windows users with WSL2.
Problem
The ProviderSettingsManager was calling an async
initialize()method in its constructor without proper timeout handling. If the initialization process got stuck (e.g., due to I/O issues or race conditions), the extension would freeze indefinitely with a spinner.Solution
ensureInitialized()method with configurable timeout (default 10 seconds)ensureInitialized()asynchronously with error handlingTesting
Impact
This change prevents the extension from freezing during startup while maintaining backward compatibility. The initialization is now non-blocking and will timeout after 10 seconds if it gets stuck, allowing the extension to continue functioning (with potential limitations) rather than hanging indefinitely.
Fixes #8462
Important
Adds timeout protection to
ProviderSettingsManagerinitialization to prevent indefinite hangs, ensuring idempotent and error-handled startup.ensureInitialized()method with a 10-second timeout toProviderSettingsManagerto prevent indefinite hangs during initialization.ensureInitialized()asynchronously with error handling.ensureInitialized()to ensure proper initialization.initializeWithTimeout()to handle initialization with timeout logic inProviderSettingsManager.ts.initializationPromiseandisInitializedflags.ProviderSettingsManager.spec.tsto handle async initialization.This description was created by
for a0a75fe. You can customize this summary. It will automatically update as commits are pushed.