fix(#341/#363): OwnAutoNamingScope — per-instance override, not a global mutex — v1.15.15#366
Merged
Merged
Conversation
…bal mutex (v1.15.15) Follow-up to #341 (v1.15.5) after upstream review on OCCT#1388. Maintainer gkv311 pointed out AutoNamingScope's recursive_mutex only serialized the three known override call sites against each other -- every other read of theAutoNaming in XCAFDoc_ShapeTool.cxx (AddShape, MakeReference, SetSHUO) stayed outside any scope, so an unrelated unscoped caller on another thread could still observe another thread's temporary override. Making the flag atomic closed the memory-safety gap, not the logical one. Root design issue: theAutoNaming was never meant to express per-document intent -- the three overriding call sites each want to suppress naming for their own document build, and XCAFDoc_ShapeTool is already one instance per document, so the override belongs there. Fix: XCAFDoc_ShapeTool::OwnAutoNamingScope replaces AutoNamingScope, saving/restoring a per-instance myOwnAutonaming field instead of the shared flag -- no locking needed, and XCAFDoc_Editor::Expand()'s self-recursion still composes correctly since each nested scope restores exactly the override state it observed on entry, not an unconditional reset. Patch 0011 updated in place (same fix, corrected design, not a new patch number). Verified: same TSan stress as the original fix, zero theAutoNaming races. New isolation scenario (Scripts/repro/363-own-autonaming/) checks the property the mutex fix couldn't guarantee: half the threads override locally while the other half do unscoped AddShape() on independent documents, concurrently -- 3000 operations, zero leaks. Full production OCCT.xcframework rebuild (macOS, iOS device, iOS simulator); OCCTBridge.xcframework also rebuilt (ABI-only -- XCAFDoc_ShapeTool grew a field, no bridge source changed) and re-verified against the new kernel; full swift test clean, both build paths. Upstream: OCCT#1388 updated to the new design and re-reviewed -- CI green across all 3 platforms, every build/GTest/regression/test job. Closes #363 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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
AutoNamingScope'srecursive_mutexonly serialized the three known override call sites against each other — every other read oftheAutoNaminginXCAFDoc_ShapeTool.cxx(AddShape,MakeReference,SetSHUO) stayed outside any scope, so an unrelated unscoped caller on another thread could still observe another thread's temporary override. Making the flag atomic closed the memory-safety gap, not the logical one.theAutoNamingwas never meant to express per-document intent — the three overriding call sites each want to suppress naming for their own document's build, andXCAFDoc_ShapeToolis already one instance per document, so the override belongs there.XCAFDoc_ShapeTool::OwnAutoNamingScopereplacesAutoNamingScope, saving/restoring a per-instancemyOwnAutonamingfield instead of the shared flag — no locking needed, andXCAFDoc_Editor::Expand()'s self-recursion still composes correctly since each nested scope restores exactly the override state it observed on entry, not an unconditional reset. Patch0011updated in place (same fix, corrected design, not a new patch number).theAutoNamingraces. New isolation scenario (Scripts/repro/363-own-autonaming/) checks the property the mutex fix couldn't guarantee: half the threads override locally while the other half do unscopedAddShape()on independent documents, concurrently — 3000 operations, zero leaks.OCCT.xcframeworkrebuild (macOS, iOS device, iOS simulator).OCCTBridge.xcframeworkalso rebuilt — ABI-only,XCAFDoc_ShapeToolgrew a field so the previously-compiled bridge static library needed relinking against the new kernel layout, even though no bridge source changed.CLAUDE.mdKnown OCCT Bugs,docs/CHANGELOG.md,docs/thread-safety.md,Scripts/patches/README.md,okf/references/carried-occt-patches.md,Scripts/repro/341-meshcaf/README.md.Test plan
swift buildclean (source bridge, rebuilt kernel)OCCTSWIFT_BRIDGE_PREBUILT=1 swift buildclean (rebuilt bridge + rebuilt kernel)Issue341MeshCafThreadSafetyTests/Issue361SharedSingletonThreadSafetyTests/Issue359pass on both build pathsswift test— 4427 tests / 1284 suites, cleanCloses #363
🤖 Generated with Claude Code