docs(engine): state and pin the concurrent-use guarantee - #332
Open
OmarAlJarrah wants to merge 1 commit into
Open
docs(engine): state and pin the concurrent-use guarantee#332OmarAlJarrah wants to merge 1 commit into
OmarAlJarrah wants to merge 1 commit into
Conversation
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.
Summary
One
engine.Enginecan serveRuncalls from several goroutines at once, but nothingsaid so and nothing pinned it. An embedder had no way to tell, so the safe move was to
over-construct — an engine per request — and a later change that parked a cache or a
scratch buffer on the
Enginewould have broken the property with a green build.Rather than take the "pure, reentrant stages" rule on trust, I checked that the guarantee
actually holds before writing it down:
varreachable fromRun(the 20 packages ingo list -deps ./engine) is a declaration-only table: keyword sets,reflect.Typevalues, sentinelerrors. None is assigned or appended to anywhere after package init.
newEngine,createOutput,renameOutput,harness.compile,reserializeJSON,encodeYAML—and those are genuinely mutable package state; a test swapping one while another
goroutine compiled would race. But all of them live in
cmd/morphic,cmd/morphic-harness,internal/harnessandir/irtest, none of which is reachablefrom
engine.Run. They do not narrow the guarantee.So the guarantee is stated as broadly as it is true, and the one place it is genuinely
narrower is called out rather than papered over:
compilers.Registry.Lookupis safeconcurrently only once registration has finished, because
Registerwrites anunsynchronized map.
engine.NewWithsatisfies that by construction — it fills a freshregistry and only then wraps it — but an embedder registering a compiler after starting
goroutines would not.
Doc comments on
engine.Engine,engine.Engine.Run,compilers.Compiler,compilers.Registryandpass.Validate, plus one new test. No behaviour change.This deliberately does not touch
.github/workflows/gate.yml; adding-raceto the gateis a separate change. The new test is race-clean today, so it is covered the moment that
step lands, and until then it still earns its keep as a determinism check. For whoever
sizes that step: the full suite under
-raceis 49s wall-clock here, and the slowestsingle package is
internal/harnessat 18.7s — well inside the coverage script's-timeout 90s.Test plan
engine/concurrency_test.godrives the whole 65-spec conformance corpus through a singleshared
*engine.Enginefrom 8 goroutines, and requires every document to bebyte-identical to a baseline compiled with a fresh
Engineper spec.Two choices in it are load-bearing. It does not build an
Engineper goroutine — thatshares nothing and proves nothing. And it does not stop at "no race reported": a data
race is only one way concurrency corrupts output, so the documents are compared, which
also re-checks the determinism invariant. The baseline uses unshared engines on purpose;
a baseline drawn from the shared engine would carry the same corruption as the concurrent
runs and cancel it out.
Clean run:
Whole repo, race detector on:
go test ./... -race -count=1exits 0 with zeroWARNING: DATA RACEoccurrences.Proof the test bites
Three deliberate defects, each planted, observed, and reverted.
Per-instance state on the
Engine(lastFormatfield, assigned inRun) — theshape a memoizing
Enginewould take. The new test fails under-race:The pre-existing engine tests all pass with this planted, because each builds its own
Engine; sharing one is what catches it. Note also that the whole suite is greenwithout
-race, which is exactly why the detector step matters.A race-free wrong answer — a fully mutex-guarded memo on the
Engine, keyed onsource format instead of on the spec. Zero
DATA RACEreports, and the test stillfails, on the comparison:
This is the case a race-only test would wave through, and the reason the baseline is
built from unshared engines.
A degenerate baseline — every baseline entry compiled from the same spec, to
confirm the anti-vacuity guard is not decorative:
Gate
gofmt,go vet,golangci-lint,go buildand./scripts/check-coverage.shall pass;coverage stays at 100%.
Closes #82