Skip to content

Release a watched engine with its service, run the watcher only with automatic updates on, and watch only the data file - #184

Merged
jwrosewell merged 3 commits into
release/4.5.12from
fix/data-update-file-watcher
Oct 27, 2026
Merged

jwrosewell merged 3 commits into
release/4.5.12from
fix/data-update-file-watcher

Conversation

@jwrosewell

@jwrosewell jwrosewell commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

Closes #162.

Problem

Three faults in the data update service's file system watcher. #162 has the measurements.

  1. install_watcher gave the watcher a callback holding an Arc of the registration, and stored the watcher in that registration (pipeline-engines/src/data_update_service.rs lines 334 to 377). The two kept each other alive, with the engine, its data set, the open data file and the watcher's thread, after the service and every caller had dropped them, and a later change to the file still refreshed the engine. The facade pipeline builders turn the watcher on by default, so dropping an on-premise pipeline freed none of these.
  2. register installed the watcher whenever file_system_watcher_enabled was on (line 175). The specification makes automatic updates the master switch for polling and the watcher alike (data-updates.md lines 89 to 103), and .NET creates the watcher only with automatic updates on (DataUpdateService.cs lines 261 to 266).
  3. The watch is on the data file's folder, and the callback refreshed the engine for any modify or create event in it (line 355), starting from no recorded modified time, so another file written in the folder reloaded the whole data set.

Change

  1. The callback holds the registration weakly, so nothing it holds outlives the service, and on_file_changed needs no service.
  2. register installs the watcher only when automatic updates are on as well. The documentation of the switches says so, in the data file configuration, both engine builders, both facades, the README and the update example.
  3. The callback acts only on events that name the data file, and the file's modified time is recorded when the watcher is installed, so a refresh needs a newer file.

How it was verified

New tests in pipeline-engines/tests/file_watcher.rs, each against a counting engine:

  • dropping_the_service_releases_the_engine_and_stops_the_watcher drops the service and the engine, checks a weak reference to the engine no longer upgrades, then changes the file and checks nothing is refreshed.
  • the_watcher_runs_only_with_automatic_updates_on changes the file of an engine with automatic updates off and checks it is not refreshed, then does the same with them on and checks it is.
  • the_watcher_ignores_other_files_in_the_folder writes another file in the folder and checks nothing is refreshed, then changes the data file and checks the engine is.

Before the change, on a branch holding only the new tests, all three fail:

  • dropping_the_service_releases_the_engine_and_stops_the_watcher: the engine is still alive after the service and its caller dropped it.
  • the_watcher_runs_only_with_automatic_updates_on: the watcher refreshed an engine with automatic updates off, one refresh where none was expected.
  • the_watcher_ignores_other_files_in_the_folder: another file in the folder refreshed the engine.

After it, all three pass.

Measured separately with the native library's tracking allocator, on a Device Detection engine over a copy of the Lite Hash file under LowMemory: with the watcher on, 6,662 native bytes stayed allocated after the engine and the service were dropped, against none with it off, and replacing the data file afterwards failed with Access is denied because the engine still had it open.

Locally on Windows, with all seven fixes of this sweep merged together (#175, #178, #182, #184, #194, #195 and #196), cargo test --no-fail-fast -p fiftyone-pipeline-engines -p fiftyone-device-detection-onpremise -p fiftyone-ip-intelligence-onpremise -p fiftyone-device-detection -p fiftyone-ip-intelligence -- --nocapture, with 51DEGREES_DD_PATH naming the Lite Hash file, Enterprise-HashV41.hash beside it, and 51DEGREES_IPI_PATH naming the ASN file, 51Degrees-LiteV41.ipi beside it, gives 130 passed, 0 failed and 2 ignored across 24 test binaries. With the fixes merged, cargo doc --workspace --no-deps with RUSTDOCFLAGS="-D warnings" passes, and rustfmt --check passes on every file the seven fixes change. cargo clippy --workspace --all-targets -- -D warnings on the merged fixes has not run yet. The Pull Request workflow, which builds and tests each pull request on every platform, is waiting for approval to run.

Behaviour that changes

  • Dropping a data update service, or the pipeline that owns one, now frees the engines registered with it, closes their data files and stops the watchers' threads.
  • With automatic updates off there is no file system watcher, whatever file_system_watcher_enabled says, as the specification says. A deployment built with .auto_update(false) that relied on the watcher no longer reloads when the file changes on disk. It can call the engine's refresh, or the service's check_for_update, when it places a new file.
  • Only a change to the data file itself refreshes the engine.

What remains

Nothing in this change.

…automatic updates on, and watch only the data file

The watcher's callback held the registration that owned the watcher, so
the registration, its engine and the data set the engine held, the open
data file and the watcher's thread outlived the service. The callback now
holds the registration weakly. Automatic updates are the specification's
master switch for the watcher as well as for polling, and the watcher now
refreshes the engine only for events that name the data file, from the
modified time the file had when it was registered.
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