fix: close #216 — track and unregister Elements owned by ui.Template - #221
Merged
Conversation
A wrapped ui.Template re-registered its nested Elements on every update and
never unregistered the previous ones. Template.JawsUpdate executes into a
builder and queues SetInner, which replaces the wrapper's entire inner DOM, so
every Element rendered inside it during the previous execution is dead while
staying in rq.elems and rq.tagMap for the Request's lifetime.
The browser hid most of this: jawsRemoving acks an Inner by reporting the ids
of the descendants it removed, and handleRemove unregisters them. That ack can
only name DOM nodes carrying id="Jid.N", so the leak was exact for Elements
with no DOM identity of their own — an unwrapped nested template, which
ui.Template and RequestWriter.Template both document as supported, being the
direct case. Repeated updates grew the element registry and tag routing state
without bound, and tag broadcasts increasingly walked stale Elements.
RequestWriter gains an unexported elementRendered hook, called by NewUI with
each Element it rendered, and ui.Template becomes a pointer widget that uses it
to track the Elements its execution creates. It is unexported because
RequestWriter is embedded in With: an exported func field would be callable as
{{call $.ElementRendered $.Element}}, letting a template make an Element own
itself and have its own wrapper unregistered on the next update.
A successful update unregisters the previous generation along with the DOM that
held it. A failed execute unregisters what it created and restores the previous
set, which still matches the untouched DOM, so the next successful update
reclaims it rather than leaking it; a lookup failure returns before the set is
detached. Initial-render failures, including a failed closing-tag write, drop
the nested UI the failed execution had already created.
ContainerHelper participates through the same unexported elementOwner
interface, so template/container nesting reconciles in both directions:
a replaced container Element takes its children with it, and a removed
container child releases the nested UI it owned. Its removals collect
descendants only once Element.Remove has actually applied — Remove can reject
the operation and leave the DOM alone — and its failed-append cleanup stays
ahead of MustLog, which panics when no logger is configured.
Request.DeleteElements unregisters a whole subtree in one pass over the
registry, using the batched pattern handleRemove already uses. A per-descendant
DeleteElement retakes rq.mu and rescans rq.elems and every tagMap slice, which
is quadratic in the size of the generation being replaced.
BenchmarkTemplateUpdateOwnedCleanup (1000 nested Elements, 3 extra wrapper
tags, arm64, -count=6):
per-element DeleteElement -> batched DeleteElements
sec/op 7.923m ± 2% -> 3.174m ± 11% -59.94% (p=0.002 n=6)
allocs/op 26.79k ± 0% -> 26.79k ± 0% ~ (p=0.015 n=6)
base (no cleanup, leaks a generation per update) -> this change
sec/op 2.553m ± 1% -> 3.174m ± 11% +24.33% (p=0.002 n=6)
B/op 1.001Mi ± 0% -> 1.131Mi ± 0% +12.92% (p=0.002 n=6)
allocs/op 25.77k ± 0% -> 26.79k ± 0% +3.95% (p=0.002 n=6)
The second comparison is the cost of doing the cleanup at all, measured against
a revision that does none.
Breaking: NewTemplate returns *Template and a ui.Template value no longer
satisfies jaws.UI, so one Template backs one live Element like every other
stateful widget in the package. Callers passing NewTemplate(...) straight to
NewUI or NewElement are unaffected; ui.Template{...} literals used as UI need
&. Containers whose JawsContains builds a fresh *Template per call no longer
reuse child Elements, matching every pointer widget today, and a Template with
a non-comparable Dot can no longer be hashed as a container pool key.
Two paths are deliberately left alone, neither a regression: Register and
RadioGroup create Elements through Request.NewElement directly, bypassing
NewUI, and an owner deleted out-of-band (jw.Delete, jw.SetInner, a user
Element.Remove) still leaves its DOM-less descendants registered, since only
jaws core observes those deletions.
…ontract
The UI contract said a typed nil "is usable, and tolerating a nil receiver is
the concrete type's responsibility", which reads either as core accepting one
and leaving the consequences to the type, or as an obligation on every type to
survive one. No widget in lib/ui satisfies the second reading: *Span,
*Container, *JsVar and the rest dereference their fields, so the sentence
described an obligation the package never met.
Say which it is. Usable covers the map-key requirements, since a typed nil is
comparable and equal to itself, and means JaWS dispatches render, update and
event calls to it like any other value; only a nil UI interface is a no-op.
Surviving such a call is a property of the concrete type, so passing a nil
pointer of a type that does not document tolerance is a caller error rather
than a framework-handled case. Element.JawsRender and NewErrUnusableUI carried
the same ambiguous phrasing and are aligned with it.
Package ui now states its own position: its widgets dereference their fields
and none document nil-receiver tolerance, with the zero value (&Template{})
as the supported empty form. That replaces the equivalent note that lived only
in a comment in template.go.
The ownership documentation said a template's nested UI and the elements a failed execution registered are unregistered, without qualification. Ownership is recorded only through RequestWriter.NewUI, and two RequestWriter helpers do not go through it: Register and RadioGroup create their Elements with Request.NewElement directly, so a template neither owns nor reclaims them. An audit of the 32 RequestWriter methods confirms those two are the only widget helpers that bypass NewUI; the remaining non-NewUI methods are accessors. Narrow the claims to the Elements created through NewUI — which every RequestWriter widget helper, including a nested Template, does use — and state the exclusion where it is read: on Template, on RequestWriter.Register, on RequestWriter.RadioGroup, and in the lib/ui README. An Element either helper creates inside a template body outlives the content it belongs to and every re-render adds another, though the browser still reports the removal of one whose generated id reached the DOM. TestTemplate_UpdateDoesNotTrackRegisterOrRadioGroup pins that behavior, so a later change routing either helper through the hook fails a test that names the docs to update with it. Also correct two overreaching statements about lib/ui in the core contract and the skill: Option and Register are value types, so jaws.UI now says the pointer-valued widgets dereference their fields, and the skill's typed-nil rule matches the clarified contract instead of requiring nil-receiver tolerance.
…uest end Saying Register and RadioGroup Elements stay registered for the Request lifetime contradicted the next sentence in the same paragraph, which noted the browser still reports their removal. The browser is in fact the whole mechanism: applying a wrapper's new inner content makes jawsRemoving report every descendant carrying a JaWS id, and handleRemove unregisters those Elements. Not being tracked by the template means their cleanup depends on that acknowledgement, not that they survive to the end of the Request. Only an Element whose id never reaches the DOM has nothing to report it: one from an execution that failed before delivering its markup, or a Register call whose returned Jid the template discards. State that as the persisting case, which also matches the unrendered-radio note already on RadioElement. Also stop calling NewUI the path of "every RequestWriter widget helper", since Register and RadioGroup are widget helpers that do not take it. It is the path of every widget helper except those two. The characterization test measures server-side growth with no client attached, so its comment now says that is the point rather than implying no cleanup exists.
Owner
Author
|
Filed #222 for the |
linkdata
added a commit
that referenced
this pull request
Aug 3, 2026
…223) * fix: close #222 — track the Elements Register and RadioGroup create RequestWriter.Register and the lazy radio/label creation behind RadioGroup called Request.NewElement directly, so they were the only widget helpers a ui.Template did not track. A template that updates registered a fresh set on every execution and unregistered none, leaving the browser's removal acknowledgement as the only cleanup — and that can name only ids which reached the DOM. Growth was unbounded for every shape where one does not: a $.Register whose returned Jid is discarded or printed as text, a RadioElement.Label rendered without its Radio (which leaves the radio Element created but never rendered), and an execution that fails before its markup is delivered. Stale Elements also keep answering tag lookups, so Dirty(updater) fans out to ids absent from the DOM, where the client throws. Both helpers now report their Elements to the writer's owner, so the Template that rendered them reclaims them like any other nested UI. Reporting happens at creation rather than after a successful render, and the hook is renamed elementCreated to say so. That is what covers the unrendered radio: the Element exists because its Jid supplies the group's name= and the label's for=, and it may never render. The cost is that an owner's set can hold an Element whose render failed and which NewUI already unregistered, which is free — Request.DeleteElements skips elements it finds unregistered and every rollback deletes the whole set at once. The hook loses its error return. Its sole implementation cannot fail, and neither new call site could propagate one: Register returns a jid.Jid and Radio/Label return template.HTML, so both would have had to swallow it or route it through MustLog, which panics when no logger is configured. Neither helper routes through NewUI, and the reason differs per helper, so each carries its own comment. NewUI renders, and Element.JawsRender appends a debug comment when Jaws.Debug is set; Register's documented usage puts the returned Jid inside an attribute, where that comment would corrupt the markup. Radio and Label instead return their HTML for the template to place, and need the Element before that render for the name= and for= attributes. Ownership follows the RequestWriter that RadioGroup was called on, which is the template whose body called it rather than the wrapper the markup lands in. Those differ only when RadioElement values cross a template boundary in the dot; RadioGroup's doc states the condition, and a test pins it by updating the nested wrapper alone and asserting the group survives, since an outer update cannot distinguish the two attributions. Docs updated where #221 recorded the exception: Template, Register, RadioGroup, RadioElement, the lib/ui README and the tracked skill. The characterization test that asserted the growth becomes TestTemplate_UpdateTracksRegisterAndRadioGroup, now covering the discarded-Jid and label-only shapes, alongside new tests for execution failing after either helper ran and for the attribution boundary. * docs: do not promise request-lifetime registration without a template owner Register and RadioElement said an Element with no template owner stays registered until the Request ends. Losing the owner only removes one cleanup path: the browser still reports the JaWS ids it removes when an ancestor's content is replaced, and Element.Remove unregisters a managed child outright. Say that cleanup falls to the ordinary DOM-removal handling in that case, and that only an Element no removal ever reports — one whose Jid never becomes an element id — necessarily lasts until request teardown. For the radio Element a Label leaves behind, that condition is met by construction: it has no DOM node for any removal to report, so the original claim holds there and now says why.
linkdata
added a commit
that referenced
this pull request
Aug 4, 2026
* fix: keep ui.Template a value, holding its state on the Element #221 gave ui.Template per-Element ownership state, which forced it to become a pointer. ContainerHelper.reconcile keys its reuse pool on childElem.UI() — value equality — so a container whose JawsContains rebuilds ui.NewTemplate(...) children on every call stopped matching the pool. Every update removed and re-appended the whole collection: new Jids, full DOM churn, browser work proportional to the collection rather than to the change. The Element is the natural place for state keyed to an Element, so core gains one slot for it and Template goes back to being a plain comparable value. jaws.ElementState and jaws.SetElementState reach a single any field on Element, guarded by Request.mu. Loading and claiming are separate so contention is reported rather than absorbed: a second claim returns ErrElementStateClaimed even for state of the same type, since same type does not mean same owner, and a get-or-create would silently let a second Template mutate the first one's generation. A nil interface returns ErrElementStateNil and stores nothing, because nil is how an unclaimed slot is represented; that check precedes the occupancy check. A typed nil is a non-nil interface and does claim the slot. Both functions are package-level rather than methods: ui.With embeds *Element and ui.RequestWriter, so any method returning a value is callable from a template, and {{$.Element.SetElementState $.Dot}} would let a template claim the slot out from under its renderer. Claiming happens only in JawsRender, before tag registration, handler registration and any write, so a contended Element fails having changed nothing. Template.render and pageTemplate.JawsRender each claim and pass the state to execute, which takes it as a parameter precisely so a second entry point cannot silently skip the claim — pageTemplate bypasses render, and would otherwise track nothing at all. Template.JawsUpdate only loads, reporting ErrElementStateUnclaimed for an Element no Template rendered, and lib/ui's ownership walk looks in the slot as well as on the UI value, matching *templateState specifically: the slot may legitimately hold a typed nil that satisfies elementOwner through a promoted method, which a broader assertion would dereference. Two released behaviours narrow, both because Template was previously stateless. Rendering the same or another Template twice onto one Element now fails the second claim; combine the partials under one claiming Template, or give each its own Element, which a nested {{$.Template ...}} already does. And a wrapped Template updating an Element it never rendered is no longer silent — it reports through MustLog, which panics when no Jaws.Logger is configured — so a wrapped Template cannot be a Register updater. An unwrapped Template still can: its updates are a documented no-op, though only RequestWriter.Register also delivers its event handlers, since Register embeds jaws.Updater and promotes no handler methods. BenchmarkContainerOfTemplatesUpdate, 200 rebuilt children with an unchanged collection, arm64, -benchtime=200x -count=6: sec/op 1220.08µ ± 3% -> 73.62µ ± 35% -93.97% (p=0.002 n=6) B/op 194.93Ki ± 0% -> 28.48Ki ± 0% -85.39% (p=0.002 n=6) allocs/op 4046.0 ± 0% -> 406.0 ± 0% -89.97% (p=0.002 n=6) Costs, both expected and measured rather than assumed. Every Element pays two words for the slot, which BenchmarkElementCreateBatch shows as +8.17% B/op over a 64-element batch (16 bytes each) with time and allocation count unchanged. Each rendered Template allocates its state, visible as +3.7% allocs/op in the 1000-nested-child cleanup benchmark, with sec/op unchanged. Containers whose children are already stable are unaffected. * fix: clarify Template value contract and stabilize test * fix: correct the Element-cost measurement, test contracts and docs Review follow-ups to the Template value change. No production behaviour changes; the benchmark, the tests and the documentation do. BenchmarkElementCreateBatch never called b.ResetTimer, so building the Jaws and the Request was divided by b.N into every reported figure, and the deferred Close ran after the loop's final b.StartTimer. Both time and allocations were affected, and the byte figure depended on b.N rather than on the Element: -benchtime 200x 2000x 20000x before 13540 5965 5204 B/op after 5127 5121 5120 B/op Against the merge base the honest numbers are B/op 4.000Ki -> 5.000Ki (+25.00%, p=0.002 n=6) and sec/op 2.077µ -> 2.240µ (+7.87%, p=0.002 n=6). The struct grows by two machine words — 64 -> 80 bytes on 64-bit, 40 -> 48 on 386 and arm — but unsafe.Sizeof establishes struct growth, not heap bytes per operation, so both figures come from the corrected benchmark. Four tests assigned Jaws.Logger to a Jaws that already had a Request, one of them directly below a comment saying that is unsupported. The Jaws contract is explicit that the exported configuration fields must be set before Requests are created, so newConfiguredCoreRequest takes a configure hook that runs first. Five AddTemplateLookuper errors and one JawsRender error were blank-discarded, and a render error was matched by substring where errors.Is resolves the sentinel through html/template's ExecError. TestTemplate_EqualValuesKeepIndependentGenerations asserted only that four Elements stayed registered. An update that reclaimed the wrong wrapper's child and created a replacement leaves the same count, so it now captures each wrapper's tracked generation and asserts the replacement is scoped to it. TestTemplate_SecondClaimOnOneElementFails was cited as proving the rejected Template adds no handler, but supplied and observed none. Handlers are unexported, so TestTemplate_SecondClaimRegistersNoHandler delivers a real click instead, with a control Element rendered by a claiming Template carrying the same handler through the same params path — otherwise "not called" would pass with events broken entirely — and that control's second click bounds the drain. assertNoDOMMutation treated a 300ms timeout as success, so a stalled machine produced a false pass, and it slept 1.2s per run. It now drains to an Alert probe. Two steps are needed for different reasons: the update ran on the test goroutine, so its messages are already queued and the unbuffered InCh send forces that batch out through the request loop's sendQueue before anything else is selected; only then is the Alert queued, since getSendMsgs sorts by Jid and would otherwise place a Jid-0 Alert ahead of the element-addressed operations in one flush. The two container tests now run ten times in 1.4s. Documentation, all describing behaviour the code already had: - Template said JawsUpdate "does nothing" on an unclaimed Element; a wrapped one reports ErrElementStateUnclaimed through MustLog, which panics with no Logger configured. Register's type doc omitted the same consequence. - Comparability is necessary but not sufficient for a Dot: rendering expands it, and TagExpand rejects string, bool, the sized and unsized integer and float types, template.HTML, template.HTMLAttr, jid.Jid and key.Key. A plain string Dot is comparable and reflexive yet fails at render. The skill said "numeric", which both omits jid.Jid and key.Key and implies uintptr and the complex types are rejected when they are not. - ErrElementStateUnclaimed said "is returned"; it is reported through MustLog, reaching a caller only where html/template recovers that panic. Lookup runs first, so a missing template reports ErrMissingTemplate. - ElementState said a widget finding no state "did not render" the Element; most widgets never claim, so it says nothing about rendering. Neither function documented its concurrency guarantee, that only the claim is synchronized, or that the precondition is a Request-backed Element rather than merely a non-nil one. - The lock hierarchy called per-Element state a leaf lock and then said the widget state slot is not one. * docs: clarify Template update and state contracts
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
1 participant
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.
Closes #216.
The leak
A wrapped
ui.Templatere-registered its nested Elements on every update and never unregistered the previous ones.Template.JawsUpdateexecutes into a builder and queuesSetInner, which replaces the wrapper's entire inner DOM, so every Element rendered inside it during the previous execution is dead while staying inrq.elemsandrq.tagMapfor the Request's lifetime.The browser hid most of this:
jawsRemovingacks anInnerby reporting the ids of the descendants it removed, andRequest.handleRemoveunregisters them. That ack can only name DOM nodes carryingid="Jid.N", so the leak was exact for Elements with no DOM identity of their own — an unwrapped nested template ({{$.RequestWriter.Template "" "child" .}}), which bothui.TemplateandRequestWriter.Templatedocument as supported, being the direct case. Repeated updates grew the element registry and tag routing state without bound, and tag broadcasts increasingly walked stale Elements.The fix
RequestWritergains an unexportedelementRenderedhook, called byNewUIwith each Element it rendered, andui.Templatebecomes a pointer widget that uses it to track the Elements its execution creates. Unexported matters:RequestWriteris embedded inWith, so an exported func field would be callable from any template as{{call $.ElementRendered $.Element}}, letting a template make an Element own itself and have its own wrapper unregistered on the next update.Update semantics:
SetInnerreplacedSetInnerwas queued, so it still matches the DOMRestoring on a failed execute is what lets the next successful update reclaim that generation instead of leaking it. Initial-render failures, including a failed closing-tag write, drop the nested UI the failed execution had already created.
ContainerHelperparticipates through the same unexportedelementOwnerinterface, so template/container nesting reconciles both directions: a replaced container Element takes its children with it, and a removed container child releases the nested UI it owned. Its removals collect descendants only onceElement.Removehas actually applied —Removecan reject the operation and leave the DOM alone — and its failed-append cleanup stays ahead ofMustLog, which panics when no logger is configured.Request.DeleteElementsUnregisters a whole subtree in one pass over the registry, using the batched pattern
handleRemovealready uses. A per-descendantDeleteElementretakesrq.muand rescansrq.elemsand everytagMapslice, which is quadratic in the size of the generation being replaced.BenchmarkTemplateUpdateOwnedCleanup(1000 nested Elements, 3 extra wrapper tags, arm64,-count=6), committed as a regression guard:The second comparison is the cost of doing the cleanup at all, measured against a revision that does none. The benchmark is written so it compiles on both revisions: it returns the update as a closure, so only type inference sees the value/pointer difference.
Breaking change
NewTemplatereturns*Templateand aui.Templatevalue no longer satisfiesjaws.UI, so oneTemplatebacks one live Element like every other stateful widget in the package (*Span,*Container,*JsVar). Callers passingNewTemplate(...)straight toNewUI/NewElementare unaffected;ui.Template{...}literals used as UI need&. Containers whoseJawsContainsbuilds a fresh*Templateper call no longer reuse child Elements — identical to every pointer widget today — and it removes the hazard of aTemplatewith a non-comparableDotbeing hashed as a container pool key.lib/ui/doc.go's canonical multiplicity classification and thelib/uiREADME are updated to match.Not covered
Neither is a regression; both need core-side knowledge of ownership:
RequestWriter.RegisterandRadioGroupcreate Elements viaRequest.NewElementdirectly, bypassingNewUI, so no template tracks or unregisters them. Their cleanup falls entirely to the browser ack, which reports the ids removed from the DOM as the wrapper's new content is applied. An Element whose id never reaches the DOM has nothing to report it and stays registered until the Request ends — a discarded$.RegisterJid, or an execution that failed before delivering its markup.TestTemplate_UpdateDoesNotTrackRegisterOrRadioGrouppins the server-side behaviour.jw.Delete(tag),jw.SetInner(tag, html), a userElement.Remove— leaves its DOM-less descendants registered, since only jaws core observes those deletions.Testing
13 new tests in
lib/ui/template_owned_test.go(the issue repro through the real broadcast loop, generation identity, execute/lookup/render/closing-write/hook failure paths, recursion depth, both nesting directions, the failed-append panic window, page-template failure, many tagged descendants) plus aRequest.DeleteElementsunit test covering nil/foreign/duplicate input, the single-element fast path and tag-entry cleanup.They were checked to be load-bearing rather than merely passing: with the cleanup neutered in a throwaway worktree, all 13 fail.
Gate run locally:
gofmt,go vet(including the copylocks fallout of the new mutex),staticcheck,golangci-lint(0 issues),gosec, and the race, release-tag and debug-tag test legs.lib/uicoverage is 100.0%.