[jwrosewell]

The problem

When a data file has an update URL and no file path, the data update service applies a downloaded update in memory. On rc at b92cfcf, checkForUpdateFromUrl calls refreshData(identifier, data) on the engine with the downloaded data, or puts the data in the configuration where there is no engine yet. It then returns memoryDownloadResult, which is always AUTO_UPDATE_IN_PROGRESS at that point.

checkForUpdate reads AUTO_UPDATE_IN_PROGRESS as "an update is available but the engine has not been refreshed". It therefore calls updatedFileAvailable, which calls refreshData(identifier) on the same engine. That is a second refresh, through the method that reloads from a data file, when the update was applied in memory because there is no data file path. The interface documentation of refreshData(String) says it reloads from the file at getDataFilePath(), and that an engine built from a byte[] should be refreshed through the byte[] overload instead. If the second refresh throws, updatedFileAvailable retries it up to 10 times and the check reports AUTO_UPDATE_REFRESH_FAILED, although the first refresh had applied the update.

The existing test DataUpdateService_Register_UpdateOnStartup_InMemory checks only the call with the downloaded data, so it did not catch the second call.

What .NET does

On the same path, .NET's CheckForUpdateFromUrl sets the result to AUTO_UPDATE_SUCCESS once RefreshData(identifier, stream) succeeds (line 961), or once the stream is put in the configuration where there is no engine (line 978). CheckForUpdate calls UpdatedDataAvailable only for AUTO_UPDATE_IN_PROGRESS (line 855). On AUTO_UPDATE_SUCCESS it clears the timer and registers the data file again (lines 862 and 865). So .NET refreshes the engine once.

What changed

• The in-memory path of checkForUpdateFromUrl now returns AUTO_UPDATE_SUCCESS once the engine, or the configuration, holds the new data, as .NET does.
• checkForUpdate has a new branch for AUTO_UPDATE_SUCCESS, which clears the timer the same way updatedFileAvailable did on this path. registerDataFile then registers the next check.
• checkForUpdateFromUrl never returned AUTO_UPDATE_SUCCESS before, so the new branch is only reached from the in-memory path.

Polling after a manual update

The data updates page on rc says the following in three places.

• Under Automatic update master switch:

  This master switch only governs automatic activity - it does not disable Programmatic update, which remains available for triggering an update manually

• Under Programmatic update, a newer file is processed "as per newFile above".
• Under newFileAvailable, the steps include "pause update polling, if any", "load file" and "resume polling, if any".

Before, checkForUpdate registered the data file again only after a check the timer started. A manual update cleared the timer, through updatedFileAvailable on the file path or the in-memory success branch, and set no new one, so polling stopped after it.

• checkForUpdate now also registers the data file again after a manual check that applied an update, where the data file is registered with the service. So registerDataFile sets the next timer when automatic updates are on, as it does after an automatic check. This covers both the file path and the in-memory path.
• A manual update never leaves two timers. The pending timer is cancelled and cleared under futureLock, by updatedFileAvailable (line 294) or by the in-memory success branch (line 232), before registerDataFile runs. registerDataFile schedules a timer only while the future is null, under the same lock (lines 448 to 450).
• A manual check that finds no update leaves the existing timer alone, as before.
• A data file that was never registered is not registered by a manual check. That keeps DataUpdateService_CheckForUpdate_FileUpdated, which makes a manual check on an unregistered data file and expects no timer, as it was.
• Java never turns automatic updates off on a manual check. .NET's manual CheckForUpdate does (DataUpdateService.cs line 356 at 5b4d06e), and that is being changed to match the master switch rule above.

Overlap with other drafts

Two open drafts also edit DataUpdateServiceDefault.java. This change stays off their lines, and git merge-tree shows it merges cleanly with each of their heads.

• Make the data update follow the specification #79 (head 98f3a9a) changes the checkForUpdateFromUrl call in checkForUpdate (old line 218), the downloadFile call and the log lines in the in-memory path (old lines 792, 797 to 799 and 810 to 812), and the retry loop in updatedFileAvailable. This change edits old line 808, adds a branch after old line 228, and changes the re-registration condition at old line 252, which Make the data update follow the specification #79 does not edit. Between them sit unchanged lines (old line 809, and old lines 219 to 228). Make the data update follow the specification #79 also edits DataUpdateServiceTests.java at old lines 371, 884 and 1391, and the new test sits after old line 1222.
• Show a license key only by its last 4 characters in logs and error messages #85 (head cb0b80e) edits downloadFile (old lines 615 to 671) only.

For the release notes

• When a data file has no file path, an update downloaded from the update URL now refreshes the engine once, with the downloaded data. Before, the engine was refreshed a second time through refreshData(String), and a failure of that second refresh was reported as a failed update.
• A manual update of a data file registered for automatic updates now sets the next update timer, so polling resumes after it. Before, polling stopped after a manual update. A manual check that finds no update leaves the timer as it was.

How to verify

mvn -pl pipeline/pipeline.engines -am test -Dtest='DataUpdateServiceTests' -Dsurefire.failIfNoSpecifiedTests=false

Class	Test methods at rc (b92cfcf)	This branch (4215118)
DataUpdateServiceTests	26	30, each run for the class's 4 parameter sets

• 0b500e7 holds the test alone, on rc's code. DataUpdateService_CheckForUpdate_InMemory_RefreshesOnce makes a manual check for a data file with an update URL and no file path, and counts every refreshData call on the mocked engine. It is written to fail there in all 4 parameter sets, finding two calls, refreshData(String, byte[]) and then refreshData(String).
• 66a5704 holds the fix. DataUpdateService_Register_UpdateOnStartup_InMemory should still pass, as it still sees one call with the downloaded data and one timer scheduled.
• d99c9b7 makes the watcher test wait for the watcher's update rather than the polling timer's check.
• 6ff3192 holds three manual check tests alone, on d99c9b7's code. Each runs for both settings of automatic updates on a data file registered with the service.
  • DataUpdateService_CheckForUpdate_FileUpdated_TimerSetAgain applies an update from the data file.
  • DataUpdateService_CheckForUpdate_InMemory_TimerSetAgain applies one downloaded in memory.
  • Both expect one more timer and a future on the data file where automatic updates are on. Both are written to fail there on d99c9b7's code, where the manual update leaves no timer.
  • DataUpdateService_CheckForUpdate_NoUpdate_TimerKept checks that a manual check finding no update keeps the same timer and schedules none. It passes before and after the change.
• 399afbb holds the fix. The commit body quotes the specification lines above.
• 4215118, this branch's head, adds to both TimerSetAgain tests a check that the timer pending before the manual update was cancelled and replaced. With the count of scheduled timers, that shows one live timer is left.

None of these commits has been built locally. The results of the CI runs are to be added here.