Repository navigation
SerializationContext.modify mutates in place, so a caller-supplied context cannot be shared across threads #344
Description
Summary
SerializationContext.modify is a context manager that mutates the context in
place and restores the previous values on exit (hcl2/utils.py:134 at
v8.1.3). Every rule that needs to change the context does so through it, on
whatever object it was handed.
That makes a caller-supplied context unsafe to share. serialize() accepts one,
threads it through the whole tree, and the rules underneath write to it, so two
concurrent parses sharing a single context observe each other's flags exactly as
two parses sharing the module-level default used to.
This is not the defect in #327. That one is about the implicit shared object
created by context=SerializationContext() as a default argument, and removing
it does nothing for a context the caller creates and shares deliberately.
Reproduction
from concurrent.futures import ThreadPoolExecutor
from hcl2.api import parses
from hcl2.utils import SerializationContext, SerializationOptions
TOGGLES = "z = f([1, 2, 3], {a = 1})\n"
PLAIN = "x = [1, 2, 3]\ny = {a = 1}\n"
EXPECTED = {"x": [1, 2, 3], "y": {"a": 1}}
shared = SerializationContext()
options = SerializationOptions()
def work(index):
tree = parses(TOGGLES if index % 2 else PLAIN)
result = tree.serialize(options, shared)
return None if index % 2 else result
with ThreadPoolExecutor(max_workers=8) as pool:
results = [r for r in pool.map(work, range(6000)) if r is not None]
print(sum(r != EXPECTED for r in results), "of", len(results), "corrupted")Three consecutive runs on the branch that fixes #327: 1635, 2228 and 2591 of
3000 parses corrupted. A tuple comes back as its inline HCL source
('[1, 2, 3]') rather than a list, with no exception raised. On released 8.1.3
the same script reports 31 of 400 at the smaller size the shared default already
made observable.
Suggested shape of the fix
Make the context immutable and have modify return a new one rather than
writing to the caller's. The class already carries exactly that method, and it
has no callers anywhere in the package:
def replace(self, **kwargs) -> "SerializationContext":
"""Return a new context with the given fields overridden."""
return replace(self, **kwargs)Every with context.modify(...) block would become a rebind that is passed to
the children it wraps, which the rules already do for the context itself. With
@dataclass(frozen=True) on top, a shared context stops being a hazard by
construction rather than by convention, and the fix for #327 becomes an
optimisation rather than a correctness fix.
It is a bigger change than #327 -- every mutation site moves -- which is why
this is filed separately rather than folded into that PR.
This issue, and the investigation behind it, were produced by an AI assistant (Claude) working on behalf of the author. Please review with that provenance in mind.
Activity
livingstaccato commentedon Sep 2, 2026
A suggestion on how this might be taken, since the shape of the fix is a judgement call rather than a correction.
The narrow version is mechanical and self-contained: make
SerializationContexta frozen dataclass, and havemodifyreturn a new context instead of writing to the caller's. The class already carriesreplace(), which does exactly that and has no callers, so the pieces are in place. Everywith context.modify(...)block becomes a rebind passed to the children it wraps — which the rules already do with the context itself, so the shape of the call sites does not change, only what they hand down.That has a property worth stating plainly: it makes a shared context safe by construction rather than by convention. #327's fix removes the shared object nobody asked for; this removes the hazard from the one a caller creates deliberately. With it in place, #327 becomes an optimisation rather than a correctness fix.
The cost is that it touches every mutation site in one change, and
modifyis public. If a consumer has written a rule that calls it, their rule keeps working — the context manager still exists — but a rule that mutated the context expecting the change to outlive the block would stop working. That is a real if unlikely break, and probably wants a minor version rather than a patch.Happy to take this if it is wanted; leaving it filed rather than guessing at the version policy.
fix: make SerializationContext immutable (amplify-education#344)
livingstaccato commentedon Sep 3, 2026
Update: this is implemented in #334, which closes it alongside #327 —
SerializationContextis a frozen dataclass,replace()returns a new context, everywith context.modify(...)site is converted to a rebind, andmodifyis removed.The version-policy note above needs correcting rather than restating:
modifyis removed outright, not preserved, so a consumer calling it gets anAttributeErrorrather than the subtler change I described there. That is a larger break than this issue states, and firmly a minor rather than a patch.The offer above is out of date — the work is written and up for review.
🤖 Drafted with Claude Code.
refactor: remove the code the deleted terminal was the only route to