Repository navigation
Conversation
…terfaceModel registries XSControl_Controller keeps every recorded controller in the file-scope map listad, and Interface_InterfaceModel keeps its template models in the file-scope map atemp. Both are process-wide by design and both are read and modified with no synchronisation: Record does IsBound, ChangeFind and Bind, Template does HasTemplate and then ChangeFind, so two threads rehash or read the map under each other. Guard each with a recursive mutex. Recursive is needed for atemp, since Template calls HasTemplate while holding the lock. Add GTests that register and read many entries from several threads and check that none is lost.
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.
Pre-Submission Checks
.github/CONTRIBUTING.md.Group - Summaryformat.Problem / Motivation
Two file-scope maps in
TKXSBaseare process-wide registries that any thread can read and modify, and neither has any synchronisation:listadinXSControl_Controller.cxx, the controllers recorded by name.RecorddoesIsBound, thenChangeFind, thenBind;RecordeddoesFind. EverySTEPControl_Controller::Init(),IGESControl_Controller::Init()and everyAutoRecord()goes through it.atempinInterface_InterfaceModel.cxx, the template models by name.TemplatecallsHasTemplateand thenChangeFind, a check-then-act over two lookups;SetTemplatebinds andListTemplatesiterates.Two threads recording into the same
NCollection_DataMaprehash it under each other, or one reads a bucket chain while the other relinks it. The result is a lost entry, a hang, or a crash inside the map.Proposed Solution
Guard each map with a
std::recursive_mutex, taken in every function that touches the map. A lock fits these two because one registry per process is the design; the state is shared on purpose, so there is nothing to relocate into an instance. The mutex is recursive becauseInterface_InterfaceModel::Templatetakes it and then callsHasTemplate, which takes it again.Interface_Static's parameter table (also a process-wide registry) is not touched here; it is the subject of #1553, and a second lock on the same data through another path would only invite a lock-order inversion.Two tests in
TKXSBase/GTests, which had an empty list waiting for a first entry:XSControl_ControllerTest.ConcurrentRecordKeepsEveryController: eight threads record 250 controllers each under distinct names while reading them back, then every controller must be found.Interface_InterfaceModelTest.ConcurrentTemplateRegistrationKeepsEveryTemplate: eight threads register and fetch 250 templates each, then every name must be present andListTemplatesmust list all of them.The controllers and models are built on the main thread first, so the tests exercise the registries and not whatever else a constructor initialises.
Validation
Local build of current
IR(Release,BUILD_RELEASE_DISABLE_EXCEPTIONSon as CI builds it, macOS arm64, clang),OpenCascadeGTest. These are concurrency tests, so the result is a rate:IRwithout the source changeXSControl_ControllerTest.ConcurrentRecordKeepsEveryControllerInterface_InterfaceModelTest.ConcurrentTemplateRegistrationKeepsEveryTemplateAcross the 30 unpatched runs the failures were 17
SIGTRAP, 4SIGABRT, 2SIGSEGV, 4 hangs killed by a 60 s timeout, and 2 clean assertion failures (entries lost); the one passing run is the race not firing.The local tree has
TKXSBaseandTKDEfromDataExchangeplus the modeling modules, so the otherDataExchangesuites (STEP, IGES, XCAF) were not built here; the neighbouring*Interface*,*XSControl*,*IFSelect*,*Transfer*tests that exist in that tree pass (16 of 16).Checks performed:
clang-format 18.1.8 with the repository
.clang-format, the license check and the include cleanup of the formatting job report no change on the touched files, and none contains a non-ASCII character.Review Notes
The two source files are independent of each other; each change can be reviewed on its own.