Repository navigation
Release a watched engine with its service, run the watcher only with automatic updates on, and watch only the data file - #184
Merged
Conversation
…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.
This was referenced Oct 27, 2026
Merged
Merged
Merged
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.
Closes #162.
Problem
Three faults in the data update service's file system watcher. #162 has the measurements.
install_watchergave the watcher a callback holding anArcof the registration, and stored the watcher in that registration (pipeline-engines/src/data_update_service.rslines 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.registerinstalled the watcher wheneverfile_system_watcher_enabledwas 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).Change
on_file_changedneeds no service.registerinstalls 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.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_watcherdrops 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_onchanges 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_folderwrites 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 withAccess is deniedbecause 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, with51DEGREES_DD_PATHnaming the Lite Hash file,Enterprise-HashV41.hashbeside it, and51DEGREES_IPI_PATHnaming the ASN file,51Degrees-LiteV41.ipibeside it, gives 130 passed, 0 failed and 2 ignored across 24 test binaries. With the fixes merged,cargo doc --workspace --no-depswithRUSTDOCFLAGS="-D warnings"passes, andrustfmt --checkpasses on every file the seven fixes change.cargo clippy --workspace --all-targets -- -D warningson the merged fixes has not run yet. ThePull Requestworkflow, which builds and tests each pull request on every platform, is waiting for approval to run.Behaviour that changes
file_system_watcher_enabledsays, 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'srefresh, or the service'scheck_for_update, when it places a new file.What remains
Nothing in this change.