Skip to content

Commit

Permalink
Improve AbstractScriptFileWatcher initialization order and file handl…
Browse files Browse the repository at this point in the history 
…ing (#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 <jcode@tanagra.id.au>
  • Loading branch information
@jimtng
jimtng authored Mar 18, 2023
1 parent 79c7ec8 commit 1e55914
Show file tree
Hide file tree
Showing 2 changed files with 42 additions and 56 deletions.
87 changes: 34 additions & 53 deletions ...g/openhab/core/automation/module/script/rulesupport/loader/AbstractScriptFileWatcher.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -104,16 +104,9 @@ public AbstractScriptFileWatcher(final WatchService watchService, final ScriptEn
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;
}

/**
Expand Down Expand Up @@ -144,6 +137,9 @@ public void activate() {
} 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);
}

Expand Down Expand Up @@ -223,27 +219,22 @@ private List<Path> listFiles(Path path, boolean includeSubDirectory) {

@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<String> 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));
}
}

Expand All @@ -269,20 +260,21 @@ private CompletableFuture<Void> removeFile(String scriptIdentifier) {
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;
return null;
});
} finally {
if (scriptMap.containsKey(scriptIdentifier)) {
logger.warn("Failed to unload script '{}'", scriptIdentifier);
}

if (ref.getLoadedStatus().get()) {
manager.removeEngine(scriptIdentifier);
logger.debug("Unloaded script '{}'", scriptIdentifier);
} else {
logger.debug("Dequeued script '{}'", scriptIdentifier);
}
} finally {
scriptLockMap.remove(scriptIdentifier);
lock.unlock();
}
Expand Down Expand Up @@ -353,20 +345,6 @@ private boolean createAndLoad(ScriptFileReference ref) {
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);
Expand All @@ -381,13 +359,16 @@ public synchronized void onReadyMarkerAdded(ReadyMarker readyMarker) {
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))
Expand Down
11 changes: 8 additions & 3 deletions ...enhab/core/automation/module/script/rulesupport/loader/AbstractScriptFileWatcherTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -554,7 +554,7 @@ public void testSortsAllFilesInNewDirectory() {
}

@Test
public void testDirectoryRemoved() {
public void testFileRemoved() {
scriptFileWatcher = createScriptFileWatcher(true);
when(scriptEngineManagerMock.isSupported("js")).thenReturn(true);
ScriptEngineContainer scriptEngineContainer = mock(ScriptEngineContainer.class);
Expand All @@ -565,16 +565,18 @@ public void testDirectoryRemoved() {

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());
}
Expand Down Expand Up @@ -622,6 +624,9 @@ public void testIfInitializedForLateInitialization() {
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)));

Expand Down

0 comments on commit 1e55914

Please sign in to comment.