Skip to content

Process-global side tables are cleared by test guards that readers are not required to take — three flakes in two days #7672

Description

@proggeramlug

Three flaky tests in two days, all the same architecture: a process-global side table, plus per-suite guards that clear it, plus tests that read it without taking the clearing lock.

fixed in table how it presented
#7665 opt_report's Mutex<Vec<Entry>> rows.len() == 2 failed at 3 — a neighbour's entry in the snapshot
#7665 ext_registry's USED_PROVIDERS "empty after an unlisted symbol" failed with ioredis still present
#7671 closure/dynamic_props.rs's CLOSURE_PROPS a static method value read back as TAG_UNDEFINED

In every case the lock existed and was correct for the tests that took it. The failure is that nothing requires a reader to take it, so the defence is opt-in and the opt-in is invisible at the read site.

Each was exposed by an unrelated PR adding tests and changing the parallel schedule — never introduced by it. That is the expensive part: the author of the exposing PR spends the diagnosis, and the natural conclusion ("my change broke something") is wrong.

The remaining exposure, surveyed

Files whose tests read closure dynamic props, and whether they take crate::gc::global_side_table_test_lock():

guarded file (test count)
3 of 4 closure/dynamic_props.rs
2 of 9 object/global_this_webassembly.rs
2 of 2 object/native_module_stream.rs (after #7671)
0 array/tests.rs (65), node_stream_tests.rs (42), node_submodules/tests.rs (33), object/instanceof.rs (5), object/native_module/constants.rs (4), value/to_string.rs (4), node_stream_state_tests.rs (4), tls.rs (3), object/native_module/callable_export_arity_table.rs (3), perf_hooks.rs (2), object/prototype_chain.rs (2), object/native_call_method.rs (2), value/dynamic_object.rs (2), and 6 more with 1 each

Roughly 180 unguarded tests. Most will never bite — the hazard needs a test that reads a persisted prop across a window a guard can land in — but "most" is doing real work in that sentence, and the three we found were each a surprise.

Options, in rough order of preference

  1. Make the clear observable. Have test_clear_closure_side_tables() record that it ran, and give readers a cheap assertion that no clear happened during their read. Turns a silent wrong value into a named failure at the point of damage, without serialising anything.
  2. Invert the default. Make the reader-side accessor take the lock, with an explicit opt-out for the guards that must clear. Costs some parallelism; removes the class.
  3. Scope the tables per-thread in tests. Largest change, and CLOSURE_PROPS is keyed by heap address with a GC scanner attached, so this needs care.
  4. Blanket-lock the ~180 tests. Rejected as the first move: it serialises a large part of the suite for a hazard whose incidence we cannot yet bound.

Option 1 is the one that pays for itself even if the class never bites again, because it converts "intermittent, diagnosed by luck" into "fails with a message naming the clearer".

Context: #7665, #7671, and gc/tests/support.rs's own comment, which documents the hazard but does not enforce anything.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions