-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Description
Summary
_onclose() in src/shared/protocol.ts has two related cleanup gaps that can cause issues when a Protocol instance is reused across connections via close() + connect().
Bug 1: _timeoutInfo not cleared in _onclose()
_onclose() methodically clears _responseHandlers, _progressHandlers, _taskProgressTokens, _pendingDebouncedNotifications, and _requestHandlerAbortControllers, but does not clear _timeoutInfo. Pending setTimeout callbacks from old requests survive close() and can fire cancel() against a new transport after reconnect.
Reproduction steps:
- Protocol connects to transportA. Client sends a request with messageId=5 and timeout=60000ms.
_setupTimeout(5, 60000, ...)stores a setTimeout callback in_timeoutInfo.- At t=30s,
close()is called._onclose()clears response handlers and aborts controllers, but_timeoutInfois untouched — the 60s timer keeps ticking. connect(transportB)is called.- At t=60s, the old timeout fires.
cancel()callsthis._transport?.send(notifications/cancelled)for requestId=5. Sincethis._transportnow points to transportB, a spurious cancellation notification is sent to the wrong transport.
Fix:
Iterate _timeoutInfo entries calling clearTimeout() for each, then clear the map in _onclose().
Bug 2: Stale .finally() can delete new connection's abort controller
In _onrequest, the fire-and-forget promise chain's .finally() does this._requestHandlerAbortControllers.delete(request.id). The request.id is captured from the closure, but there is no check that the stored controller matches.
Reproduction steps:
- Client A sends request id=1 on transportA. An AbortController (controllerA) is stored at
_requestHandlerAbortControllers.set(1, controllerA). The handler starts async work. close()is called._onclose()aborts controllerA and clears the map. The handler is aborted, but the promise chain continues (the error path runs).connect(transportB)is called.- Client B sends request id=1 (common since client IDs typically start from 0). A new AbortController (controllerB) is stored at
_requestHandlerAbortControllers.set(1, controllerB). - The old handler's promise chain from step 1 finally settles.
.finally()runs:this._requestHandlerAbortControllers.delete(1)— this deletes controllerB, not controllerA. - Request id=1 on connection B now has no abort controller in the map. If cancellation is attempted, the handler cannot be aborted.
Fix:
Capture a reference to the specific AbortController in the .finally() closure and only delete from the map if the stored value matches:
const controller = abortController;
promise.finally(() => {
if (this._requestHandlerAbortControllers.get(request.id) === controller) {
this._requestHandlerAbortControllers.delete(request.id);
}
});Impact
Both issues are pre-existing on v1.x. Practical impact is low because (1) spurious cancellations use stale request IDs that would be ignored, (2) Protocol reuse across different clients is uncommon. However, the issues become more relevant as close() + connect() becomes a supported reconnect pattern.