Fall back to a rope's published text when a concurrent flatten has released its operands

[lahma]

JsString.RopeString (the deferred a + b from #3386) memoizes its text on first read and then releases both operands, so a flattened value stops retaining the tree it was built from. The release was two plain writes after the memo, and the walk that builds the text tested the memo and then dereferenced the operands. A second thread finishing its own flatten of the same node between those two reads left the walk holding a null operand, and the next node.ToString() threw NullReferenceException (JsString.cs:1024 on main @ 431590d25).

It needs no host sharing anything

The issue frames this as a host handing one result to several threads. There is a more direct route, and it is a documented one:

• Engine.PrepareScript folds a literal-plus-literal into a JintConstantExpression and publishes it on the AST's UserData (Engine.Ast.cs, FoldConstants, on by default).
• Prepared<Script> is documented as reusable, thread-safe and runnable concurrently on separate engines (Engine.PrepareScript's XML doc, docs/guide/thread-safety.md).
• When the folded result is at least MinDeferredConcatenationLength (512) characters, that constant is a RopeString: one instance, read by every engine running the preparation. The new test pins this with BeSameAs.

So two engines running one preparation at the same time race the first read of that node, which is the crash above. Before #3386 the same constant (and every a + b result) was a flat JsString. Its text is assigned in the constructor and never written again, so concurrent reads were safe.

On the host-sharing question itself: docs/guide/thread-safety.md rules out passing an object-valued JsValue to another engine and says nothing about primitives. The root AGENTS.md rule against sharing a JsValue across engines exists because an ObjectInstance holds its engine and realm, and a JsString holds neither. The closest thing to a stated contract is LazyJsString's: "A host that reads the same instance from several of its own threads may see Materialize run more than once … nothing tears." In other words, a concurrent first read may waste work but must not fail. The rope did not meet that standard.

The change

The issue's remedy B, confined to the tail of Flatten and the walk in CopyInto:

// Flatten
_value = value;
Volatile.Write(ref _left, null);
Volatile.Write(ref _right, null);

// CopyInto, on a rope whose memo tested null
var left = Volatile.Read(ref rope._left);
var right = Volatile.Read(ref rope._right);
if (left is not null && right is not null) { /* descend as before */ }
else text = Volatile.Read(ref rope._value);   // released ⇒ memo visible

Why both sides need to be volatile. Under the .NET memory model, "the effects of ordinary reads and writes can be reordered as long as that preserves single-thread consistency". Object assignment is a release only "with respect to accesses to the instance's fields". So a plain _left = null may become visible on a weak-memory target (ARM64) before the _value store it follows, and a reader that falls back to the memo on a released operand would find neither. The volatile writes release the memo ahead of each operand's release, and the volatile reads acquire it before the fallback reads it. Two flattens racing each other stay harmless: each builds the same text, and the last store wins.

Why not remedy A (never release). The release is what keeps a flattened accumulator from retaining one node per iteration plus every operand string. For the 4,096-iteration s = s + chunk16 shape, that is 4,065 nodes of 56 bytes each (about 228 KB) kept alongside the flat text. When the operands are distinct strings, the text is also kept a second time, in pieces. Keeping the release keeps #3386's memory profile. Memory accounting is untouched (see #4162, which works in the same file).

Hot path. ToString() => _value ?? Flatten() is unchanged: it is still a plain load, and it needs nothing more because storing the memo reference already publishes the string's contents. Checked in the net10.0 x64 FullOpts disassembly: Flatten's two releases are the same plain mov [rbx+0x18], 0 / mov [rbx+0x20], 0 as before, and CopyInto reads the operands with plain movs and adds two test/jcc per visited rope node, with no fence. On ARM64 the cost is two stlr per flatten and two ldar per rope node visited during a flatten. It is paid only while a node is being flattened, never on a read of an already-flat value.

Neighbours checked

• CopyInto's traversal: the only reader of _left/_right, and all of its reads changed. An outer flatten never releases an inner node's operands (only the node being flattened releases its own), so the race is always "this node is being flattened elsewhere". That covers the root and any shared inner node.
• Other _value readers: the base Equals/GetHashCode read _value and fall back to ToString() on null. A non-null memo on a rope is always the complete text, so they are correct as they stand.
• SlicedString: _value ??= _source.Substring(...) is benign. It is idempotent, its source fields are readonly, and it releases nothing.
• ConcatenatedString: the issue calls its memo race benign. On x86/x64 it is, because both threads build the same text and nothing is released. On ARM64 it is not strictly so: _value = …; _dirty = false; are ordinary writes, so a reader can see _dirty == false before the new _value and return the pre-append text (a wrong value, not an exception). It is reachable only if a host shares a dirty += result across threads. A preparation never produces one, because Concat snapshots such operands through Immutable. It is not changed here because it sits on the += hot path; I am noting it as a follow-up.

Evidence

New fixture Jint.Tests/Runtime/RopeStringConcurrencyTests.cs:

• TwoThreadsReadingANodeForTheFirstTimeBothGetItsText: 20,000 fresh ropes and two dedicated threads, released together per rope by a spin gate. One thread is held back by a swept SpinWait so its start crosses the other's flatten. The held-back thread alternates between attempts.
• EnginesRunningOnePreparedScriptAtOnceAllReadItsFoldedConcatenations: four engines each run 150 fresh preparations of 64 folded 600-character constants. It first asserts that the folded constant is a RopeString shared by two engines, so the test cannot silently stop covering anything.

Before the fix (431590d25), every run failed with NullReferenceException at RopeString.CopyInto:

run	TFM	two threads, one node	engines sharing a preparation
1	net10.0	1,533 of 40,000 reads	205 of 600 evaluations
2	net10.0	383 of 40,000	417 of 600
3	net10.0	447 of 40,000	387 of 600
1	net472	1,203 of 40,000	235 of 600

After the fix: 0 failures in 5 further net10.0 runs of the fixture and in the full suites below. A local, uncommitted counter on the new fallback branch showed it taken 12–25 times per run in the two-thread test and 1–23 times in the prepared-script test on net10.0 (24 and 1 on net472). The race is still being hit, and it now resolves to the memo.

Full suites at 299dfb078 on base 431590d25, with no --timeout on the whole-project runs:

project	TFM	total	passed	skipped	failed
Jint.Tests	net10.0	12,660	12,655	5	0
Jint.Tests	net472	8,708	8,702	6	0
Jint.Tests.PublicInterface	net10.0	3,759	3,738	21	0
Jint.Tests.PublicInterface	net472	2,966	2,945	21	0

test262 was not run: the change alters no spec-visible behaviour, only whether a concurrent first read throws.

Benchmarks: not run

Measurement is a separate serial phase on an idle machine. These are the rows that execute the changed code, with per-operation counts from a local instrumented census (a counter, not a timing):

row	flattens / op	rope nodes walked / op
StringConcatLargeBenchmark.AssignAppendThenScan	1	4,065
DromaeoBenchmark.ObjectRegExp (classic / modern)	100	504 / 476
DromaeoBenchmark.ObjectString	2	4
SunSpiderBenchmark 3d-raytrace, crypto-aes, regexp-dna, string-tagcloud	1	1–2

These rows build ropes but never flatten them, so they are controls that run none of the changed code: AssignAppendSmallChunks, AssignPrependSmallChunks, AssignChainThree, ConcatLargePair, ChainLargeThree.

Closes #4161

🤖 Generated with Claude Code