Conversation
FELIX-5832 added a canHandle filter to the CM_UPDATED path, under the rule its subject states: only handle ConfigurationEvents for config objects managed by us. Three sites read or write pidToFile, and that change covered one of them. init() adopts every configuration that records felix.fileinstall.filename, whatever the format of the file. The CM_DELETED path then deletes the file that the resulting entry names. Another ArtifactInstaller records the same property for a format of its own, so after a restart this installer owns that installer's files and deletes one when its configuration is deleted. init() applies the filter now, so a file of another format no longer enters pidToFile. The CM_DELETED path applies it too, because deleting the file is the act that loses data and a filter at each writer makes the invariant depend on every future writer repeating it. canHandle reads the file name only, so new File is enough in init(). Passing fromConfigKey would call URI.create, which throws on a value that is not a URI, and the catch around that loop would then leave pidToFile half-built. Behaviour for .cfg and .config files is unchanged. Two tests cover the pair, and the first fails when both filters are removed.
A reviewer of the same change on a fork measured what the suite holds. Removing the init() filter alone left the suite green, removing the CM_DELETED filter alone left it green, and only removing both failed a test. Either filter keeps a foreign file alive, so a test that reads the file cannot fail on one site alone. pidToFile is package-private now, and three tests read it. ConfigInstaller already exposes shouldSaveConfig and getConfiguration to this test class for the same reason, and the comment on the field states why the map is not private. testInitLeavesAConfigurationOfAnotherInstallersFormatAlone runs init() over a configuration that records a .yml file, and asserts the map stays empty. testInitAdoptsAConfigurationOfItsOwnFormat does the same with a .cfg file and asserts the map names it. A third test puts a .yml file in the map itself, which is the state the deletion site is there for, and raises CM_DELETED. Each of the three fails when its own filter goes away. The two tests that read the file stay, because they state the behaviour a user sees.
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.
Fixes FELIX-6862.
What happens
ConfigInstalleradopts every configuration that recordsfelix.fileinstall.filename, whatever the format of the file. TheCM_DELETEDpath then deletes the file that the resultingpidToFileentry names. An installer therefore deletes a configuration file that anotherArtifactInstallerhandles.Why
FELIX-5832 added a
canHandlefilter to theCM_UPDATEDpath in 2018, under the rule its subject states: only handleConfigurationEventobjects managed by us. Three sites read or writepidToFile, and that change covered one of them.init()puts every configuration that records a file name intopidToFile, with no filter.CM_DELETEDpath removes the pid frompidToFileand deletes the file, with no filter.The defect needs a restart, because
init()is the only unfiltered writer ofpidToFile. The guardedCM_UPDATEDpath rejects a foreign format, so a configuration created while the container runs never enters the map.What changed
init()applies the filter, so a file of another format no longer enterspidToFile. TheCM_DELETEDpath applies it too, because deleting the file is the act that loses data. A filter at each writer alone makes the invariant depend on every future writer repeating it.init()passesnew File(fileName)rather thanfromConfigKey(fileName).canHandlereads the file name only, andfromConfigKeycallsURI.create, which throws on a value that is not a URI. The catch around that loop would then leavepidToFilehalf-built.Behaviour for
.cfgand.configfiles is unchanged.pidToFileis package-private, so that a test can read whatinit()adopts.ConfigInstalleralready exposesshouldSaveConfigandgetConfigurationtoConfigInstallerTestfor the same reason.Tests
Two tests read the behaviour through the file.
testCmDeletedKeepsAFileOfAnotherInstallersFormatcreates a.ymlfile, runsinit(), raisesCM_DELETED, and asserts the file survives. The test fails when both filters are removed.testCmDeletedStillRemovesAFileOfItsOwnFormatdoes the same with a.cfgfile and asserts the file is deleted. The result is the same with and without the filters, so the test guards the unchanged path.Either filter keeps the
.ymlfile alive on its own, so the two tests above pin the behaviour rather than each filter. Three more tests readpidToFile, and each one fails on one site alone.testInitLeavesAConfigurationOfAnotherInstallersFormatAlonerunsinit()over a configuration that records a.ymlfile, and asserts the map stays empty.testInitAdoptsAConfigurationOfItsOwnFormatdoes the same with a.cfgfile, and asserts the map names it.testCmDeletedKeepsAFileTheMapNamesAndThisInstallerDoesNotHandleputs a.ymlfile in the map itself, which is the state the deletion site is there for, and raisesCM_DELETED.The comment at each filter states why that filter is there, and the comment on the field states why the map is not private.
mvn teston thefileinstallmodule passes on JDK 11, exceptDirectoryWatcherTest.testInvalidTempDir. That test also fails on an unmodifiedmasteron this machine, which is FELIX-6837.