refactor: remove redundant initialized flag from DatabaseSessionService#660
refactor: remove redundant initialized flag from DatabaseSessionService#660MarvelNwachukwu wants to merge 2 commits intomainfrom
Conversation
The initPromise field already tracks initialization state, making the separate boolean flag unnecessary. Simplifies the deduplication logic.
🦋 Changeset detectedLatest commit: a8d7163 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the database session initialization logic by removing the initialized flag and streamlining the initializeDatabase and ensureInitialized methods to rely solely on the initPromise for promise deduplication. However, a critical race condition has been identified in the initializeDatabase method: if an initialization promise fails, its .catch() handler might incorrectly nullify a new initPromise if a subsequent initialization starts before the catch block executes, potentially leading to redundant initializations. The original logic for clearing initPromise on failure was more robust and should be reinstated.
| this.initPromise = this._doInitialize(); | ||
|
|
||
| // Clear the promise on failure to allow retries. | ||
| promise.catch(() => { | ||
| // Avoid race conditions if a new initialization has started. | ||
| if (this.initPromise === promise) { | ||
| this.initPromise = null; | ||
| } | ||
| this.initPromise.catch(() => { | ||
| this.initPromise = null; | ||
| }); | ||
|
|
||
| return promise; | ||
| return this.initPromise; |
There was a problem hiding this comment.
This implementation introduces a race condition when handling initialization failures. If an initialization promise fails, and a new initialization starts before the .catch() handler of the failed promise executes, the handler will incorrectly nullify the initPromise of the new initialization. This can lead to unexpected behavior and redundant initializations.
To fix this, you should capture the promise in a local variable and check if this.initPromise still refers to the same failed promise within the .catch() block before setting it to null. This was the behavior in the previous implementation and is crucial for correctness.
const promise = this._doInitialize();
this.initPromise = promise;
// Clear the promise on failure to allow retries.
promise.catch(() => {
// Avoid race conditions if a new initialization has started.
if (this.initPromise === promise) {
this.initPromise = null;
}
});
return promise;There was a problem hiding this comment.
Good catch on the defensive guard. In practice, JS's single-threaded microtask ordering means our .catch() (registered first) always fires before any caller's rejection handler could retry and reassign initPromise. So the race described can't actually occur in JS's event loop.
That said, the identity check is cheap and makes the intent explicit — restored it in a8d7163.
Restore the identity check in the .catch() handler to guard against theoretical race conditions. Add missing changeset for the PR.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the DatabaseSessionService by removing a redundant initialized flag and its associated checks, streamlining the database initialization process to rely solely on the initPromise for deduplication. The reviewer suggests restoring a comment explaining the purpose of the if (this.initPromise === promise) check in the initializeDatabase method's error handling, as its removal could negatively impact maintainability and potentially lead to future race condition bugs.
| // Clear the promise on failure to allow retries. | ||
| promise.catch(() => { | ||
| // Avoid race conditions if a new initialization has started. | ||
| if (this.initPromise === promise) { |
There was a problem hiding this comment.
The comment explaining this conditional check was removed. This check is crucial for handling a subtle race condition, and its purpose is not immediately obvious. Removing the comment harms maintainability, as a future developer might not understand its importance and could mistakenly remove it, introducing a bug.
Please restore a comment explaining why this check is necessary.
// This check prevents a race condition where a new initialization may have started
// before this catch block executes for a failed one.
if (this.initPromise === promise) {1 participant
Pull Request
Description
Removes the redundant
initializedboolean fromDatabaseSessionService. TheinitPromisefield already tracks initialization state — a non-null resolved promise serves the same purpose asinitialized = true. This simplifies the deduplication logic introduced in #646.Feedback from code review by @0x_Snake.
Related Issue
Follow-up to #646
Type of Change
How Has This Been Tested?
initializedis purely internal toDatabaseSessionService— only referenced in 3 places within the same file, all of which are replaced byinitPromisechecks. No behavioral change.Checklist