Make registerSSE idempotent and stop re-processing from duplicating SSE state - #191
Conversation
htmx re-fires htmx:afterProcessNode whenever an element's attributes change, which is what a morph style swap does. Right before firing it, htmx wipes the element's internal data, so the extension lost its reference to the connection it had already opened and opened a second one. The first stayed open forever, holding a connection slot on the server and counting against the browser's per domain SSE limit. Keep the connection in a WeakMap that survives the internal data reset, so an element that is already connected to the same url keeps its stream instead of opening another one. When the element asks for a different connection, or drops sse-connect entirely, close the previous stream and rebind the descendants that were listening on it.
registerSSE always added a listener and never removed the previous one, and it could not: htmx wipes the element's internal data before re-firing htmx:afterProcessNode, so sseEventListener no longer referred to what had been registered earlier. An element processed twice ended up listening for the same event twice and swapped every message twice. Track the registered listeners in a WeakMap that survives the internal data reset, and drop them at the start of registerSSE. This also makes the onopen rebinding safe for descendants that were already registered against the new connection in the same htmx.process pass.
✅ Deploy Preview for htmx-extensions canceled.
|
|
This fixes a second bug too, and nothing here pins it.
|
The onopen rebinding walks the whole subtree with querySelectorAll, but registerSSE resolves every match against its own closest source. A descendant that sits under a nested sse-connect is therefore rebound onto the inner stream, which never dropped and is still holding the listener it registered at startup. Every reconnect of the outer stream stacked one more, and every message rendered one more time. Dropping the listeners at the start of registerSSE already covers this, but the nested fixtures only reach the initial subscription, so nothing would catch the day that call is removed as redundant. Reported against bigskysoftware#191 by jeffothy, who hit it in production: an app shell holding a user scoped stream, a chat panel nested inside holding its own, and messages doubling every time the device woke up.
The onopen rebinding walks the whole subtree with querySelectorAll, but registerSSE resolves every match against its own closest source. A descendant that sits under a nested sse-connect is therefore rebound onto the inner stream, which never dropped and is still holding the listener it registered at startup. Every reconnect of the outer stream stacked one more, and every message rendered one more time. Dropping the listeners at the start of registerSSE already covers this, but the nested fixtures only reach the initial subscription, so nothing would catch the day that call is removed as redundant. Reported against bigskysoftware#191 by jeffothy, who hit it in production: an app shell holding a user scoped stream, a chat panel nested inside holding its own, and messages doubling every time the device woke up. Claude-Session: https://claude.ai/code/session_01TrmeGu1jE9QSPZF92aGxVP
|
@jeffothy Thanks — you're right, and I verified it rather than taking it on trust. I wrote the test you described: outer One detail worth adding to your analysis, because it explains why this survived so long: for a I've also restructured the PR description to lead with this rather than with the morph |
a6dcaf4 to
692460b
Compare
Description
Two bugs, and the same non-idempotent
registerSSEunder both. The second one was found fromthe outside while this PR sat open (comment by @jeffothy,
hit in production). It is worth reading first, because it needs no morphing at all — it is
reachable by anyone whose connection drops once.
A reconnect stacks listeners on nested streams
onopenrebinds descendants after a retry:querySelectorAlltakes the whole subtree, butregisterSSEresolves each match against itsown closest source. For a plain subtree that is the same stream that just reconnected, and the
rebinding is correct: the descendants move to the new
EventSource, and whatever they hadregistered on the old one is inert, because a closed
EventSourcenever fires. That is why thishas been fine in the common case.
It stops being fine as soon as a descendant sits under a nested
sse-connect. Then itresolves to the inner stream — which never dropped, is still
OPEN, and is still holding thelistener it registered at startup. The outer reconnect adds a second one beside it. Every
subsequent message renders twice, then three times, once per outer reconnect.
@jeffothy'sreport: an app shell holding a user-scoped stream, a chat panel nested inside holding its own,
a phone that sleeps and wakes.
Nothing about this involves attribute changes or morphing. It is on
maintoday.Re-processing an element duplicates its state
Processing an element that carries
sse-connecta second time duplicated everything theextension had set up for it: it opened a second
EventSourceand orphaned the first, and itregistered the element's
sse-swaplisteners a second time. Full analysis and a reproduction ofthe connection leak are in #190.
Here the root cause is one level deeper. The extension keeps its state in the element's internal
data, and htmx clears that data in
deInitNode()right before it re-fireshtmx:afterProcessNode— so the extension has no reference left to either the connection or thelisteners it created earlier. This is also why the obvious guard, checking
api.getInternalData(elt).sseEventSourcebefore creating a new source, cannot work: it neversees anything.
htmx:afterProcessNodere-fires whenever an element's attribute hash changes, which is what amorph swap does — idiomorph preserves the node and mutates its attributes, then htmx
re-processes it. That combination has been supported since #165.
The fix
Both kinds of state move into
WeakMaps that survive the internal data reset.Listeners.
registerSSEdrops the listeners the element registered earlier beforeregistering it again, which makes it idempotent. That is the whole of the nested-reconnect fix,
and it covers both branches of
registerSSE(sse-swapandhx-trigger="sse:*"). It alsostops the dead
EventSourcefrom retaining listeners that close over the element.Connections.
ensureEventSourcenow behaves the wayensureEventSourceOnElement's doccomment already claims it does ("If a usable EventSource already exists, then it is returned"):
sse-connect(and samesse-close), notCLOSED— keep thestream, restore the reference in internal data, return. No reconnect, no dropped events.
sse-connectchanged — close the previous stream, open the new one, rebind descendants.sse-connectremoved — close the previous stream. This leaked unrecoverably before, sinceinternal data had already been wiped by then.
CLOSED(the normal retry path) — unchanged behaviour, and no eventis fired, so the existing backoff logic and its tests are untouched.
One more thing falls out: re-processing an element while a reconnect was pending used to produce
two connections, and the pending retry now finds the fresh connection and returns.
Notes on specific lines
htmx:beforeCleanupElementandmaybeCloseSSESourcenow share acloseEventSource()helper.That is not cosmetic: it has to fall back to the
WeakMap, because on the replacement paththe element's internal data has already been wiped, so a close that reads only internal data
finds nothing and silently skips. The helper also clears
internalData.sseEventSourceafterclosing, so
hasEventSource()stops reporting an already-closed connection as usable (theprevious code left it dangling — see the commented-out
// source = null).onopen's rebinding condition gainedreplacedSource ||, so descendants are also rebound whenthe stream was replaced by an
sse-connectchange rather than by a retry.var closeAttributemoved to the top ofensureEventSourcebecause the reuse check needs it,and the duplicated
api.getAttributeValue(elt, 'sse-connect')collapsed into one variable sothe new
elsebranch has something to attach to. Those are the only non-functional edits.One open question
Closing on
sse-connectchange or removal fireshtmx:sseClosewith a newdetail.typeofsourceReplaced. The docs list three values today (nodeReplaced,nodeMissing,message)and live in the
htmxrepo, so this needs a companion one-paragraph docs PR — happy to open it.Reusing
nodeReplacedinstead would avoid the cross-repo change, but it would be inaccurate:the node is still there, and handlers that treat
nodeReplacedas "this element is going away"would run teardown against a live element. Let me know which you prefer and I will adjust.
Htmx version: 2.0.4
Used extension(s) version(s): htmx-ext-sse 2.2.3
Corresponding issue: #190
Testing
11 new tests in
src/sse/test/ext/sse.js. As CONTRIBUTING asks, they were written first andfail against the current
main:The last one is the nested-reconnect case: outer
sse-connectaround an innersse-connectholding the
sse-swapelement,simulateConnectionError()on the outer, then assert the innerstream is still
OPEN, still has exactly one listener fore1, and that one event produces oneswap. It also fails with the
removeSSEListenerscall taken back out of the rest of this PR, so itpins that call specifically rather than the change as a whole. The existing nested
fixtures only ever reached the initial subscription, which is why nothing caught this.
One of the other new tests —
does not register duplicate listeners when the element is processed again, covering<div sse-connect sse-swap>where one element both owns the connection and is aswap target — passes against
mainfor the wrong reason: with two connections open, the mockonly ever fires an event on one of them, so the duplicate swap does not show up in the harness.
It fails against the connection fix alone (
2listeners instead of1), which is what it isthere to pin. The real-browser numbers below show the effect that the mock cannot.
With the whole change applied, all 39 pass (28 existing + 11 new):
beforeEachgained aneventSourcesarray so a test can count how many connections werecreated. The mock itself is unchanged.
Manual testing: I ran the real extension in headless Chrome against a small local SSE server
that counts open connections and broadcasts exactly one named event on request. The
sse-connectURL is deliberately relative — the test mock stores the URL verbatim, whereas areal
EventSourceresolves.urlto an absolute URL, so this is the one thing the suite cannotdemonstrate. Loading the page, then mutating an attribute and calling
htmx.process():Checklist
npm run test) and verified that it succeeded