Skip to content

Make InternalUnitDefinition.round thread-safe#1669

Merged
jcschaff merged 1 commit intomasterfrom
fix-unit-rounding-thread-safety
May 1, 2026
Merged

Make InternalUnitDefinition.round thread-safe#1669
jcschaff merged 1 commit intomasterfrom
fix-unit-rounding-thread-safety

Conversation

@jcschaff
Copy link
Copy Markdown
Member

Summary

InternalUnitDefinition.round(double) shared a single static DecimalFormat instance across all callers. DecimalFormat is documented as not thread-safe — its internal DigitList field is mutated during both format() and parse(). Concurrent calls (background math-mapping refresh + UI work) raced through that state and produced character-level interleavings of two threads' format outputs, e.g. "11E.11-83E-83" (= "1E-83" formatted twice with digits interleaved). Double.parseDouble then threw NumberFormatException, which the catch (ParseException) did not catch, so it escaped through StructureAnalyzer.refresh and surfaced to the user as "Failed to determine metadata for data symbols" when opening the ODE data viewer.

This was the most common surface failure in a recent corpus of user-submitted support emails: 52 of ~400 traces, across 7 distinct user incidents.

Fix

Replace the format/parse round-trip with a stateless, locale-fixed implementation:

return Double.parseDouble(String.format(Locale.US, "%.12e", value));

%.12e produces 12 fraction digits in scientific notation, matching the previous DecimalFormat pattern #0.0#E0# with setMaximumFractionDigits(12). Special values (NaN, infinities) are short-circuited.

Also removes the now-unused static numberFormatForRounding field and its configuration line in the static initializer. No other code references it.

Evidence the user-visible failure messages were genuine corruption (not bad input)

From the support-email corpus, distinct NumberFormatException messages observed across 5 independent user incidents:

"For input string: \"11E.11-83E-83\""
"For input string: \"11E.11-83E\""
"For input string: \".11EE37\""
"For input string: \".11EE3737\""
"multiple points"

Each is a different interleaving of two concurrent format outputs — independent races, not one bad value.

Test plan

  • New InternalUnitDefinitionRoundTest (@Tag("Fast")) with three tests:
    • ordinary values — idempotent and within 1e-11 relative error for ten representative magnitudes
    • special valuesNaN, ±infinities, ±0.0 preserved
    • concurrent access — 16 threads × 5,000 iterations through round() with no RuntimeException thrown (regression test for the race)
  • All 6,545 existing vcell-math Fast tests still pass
  • mvn compile test-compile -pl vcell-core -am succeeds (no downstream breakage)

Note on related work

There's a second cluster in the same support-email corpus — MathSymbolMapping.put throwing NPE inside TreeMap.fixAfterInsertion (7 incidents) — that is suspected to be another concurrency issue. That's not addressed here; if confirmed it should be a separate fix.

🤖 Generated with Claude Code

The shared static DecimalFormat used by round() is not thread-safe.
Concurrent calls (background math-mapping refresh + UI work) raced
through its internal DigitList and produced character-level
interleavings of two concurrent format outputs, e.g. "11E.11-83E-83"
where two threads each formatted ~"1E-83". Double.parseDouble then
threw NumberFormatException, which the catch (ParseException) didn't
catch, so it escaped through StructureAnalyzer.refresh up to
SimulationWorkspaceModelInfo.populateDataSymbolMetadata, which the
user saw as "Failed to determine metadata for data symbols". This
was the most common surface failure in a recent support-email
corpus (52 of ~400 traces, 7 distinct user incidents).

Replace the format/parse round-trip with a stateless, locale-fixed
implementation:

    Double.parseDouble(String.format(Locale.US, "%.12e", value))

%.12e produces 12 fraction digits in scientific notation matching
the previous DecimalFormat pattern's #0.0#E0# with
setMaximumFractionDigits(12). Special values (NaN, infinities) are
short-circuited.

Removes the static numberFormatForRounding field and its
configuration line in the static initializer; no other references
exist.

Adds InternalUnitDefinitionRoundTest with three @tag("Fast") tests:
- ordinary values: idempotent and within 1e-11 relative error
- special values: NaN, infinities, signed zeros preserved
- concurrent access: 16 threads x 5000 iterations with no failures
  (regression test for the race condition)

All 6545 existing vcell-math fast tests still pass.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@danv61 danv61 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! Nice tests, especially the multi-threaded stress test.

@jcschaff jcschaff merged commit 3331960 into master May 1, 2026
12 of 13 checks passed
@jcschaff jcschaff deleted the fix-unit-rounding-thread-safety branch May 1, 2026 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants