Repository navigation
Add the random delay before an available update as seconds - #74
Merged
Merged
Conversation
registerDataFile schedules the first check and only then stores the future it gets back, so a check with a short delay can start in between. If the check reads the future in that gap it finds none, clears nothing, and its own re-registration then finds the stored future and schedules no next check. DataUpdateService_UpdateFromFile_FileUpdated failed this way once in a full build on macOS x64 with Java 11. The two new tests force that order on every run, one for a check that applies an update and one for a check that applies none, which is the path the retry after a 429 response takes. Registration is held after scheduling until the check has read the future or is blocked on a lock that registration holds, and the check's first read waits until registration has stored the future. Every wait is bounded, so a design that deadlocks fails rather than hangs. Each failure message lists the order in which the two threads read and wrote the future. For #70
Every read and write of a data file's future now happens while holding one lock in the data update service. registerDataFile holds it from scheduling the check to storing the future, and the check takes it before it cancels, clears or replaces the future, so a check that starts at once waits until registration has stored its future. Unregistering takes it for the same reason. The lock is the service's own rather than the data file's update lock, because the file system watcher holds the update lock while it refreshes the engine and while it waits for the new file to open. Taking that lock for the future would make unregistering wait for a refresh. Only scheduling, cancelling and storing happen while holding the new lock, so it is never held while waiting for the configuration lock or an update lock. For #70
The lock's comment now says it covers the service's own reads and writes of the future, the test data file's comment says it records the check's reads, and the second test's comment says which checks share its path. No code changes. For #70
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.
For #72
This pull request was based on #71's branch because both change DataUpdateServiceDefault.java. #71 has merged, the base is now
rc, andrcis merged into this branch (166d5b8), with the conflicts in DataUpdateServiceDefault.java resolved by keeping the renamed millisecond methods.Problem
When a data file says when its next update is available, the data update service schedules the next check for that time plus a random delay of up to
MaxRandomisationSecondsseconds (default 600).applyIntervalRandomisationadded the random seconds to the interval in whatever unit it was given.getIntervalpassed seconds and multiplied by 1000, which was right.registerDataFilepassed the milliseconds until the update is available, so the random seconds were added as milliseconds and the checks spread over under 0.6 seconds instead of 10 minutes.Change
applyIntervalRandomisationis nowapplyIntervalRandomisationMillis. It takes and returns milliseconds and converts the random seconds itself.getIntervalis nowgetIntervalMillisand returns milliseconds without a second conversion. The variable passed to the scheduler isdelayMillis, and the javadoc ofgetIntervalMillisnow describes what it returns. Its second caller, the reschedule insidecheckForUpdate, already used milliseconds and is unchanged in behaviour.Verification
New test
DataUpdateService_Register_UpdateAvailableRandomDelayInSecondsregisters 50 data files whose update is available in one hour, with randomisation of 600 seconds. It checks no delay exceeds the available time by more than the maximum and that the largest exceeds it by more than one second.rcafter Store a data file's future before its check can read it #71 merged, run 42550424855: every job succeeded, with PullRequestSha 166d5b8, this branch's head.Release note
The random delay before checking for an available update is now spread over up to MaxRandomisationSeconds seconds, as configured.
Outstanding
.NET does not have the same mismatch. Its
ApplyIntervalRandomisationtakes and returns aTimeSpan(DataUpdateService.cslines 1533 to 1543), and the update-available path passes aTimeSpan(lines 244 to 250), so the random delay is added in seconds there.