Repository navigation
Log a data update download cut short, and every other HTTP failure, as the specification's Errors table rows - #129
Merged
Conversation
A download cut short and a failed connection are the table's first row, the access error, and each HTTP row is logged at Error as 'Error during check for data update' with an error carrying the row's message and the cause.
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 #125.
Problem
DataUpdateServiceDefault.downloadFilelogged every failure to reach the data update service at Warning, in wording of its own, onrcat 30e48867.The status each returned is the one the specification gives. The level and the message were not.
What the specification says
The Errors section of the data update specification, at 6570727d on the specifications
mainbranch, says the messages "are the messages and levels of the .NET data update service", and "A row at Error level is an error that carries the message shown. Where the service logs it, the entry is at Error, readsError during check for data update, and has the error attached." Its table gives these rows, each at Error.AUTO_UPDATE_HTTPS_ERRAUTO_UPDATE_HTTPS_ERRAUTO_UPDATE_ERR_429_TOO_MANY_ATTEMPTSAUTO_UPDATE_ERR_403_FORBIDDENAUTO_UPDATE_HTTPS_ERRA download that starts and then ends before the whole file has arrived is the first row, as #125 sets out. .NET reports it that way because
HttpClient.SendAsyncreads the whole response before it returns, inside the sametryas the connection. Which row covers it is raised in specifications#144, and Node.js made the same change in 51degrees-node#94.What changed
Every failure exit of
downloadFilenow logs its row of the table through one new private method,logCheckFailure, which logs "Error during check for data update" at Error with anIOExceptionattached that carries the row's message and, for a failed connection or a download cut short, the cause. The URL and the cause are masked as before, so a License Key never appears in full. The two helpers that built the old warnings are removed. The statuses do not change, and nothing public is added or removed.The integrity check failure is logged by
downloadOkResponse, notdownloadFile, and is left as it is (see What remains).Tests
da4c18fb changes the eleven tests in
DataUpdateServiceLoggingTeststhat cover these exits to expect the table's rows, and addsDownloadCutShort_LogsTheAccessError, whose response body gives ten bytes and then throws "Premature EOF". 3a474d00 is the fix.DataUpdateService*Testsclass onubuntu-latestwith the runner's Java and Maven. 11 of the 26 logging tests failed, being exactly the eleven changed or added, andDataUpdateServiceTests(132),DataUpdateServiceLicenseKeyTests(7) andDataUpdateServiceWatchedFileTests(9) passed.DataUpdateServiceTests, 7 inDataUpdateServiceLicenseKeyTestsand 9 inDataUpdateServiceWatchedFileTests.DataUpdateServiceTests.DataUpdateService_Register_TimerSetAfter429andDataUpdateService_UpdateFromUrl_HttpException, which expected the warning, and in the new check of a masked cause, because a cause with a License Key masked out of it is a copy whose message starts with the original's class name. 2b589ce changes the two tests to expect the one Error entry and the check to accept the masked copy. It changes no main code.assertKeyOnlyMaskedback above that method, from aboveassertCheckFailedwhere an earlier edit had left it. It changes a test comment only, andrc(6e03a25) was merged in before it at b7e9df1, with a merge commit and no conflict.Full build
Run 43580725187 built 97fdf24, this head, merged with
rc(ceab088) on nine configurations, Ubuntu x64 with Java 8, 11, 17 and 21, Ubuntu arm64 with Java 17, and Windows with Java 8, 11, 17 and 21, and every job passed. Each of its tenSetting 'PullRequestSha'lines names 97fdf24, and its Configure job printed "Left out 6 macOS entries".Windows_Java_11passed on its third attempt. In the first, GitHub reported "The hosted runner lost communication with the server" after 63 minutes, with no log. In the second, Maven stopped before building anything with "Failed to read artifact descriptor for org.sonatype.central:central-publishing-maven-plugin:jar:0.7.0" in all four trees, while the same job of another draft's build fetched that plugin at about the same time. Neither reached a test. The build of the earlier head, 2b589ce, was run 43526163363, also green.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.
Before you upgrade
A failed check for a data update, whether the connection failed, the download stopped part way, or the service answered 429, 403 or another failing status, is now logged at Error as "Error during check for data update", with the reason in the attached error, where it was a warning. An application that alerts on Error entries sees these.
What remains
AUTO_UPDATE_HTTPS_ERR, the statusrcalready returns. .NET decompresses after the download (DataUpdateService.cs line 1150 at 8b3220b3) and reports the table's "Any other error" row withAUTO_UPDATE_UNKNOWN_ERROR(lines 886 to 888), a status Java does not have yet. The same issue covers it.