Skip to content

Let one thread at a time read a native results instance - #80

Merged
jwrosewell merged 6 commits into
rcfrom
fix/results-one-thread-at-a-time
Nov 14, 2026
Merged

jwrosewell merged 6 commits into
rcfrom
fix/results-one-thread-at-a-time

Conversation

@jwrosewell

@jwrosewell jwrosewell commented Nov 9, 2026 •

Copy link
Copy Markdown
Contributor

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 SwigData of Device Detection and IP Intelligence on rc read 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

  • No map of its own from names to positions. SwigData.get_internal in both packages reads every value by the Property's name (results.getValueAsBool(name) and the like).
  • No cache. set_cache on both on-premise engines raises CACHE_NOT_SUPPORTED, and the constructors' cache argument 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

SwigData in device-detection/.../swig_data.py and ip-intelligence/.../swig_data.py holds a re-entrant lock on the instance for each read (get) and for close, which frees the results. Reads from several threads of one instance now run one at a time, and close waits for a read in progress. Every read of the native results goes through get, which ElementData's attribute access and AspectData.get call, and no other code in either package reads swig_results.

How it was verified

New tests, test_swig_data_threads.py in 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 if close is 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' src folders on PYTHONPATH, and runs the two test files with pytest.

  • Red, rc at df0949d7 with the two test files, run 43478118693, failed on ubuntu-latest and windows-latest, all four tests, with 1 != 6 overlapping reads and closing not waiting.
  • Green, the earlier head 1383d0e, run 43478135511, passed on both, 2 passed in each package. The test code is the same at this head, which adds only the licence header to both files.
  • Full build, run 43528314652, started by tools/pr-matrix.py on tmp/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 eleven Setting 'PullRequestSha' lines read 54b1125b278246f00953fe5ecb74d1533b97470b, 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.
  • Quick checks passed on this head, being run 43528301188 (environment variable names), run 43528301606 (only rc into main) and run 43528301196 (UTM link lint).

Head 54b1125b is the earlier head 1383d0e, then the standard licence header on both new test files (1149be0), then rc at f084e85 merged in, which merged cleanly. Head 1383d0e was the first head 43f4f95 with rc at df0949d merged in, where the one conflict was in the IP Intelligence swig_data.py imports, rc having added math and decimal for the coordinate rounding and this draft threading, 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 main was #114, merged at 06:22 UTC on 13 November.

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.
# Conflicts:
#	ip-intelligence/fiftyone_ipintelligence_onpremise/src/fiftyone_ipintelligence_onpremise/swig_data.py
@jwrosewell
jwrosewell marked this pull request as ready for review November 14, 2026 04:01
@jwrosewell
jwrosewell merged commit d0c1344 into rc Nov 14, 2026
3 checks passed
@jwrosewell
jwrosewell deleted the fix/results-one-thread-at-a-time branch November 14, 2026 04:01
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.

1 participant