From 1e55914e55ddcadcaf06f5818fcbf7e71ecf3d47 Mon Sep 17 00:00:00 2001 From: jimtng <2554958+jimtng@users.noreply.github.com> Date: Sat, 18 Mar 2023 18:15:20 +1000 Subject: [PATCH] Improve AbstractScriptFileWatcher initialization order and file handling (#3451) * Remove initialImport from the constructor of AbstractScriptFileWatcher - Calling initialImport inside the constructor may cause an NPE in child class. * Refactor processWatchEvent in AbstractScriptFileWatcher - Remove directory deletion handling and adapt test to check for file removals instead. - Include hidden files in deletions in case they were previously loaded while not hidden, set to hidden, and then deleted. Signed-off-by: Jimmy Tanagra --- .../loader/AbstractScriptFileWatcher.java | 89 ++++++++----------- .../loader/AbstractScriptFileWatcherTest.java | 11 ++- 2 files changed, 43 insertions(+), 57 deletions(-) diff --git a/bundles/org.openhab.core.automation.module.script.rulesupport/src/main/java/org/openhab/core/automation/module/script/rulesupport/loader/AbstractScriptFileWatcher.java b/bundles/org.openhab.core.automation.module.script.rulesupport/src/main/java/org/openhab/core/automation/module/script/rulesupport/loader/AbstractScriptFileWatcher.java index f65907b20..5c4faa40c 100644 --- a/bundles/org.openhab.core.automation.module.script.rulesupport/src/main/java/org/openhab/core/automation/module/script/rulesupport/loader/AbstractScriptFileWatcher.java +++ b/bundles/org.openhab.core.automation.module.script.rulesupport/src/main/java/org/openhab/core/automation/module/script/rulesupport/loader/AbstractScriptFileWatcher.java @@ -104,16 +104,9 @@ public abstract class AbstractScriptFileWatcher implements WatchService.WatchEve this.readyService = readyService; this.watchSubDirectories = watchSubDirectories; this.watchPath = watchService.getWatchPath().resolve(fileDirectory); - - manager.addFactoryChangeListener(this); - readyService.registerTracker(this, new ReadyMarkerFilter().withType(StartLevelService.STARTLEVEL_MARKER_TYPE)); - this.scheduler = getScheduler(); - - currentStartLevel = startLevelService.getStartLevel(); - if (currentStartLevel > StartLevelService.STARTLEVEL_MODEL) { - initialImport(); - } + // Start with the lowest level to ensure the code in onReadyMarkerAdded runs + this.currentStartLevel = StartLevelService.STARTLEVEL_OSGI; } /** @@ -144,6 +137,9 @@ public abstract class AbstractScriptFileWatcher implements WatchService.WatchEve } else if (!Files.isDirectory(watchPath)) { logger.warn("Trying to watch directory {}, however it is a file", watchPath); } + + manager.addFactoryChangeListener(this); + readyService.registerTracker(this, new ReadyMarkerFilter().withType(StartLevelService.STARTLEVEL_MARKER_TYPE)); watchService.registerListener(this, watchPath, watchSubDirectories); } @@ -223,27 +219,22 @@ public abstract class AbstractScriptFileWatcher implements WatchService.WatchEve @Override public void processWatchEvent(WatchService.Kind kind, Path path) { + if (!initialized.isDone()) { + // discard events if the initial import has not finished + return; + } + Path fullPath = watchPath.resolve(path); File file = fullPath.toFile(); - if (!file.isHidden()) { - if (kind == DELETE) { - if (file.isDirectory()) { - if (watchSubDirectories) { - synchronized (this) { - String prefix = fullPath.getParent().toString(); - Set toRemove = scriptMap.keySet().stream().filter(ref -> ref.startsWith(prefix)) - .collect(Collectors.toSet()); - toRemove.forEach(this::removeFile); - } - } - } else { - removeFile(ScriptFileReference.getScriptIdentifier(file.toPath())); - } - } - if (file.canRead() && (kind == CREATE || kind == MODIFY)) { - addFiles(listFiles(file.toPath(), watchSubDirectories)); + // Subdirectory events are filtered out by WatchService, so we only need to deal with files + if (kind == DELETE) { + String scriptIdentifier = ScriptFileReference.getScriptIdentifier(fullPath); + if (scriptMap.containsKey(scriptIdentifier)) { + removeFile(scriptIdentifier); } + } else if (!file.isHidden() && file.canRead() && (kind == CREATE || kind == MODIFY)) { + addFiles(listFiles(fullPath, watchSubDirectories)); } } @@ -269,20 +260,21 @@ public abstract class AbstractScriptFileWatcher implements WatchService.WatchEve return CompletableFuture.runAsync(() -> { Lock lock = getLockForScript(scriptIdentifier); try { - ScriptFileReference ref = scriptMap.remove(scriptIdentifier); + scriptMap.computeIfPresent(scriptIdentifier, (id, ref) -> { + if (ref.getLoadedStatus().get()) { + manager.removeEngine(scriptIdentifier); + logger.debug("Unloaded script '{}'", scriptIdentifier); + } else { + logger.debug("Dequeued script '{}'", scriptIdentifier); + } - if (ref == null) { - logger.warn("Failed to unload script '{}': script reference not found.", scriptIdentifier); - return; - } - - if (ref.getLoadedStatus().get()) { - manager.removeEngine(scriptIdentifier); - logger.debug("Unloaded script '{}'", scriptIdentifier); - } else { - logger.debug("Dequeued script '{}'", scriptIdentifier); - } + return null; + }); } finally { + if (scriptMap.containsKey(scriptIdentifier)) { + logger.warn("Failed to unload script '{}'", scriptIdentifier); + } + scriptLockMap.remove(scriptIdentifier); lock.unlock(); } @@ -353,20 +345,6 @@ public abstract class AbstractScriptFileWatcher implements WatchService.WatchEve return false; } - private void initialImport() { - File directory = watchPath.toFile(); - - if (!directory.exists()) { - if (!directory.mkdirs()) { - logger.warn("Failed to create watched directory: {}", watchPath); - } - } else if (directory.isFile()) { - logger.warn("Trying to watch directory {}, however it is a file", watchPath); - } - - addFiles(listFiles(directory.toPath(), watchSubDirectories)).thenRun(() -> initialized.complete(null)); - } - @Override public void onDependencyChange(String scriptIdentifier) { logger.debug("Reimporting {}...", scriptIdentifier); @@ -381,13 +359,16 @@ public abstract class AbstractScriptFileWatcher implements WatchService.WatchEve int previousLevel = currentStartLevel; currentStartLevel = Integer.parseInt(readyMarker.getIdentifier()); + logger.trace("Added ready marker {}: start level changed from {} to {}. watchPath: {}", readyMarker, + previousLevel, currentStartLevel, watchPath); + if (currentStartLevel < StartLevelService.STARTLEVEL_STATES) { // ignore start level less than 30 return; } - if (currentStartLevel == StartLevelService.STARTLEVEL_STATES) { - initialImport(); + if (!initialized.isDone()) { + addFiles(listFiles(watchPath, watchSubDirectories)).thenRun(() -> initialized.complete(null)); } else { scriptMap.values().stream().sorted() .filter(ref -> needsStartLevelProcessing(ref, previousLevel, currentStartLevel)) diff --git a/bundles/org.openhab.core.automation.module.script.rulesupport/src/test/java/org/openhab/core/automation/module/script/rulesupport/loader/AbstractScriptFileWatcherTest.java b/bundles/org.openhab.core.automation.module.script.rulesupport/src/test/java/org/openhab/core/automation/module/script/rulesupport/loader/AbstractScriptFileWatcherTest.java index dc68e373d..10f3ef141 100644 --- a/bundles/org.openhab.core.automation.module.script.rulesupport/src/test/java/org/openhab/core/automation/module/script/rulesupport/loader/AbstractScriptFileWatcherTest.java +++ b/bundles/org.openhab.core.automation.module.script.rulesupport/src/test/java/org/openhab/core/automation/module/script/rulesupport/loader/AbstractScriptFileWatcherTest.java @@ -554,7 +554,7 @@ class AbstractScriptFileWatcherTest extends JavaTest { } @Test - public void testDirectoryRemoved() { + public void testFileRemoved() { scriptFileWatcher = createScriptFileWatcher(true); when(scriptEngineManagerMock.isSupported("js")).thenReturn(true); ScriptEngineContainer scriptEngineContainer = mock(ScriptEngineContainer.class); @@ -565,16 +565,18 @@ class AbstractScriptFileWatcherTest extends JavaTest { Path p1 = getFile("dir/script.js"); Path p2 = getFile("dir/script2.js"); - Path d = p1.getParent(); scriptFileWatcher.processWatchEvent(CREATE, p1); scriptFileWatcher.processWatchEvent(CREATE, p2); - scriptFileWatcher.processWatchEvent(DELETE, d); awaitEmptyQueue(); verify(scriptEngineManagerMock, timeout(DEFAULT_TEST_TIMEOUT_MS)).createScriptEngine("js", p1.toString()); verify(scriptEngineManagerMock, timeout(DEFAULT_TEST_TIMEOUT_MS)).createScriptEngine("js", p2.toString()); + + scriptFileWatcher.processWatchEvent(DELETE, p1); + scriptFileWatcher.processWatchEvent(DELETE, p2); + verify(scriptEngineManagerMock, timeout(DEFAULT_TEST_TIMEOUT_MS)).removeEngine(p1.toString()); verify(scriptEngineManagerMock, timeout(DEFAULT_TEST_TIMEOUT_MS)).removeEngine(p2.toString()); } @@ -622,6 +624,9 @@ class AbstractScriptFileWatcherTest extends JavaTest { scriptFileWatcher = createScriptFileWatcher(true); when(startLevelServiceMock.getStartLevel()).thenReturn(StartLevelService.STARTLEVEL_RULEENGINE); AbstractScriptFileWatcher watcher = createScriptFileWatcher(); + watcher.activate(); + watcher.onReadyMarkerAdded(new ReadyMarker(StartLevelService.STARTLEVEL_MARKER_TYPE, + Integer.toString(StartLevelService.STARTLEVEL_STATES))); waitForAssert(() -> assertThat(watcher.ifInitialized().isDone(), is(true)));