Skip to content

Data Exchange, STEP - Make the non-manifold detection flag per-actor - #1552

Open
gsdali wants to merge 1 commit into
Open-Cascade-SAS:masterfrom
SecondMouseAU:fix/stepcontrol-actorread-nm-detected-per-instance
Open

gsdali wants to merge 1 commit into
Open-Cascade-SAS:masterfrom
SecondMouseAU:fix/stepcontrol-actorread-nm-detected-per-instance

Conversation

@gsdali

@gsdali gsdali commented Sep 21, 2026

Copy link
Copy Markdown
Contributor

STEPControl_ActorRead.cxx carries NM_DETECTED as an anonymous-namespace global. It is reset at the start of each shape-representation transfer, set when a non-manifold item is recognised, and read by the product-definition overload to decide whether an assembly component's COMPOUND is flattened into its parent or kept nested.

Two concurrent STEP reads therefore share a flag that gates shape construction.

The declaration's own comment already records the intended direction:

Set global var to inform outer methods that current representation item is non-manifold.
The better way is to pass this information via binder or via TopoDS_Shape itself, however,
this is very specific info to do so...

The fix

A private member with a default member initialiser. Every read and write is already inside a STEPControl_ActorRead member function, so no signature change is needed.

This is sufficient because STEPControl_Controller::ActorRead() never assigns myAdaptorRead, so it constructs a fresh actor per call which XSControl_TransferReader::Actor() caches per session. Each concurrent read owns its flag.

Measured

ThreadSanitizer, six threads reading a non-manifold and a manifold assembly concurrently, ten iterations each:

NM_DETECTED race reported Total reports
Before 5 of 5 runs 40 across 5 runs
After 0 of 5 runs 10 across 5 runs

What is NOT claimed

A wrong shape was not reproduced. The flag demonstrably leaks between threads (25 to 211 leaked reads depending on configuration), but across roughly ten thousand reads in three configurations zero shapes came back altered, because within a thread the reset reliably wins the race to that same thread's read.

So this is a confirmed data race on a flag that gates shape construction, not a demonstrated wrong answer. The GTest reflects that honestly: it pins the ownership invariant the fix relies on, that ActorRead hands out a distinct actor per call, rather than asserting an outcome that cannot be reproduced deterministically. If you would prefer a concurrency test that asserts the shape instead, I can add one, but it would be timing-dependent and I would not want to put a flaky test in your CI.

STEPControl_ActorRead.cxx carries NM_DETECTED as an anonymous-namespace global. It is
reset at the start of each shape-representation transfer, set when a non-manifold item
is recognised, and read by the product-definition overload to decide whether an
assembly component's COMPOUND is flattened into its parent or kept nested.

Two concurrent STEP reads therefore share a flag that gates shape construction.

The declaration's own comment already records the intended direction:

    The better way is to pass this information via binder or via TopoDS_Shape itself

Every read and write is inside a STEPControl_ActorRead member function, so moving the
flag to a private member needs no signature change. STEPControl_Controller::ActorRead
never assigns myAdaptorRead, so it returns a fresh actor per call which
XSControl_TransferReader caches per session; each concurrent read owns its flag.

Measured with ThreadSanitizer, six threads reading a non-manifold and a manifold
assembly concurrently, ten iterations each: the race on NM_DETECTED is reported in
5 of 5 runs before this change and 0 of 5 after, with total reports across five runs
falling from 40 to 10.

What is NOT claimed: a wrong shape was not reproduced. The flag demonstrably leaks
between threads, 25 to 211 leaked reads depending on configuration, but across roughly
ten thousand reads in three configurations zero shapes came back altered, because
within a thread the reset reliably wins the race to that same thread's read. This is a
confirmed data race on a flag that gates shape construction, not a demonstrated wrong
answer, and the accompanying GTest reflects that: it pins the ownership invariant the
fix relies on, that ActorRead hands out a distinct actor per call, rather than
asserting an outcome that cannot be reproduced deterministically.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Todo

Development

Successfully merging this pull request may close these issues.

1 participant