Fix client dying silently when a transport fails to start (#268) - #374
Open
FZambia wants to merge 6 commits into
Open
Fix client dying silently when a transport fails to start (#268)#374FZambia wants to merge 6 commits into
FZambia wants to merge 6 commits into
Conversation
The dependency resolution and selection logic in _initializeTransport is about to be extracted into helpers. Two of the five transports, sockjs and webtransport, had zero function coverage, so a mistake in moving their construction or supported() wiring would not have been detected. Covers selection and construction for sockjs and webtransport via fakes, equivalence between resolving a dependency from config and from globalThis, unsupported handling for each transport driven through the real resolution path, skipping an unsupported entry, and connecting via a string endpoint. No test depends on which globals the running Node version provides: CI spans Node 18-25 and globalThis.WebSocket is not present across that whole range, so a dependency that must be present is passed via config and one that must be absent is deleted and restored explicitly.
new WebSocket(url) is not total. Browsers throw SecurityError for a ws:// URL on an https:// page and for a URL blocked by the CSP connect-src directive, SyntaxError for a malformed URL, and a replaced global WebSocket throws for its own reasons. The connect timeout was armed before initialize(), so a throw left the wrapper's inner transport null with the timeout already scheduled: it fired transport.close() against null seconds later, from a timer frame carrying no URL, transport name or connect context. That is the TypeError reported in the issue. Worse than the log line, the client died. _transportClosed is set false just before initialize(), and with no socket nothing ever delivered onClose to reset it, so every later reconnect attempt bailed on the "waiting for transport close" guard and the client sat in connecting for the life of the page. With getToken configured the cause was also misreported: initialize() runs inside a promise callback there, so the SecurityError surfaced as a connectToken error, pointing diagnosis at the token subsystem. Failures during a connection attempt now go through one path that resets _transportClosed, advances the transport index, and emits the existing transport error carrying the exception's own message - so a CSP block names the directive that caused it, and a client configured with several transports falls through to the next one. Under connect-src 'self' that succeeds, since same-origin https is permitted while wss is not. The connect timeout uses the same path rather than calling close() and waiting for the transport to report back, so a transport that accepts close() silently no longer wedges the client, and disconnect() now clears that timeout instead of leaving it armed. Configuration faults that no retry could fix still throw, but now from connect() itself, before any state change and before the getToken hop, so they behave the same however the client is configured. Also fixed while restructuring the selection loop: it assigned this._transport before testing supported(), leaving the client holding a never-initialized wrapper when it gave up, and it wrapped the transport index only on entry, so an unsupported entry after a failed one could read past the end of the list.
EventSource fires nothing when closed, so SseTransport has to synthesize the close notification itself. It delivered that inline from close(), which made SSE the only transport re-entering the client from inside its own close(): _disconnect -> close() -> onClose -> _disconnect again, on one stack. Every other transport reports asynchronously, via onclose, an aborted fetch, or the WebTransport closed promise. That reentrancy is why _disconnect has to null this._transport before closing it. Defer the synthesized close and consume it one-shot. The deferred callback now arrives after _disconnect has advanced the transport id, so it is ignored on the id guard - nothing is lost, because every path that closes a transport already resets the closed state itself. The synthesis is kept rather than removed so this change stays independent of the connect timeout handling it now overlaps with.
Adds the WebtransportTransport close-before-initialize guard, and resolves eventsource, fetch and readableStream from globalThis as well as from config, so every transport is exercised through both resolution paths.
FZambia
force-pushed
the
fix/transport-initialize-failures
branch
from
August 5, 2026 07:26
01f7256 to
3724c18
Compare
Recovering has to leave a connection that actually works, not merely one that reports connected. These were written while validating the transport initialize fixes on this branch, and cover ground the existing suite did not: subscriptions across a transport failure and after a dropped socket, publish, history and presence once recovered, map subscriptions, falling back to an emulation transport and then genuinely using it, rotating through a three transport list, token callbacks, network offline/online, disconnecting from inside an error listener, and timer accounting after repeated failures.
FZambia
force-pushed
the
fix/transport-initialize-failures
branch
from
August 5, 2026 07:36
3724c18 to
6558a9f
Compare
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
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.
Fixes #268.
What was wrong
When the client creates a WebSocket, the browser can refuse outright and throw immediately, or hand back something that is not really a WebSocket. That happens more often than you would think:
window.WebSocket— this is what the original report used to reproduce ithttps://but the endpoint isws://(mixed content)connect-src 'self'does not allowws://orwss://, even to your own domain, and corporate proxies and browser extensions inject these headers into pages that never set themThe client was not prepared for that. It started a "did we connect in time?" timer before asking the browser for the socket. When the browser refused, the timer was already ticking. Seconds later it fired and tried to close a socket that either did not exist or was not a real one. Depending on which of the above you hit, that surfaces as:
Both are the same bug seen from two angles: the first when something replaced
WebSocketwith an object that has noclose, the second when the constructor threw and no object was created at all.The error in the logs was the small part. Two worse things happened:
The client stopped reconnecting, permanently and silently. It marked "a transport is open" and waited for that transport to report it had closed. Nothing was there to report, so every future reconnect attempt quietly gave up. The client sat in
connectingfor the rest of the page's life. For an app this looks like realtime just stopping — no live messages, no unread counters — with nothing visible to the user until they reload.The reason shown was the wrong one. If you use
getToken(most Centrifugo setups do), the failure surfaced as a token error. So the one clue you got pointed at the wrong part of your system. That is why the issue stayed open for over two years: one person spent two hours before discovering their real problem wasws://on anhttps://page.A note on the
?.workaroundSeveral people are carrying
this._transport?.close()as a patch-package patch. That fixes thenull is not an objectvariant, but not the one this issue is titled after. When an extension replacesWebSocket,_transportis not null — it is an object that simply has noclosemethod, so?.passes straight through and the call still throws. The guard here is atry/catch, which covers both. It also does not address the silent death, which is the more damaging half of the bug.What this changes
A transport that fails to start is now treated like any other failed connection attempt:
connect-src 'self'blockswss://but allows same-originhttps://, so if you havehttp_streamorsseconfigured, the client now switches to one of them and connects successfully. Before, it never got that far.The "did we connect in time?" timer was also asking the transport to report back after being told to close. A replaced
WebSocketthat quietly acceptsclose()and says nothing back would leave the client stuck forever — with no error anywhere. The timer now decides for itself that the attempt failed, so a transport that hangs, stays silent, or lacks the methods it should have can no longer wedge the client.Compatibility
No API changes. No new or changed options, methods, error types or error codes.
types.tsandcodes.tsare untouched.Two behaviour differences worth knowing about:
1. Configuration mistakes now fail loudly, in every setup. Things that no retry could ever fix — an
http://endpoint passed as a plain string, no WebSocket available, no usable transport in your list — throw fromconnect().That was already the behaviour when you do not use
getTokenorgetData. With them configured, the same mistake used to be reported as a token/data error and retried forever. It now throws, consistently.Practically: this only fires on a setup that could never connect for anyone. If you had it in production, nobody had realtime. But if you call
connect()at app startup without atry/catch, a broken config now surfaces as an exception instead of quiet retries, so it is worth a quick look at your bootstrap code.2. Connection problems now never throw from
connect(). The mirror of the above. A CSP or mixed-content failure used to escapeconnect()in setups withoutgetToken. It is now reported through theerrorevent like every other connection failure, and the client retries. If you were catching that exception, listen forerrorwithtype: 'transport'instead — you get the same message, plus the client actually recovers.Everything else is additive: you will see
errorevents withtype: 'transport'in situations that previously produced no event at all, because the client was dead.Also fixed along the way
Three related problems found while working in the same code:
[websocket, sockjs]in a browser without SockJS could read past the end of the transport list and crashdisconnect()left the connect timer running, so it kept firing after the client was goneSSE now reports its close on the next tick rather than in the middle of
close(), matching every other transport. It was the only one that called back into the client from inside its own close, which is why the disconnect code needs to null the transport before closing it.Testing
225 tests pass, up from 61.
Two new test files cover the failure modes end to end against a real Centrifugo, including the original reproduction from this issue: a replaced
WebSocketwhosecloseis missing entirely, one whoseclosethrows, and one that acceptsclose()and never reports back. Plus transports that refuse to start, falling back between transports, and recovering once the problem goes away.Two transports,
sockjsandwebtransport, had zero test coverage before this. They are now covered, which is what made it safe to move their setup code around. No file lost coverage.