Repository navigation
Let one thread at a time read a native results instance - #80
Merged
Merged
Conversation
Reading a value writes into the native results, and the first read of a component can walk its graph, so each read, and the close that frees the results, holds a lock on the instance.
…waits, and name the test times
This was referenced Nov 10, 2026
# Conflicts: # ip-intelligence/fiftyone_ipintelligence_onpremise/src/fiftyone_ipintelligence_onpremise/swig_data.py
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.
Problem
For specifications#61, "Results and threads". thread-safety.md, Element Data, lines 59 to 66, says "Some Element Data writes into something it holds when it is read, such as an on-premise Engine's Aspect Data, which reads its values from native results that each read writes into. Such Element Data MUST let only one thread at a time read from its results, for example by holding a lock for each read". A native results instance is used by one thread at a time, because reading a value writes into it and the first read of a component can walk its graph.
The on-premise
SwigDataof Device Detection and IP Intelligence onrcread the native results with no lock, so two threads reading one Flow Data's results at once, which lines 56 and 57 of the same page say MUST be safe, could both write into the same native results.What Python already did
SwigData.get_internalin both packages reads every value by the Property's name (results.getValueAsBool(name)and the like).set_cacheon both on-premise engines raisesCACHE_NOT_SUPPORTED, and the constructors'cacheargument goes through it, so the rule at lines 82 to 84 of the page, that an Engine whose Aspect Data several threads cannot share MUST NOT allow a cache, is already met.What changed
SwigDataindevice-detection/.../swig_data.pyandip-intelligence/.../swig_data.pyholds a re-entrant lock on the instance for each read (get) and forclose, which frees the results. Reads from several threads of one instance now run one at a time, andclosewaits for a read in progress. Every read of the native results goes throughget, whichElementData's attribute access andAspectData.getcall, and no other code in either package readsswig_results.How it was verified
New tests,
test_swig_data_threads.pyin both packages' test folders, use a stand-in for the native results that counts overlapping reads, and one whose read waits to be let go, so they need no native build. Each passes only if reads never overlap and ifcloseis still waiting while a read holds the results.Both runs are temporary branches holding a workflow that installs the pipeline packages from the checkout into a virtual environment of the runner's own Python (3.12.3 on Ubuntu, 3.12.10 on Windows), puts both on-premise packages'
srcfolders onPYTHONPATH, and runs the two test files with pytest.rcat df0949d7 with the two test files, run 43478118693, failed onubuntu-latestandwindows-latest, all four tests, with1 != 6overlapping reads and closing not waiting.2 passedin each package. The test code is the same at this head, which adds only the licence header to both files.tools/pr-matrix.pyontmp/pr-matrix-80h, finished with success. Configure printed "Left out 5 macOS entries", all ten Build and Test jobs (Ubuntu and Windows, Python 3.10 to 3.14) passed, and all elevenSetting 'PullRequestSha'lines read54b1125b278246f00953fe5ecb74d1533b97470b, this head. macOS was left out of this draft's full build under the 9 November decision. The rc head is built with macOS before it merges into main.Head 54b1125b is the earlier head 1383d0e, then the standard licence header on both new test files (1149be0), then
rcat f084e85 merged in, which merged cleanly. Head 1383d0e was the first head 43f4f95 withrcat df0949d merged in, where the one conflict was in the IP Intelligenceswig_data.pyimports,rchaving addedmathanddecimalfor the coordinate rounding and this draftthreading, and all three are kept.#84, the Element Data documentation, says a finished instance is safe to read from several threads, which this draft makes true for the on-premise engines, so #84 merges after this draft or with it.
Performance
A lock taken for each property read costs a little on every request. The release build's performance tests and the one-runner timing in the release body measure it on the release head.
The last release into
mainwas #114, merged at 06:22 UTC on 13 November.