Repository navigation
FIX: Return the status of a failed reload as an error - #104
Closed
jwrosewell wants to merge 1 commit into
Closed
jwrosewell wants to merge 1 commit into
jwrosewell wants to merge 1 commit into
Conversation
ReloadFromFile and ReloadFromOriginalFile discarded the status code that the C reload returns and only checked the exception. A reload that failed with a status, such as a missing file with the in memory profile or a path that is too long, returned nil although the loaded data set was not replaced. Both now build the error from StatusGetMessage the same way InitManagerFromFile does. ReloadFromFile names the file it was given. ReloadFromOriginalFile names the file the loaded data set came from, which is the file the C reload reads. Callers that ignored reload errors will now see them. TestReloadFromFileReportsStatus and TestReloadFromOriginalFileReportsStatus check both with the Lite data file the package tests already use.
Contributor
|
This repository is being merged into https://github.com/51Degrees/51degrees-go, so this pull request is carried over there as 51Degrees/51degrees-go#6, with its commits unchanged. Please continue there. This one stays open until the move is finished. |
Contributor
|
Carried over to 51degrees-go as 51Degrees/51degrees-go#6, which was closed as superseded by 51Degrees/51degrees-go#7, now merged: it checks the status of both reloads. Closing here. |
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.
Stacked on #102. Merge that first, then retarget this to main.
Closes 51Degrees/51degrees-go#4
Problem
ReloadFromFileandReloadFromOriginalFilediscarded the status code that the C reload returns and only checked the exception. A reload that failed with a status code, such as a missing file with theInMemoryprofile or a path that is too long, returned nil although the loaded data set was not replaced. 51Degrees/51degrees-go#4 has the details and links.Change
dd/resource_hash.go: both functions now check the status code and build the error withStatusGetMessageandmessageError, the same wayInitManagerFromFiledoes.ReloadFromFilenames the file it was given.ReloadFromOriginalFilenames the file the loaded data set came from, which is the file the C reload reads.dd/reload_test.goaddsTestReloadFromFileReportsStatus, which reloads a missing file into a manager loaded with theInMemoryprofile, andTestReloadFromOriginalFileReportsStatus, which loads a copy of the data file, deletes the copy and reloads from the original file. Both expect an error naming the file. They use the Lite data file thatTestMainalready requires, so they run wherever the package tests run.Callers that ignored reload errors will now see them. A failed reload now returns, for example,
The data file '<path>' could not be found. Check the file path and that the program has sufficient read permissions.In the on-premise engine,
reloadManageralready returns these errors. A failed reload now stops before it deletes the previous temporary copy and before it records the new file as the one in use. A failure from a file watcher reload is logged. A failure from an automatic update reload, which happens only when the file watcher is off, ends the engine'sreloadFileEventloop, so the next download waits forever to hand over its reload. That loop already ends that way for other reload errors. This last point comes from readingonpremise/onpremise.goand common-go'sfile_pulling.go, not from a run.Verification
Windows 11, Go 1.25.1, MSYS2 UCRT64 gcc 16.2.0,
CGO_ENABLED=1, with51Degrees-LiteV4.1.hashat the repository root and inassets/. "Before" is 4b5857d, the head of #102, and "after" is 1c51931.go build -p 2 ./...go vet -p 2 ./dd ./onpremisego vet -p 2 ./dd ./onpremisewith go.mod temporarily set togo 1.24go test -p 2 -count=1 ./ddgo test -p 2 -count=1 ./onpremisego test -p 2 -count=1 -v -run TestReloadFrom ./ddTestReloadFromFileReportsStatusandTestReloadFromOriginalFileReportsStatusfail with "reported success"For the "before" run, the new test file was run against 4b5857d. Before #102, a direct call of
C.HashReloadManagerFromFilereturned status 19 for a 65,617 character path and status 4 for a missing file, without an exception, andReloadFromFilereturned nil for both.CI
The shared build and test workflow only picks up pull requests that are not drafts and that target main, so the build and test matrix has not run on this pull request. The UTM link lint is the only check that runs here.
What remains
reloadFileEventloop ends at the first reload error. That is not changed here.TrueforIsMobile. The committed tests check only the errors.