Skip to content

Commit

Permalink
Fix NPE in ScriptService when script file with no extension is deleted
Browse files Browse the repository at this point in the history
Fixes #7689
  • Loading branch information
imotov committed Oct 3, 2014
1 parent f971ca1 commit f8398fe
Show file tree
Hide file tree
Showing 4 changed files with 199 additions and 14 deletions.
6 changes: 4 additions & 2 deletions src/main/java/org/elasticsearch/script/ScriptService.java
Expand Up @@ -543,8 +543,10 @@ public void onFileCreated(File file) {
@Override
public void onFileDeleted(File file) {
Tuple<String, String> scriptNameExt = scriptNameExt(file);
logger.info("removing script file [{}]", file.getAbsolutePath());
staticCache.remove(scriptNameExt.v1());
if (scriptNameExt != null) {
logger.info("removing script file [{}]", file.getAbsolutePath());
staticCache.remove(scriptNameExt.v1());
}
}

@Override
Expand Down
48 changes: 37 additions & 11 deletions src/main/java/org/elasticsearch/watcher/FileWatcher.java
Expand Up @@ -18,6 +18,9 @@
*/
package org.elasticsearch.watcher;

import org.elasticsearch.common.logging.ESLogger;
import org.elasticsearch.common.logging.Loggers;

import java.io.File;
import java.util.Arrays;

Expand All @@ -30,6 +33,8 @@ public class FileWatcher extends AbstractResourceWatcher<FileChangesListener> {

private FileObserver rootFileObserver;

private static final ESLogger logger = Loggers.getLogger(FileWatcher.class);

/**
* Creates new file watcher on the given directory
*/
Expand Down Expand Up @@ -228,32 +233,49 @@ private void deleteChild(int child) {

private void onFileCreated(boolean initial) {
for (FileChangesListener listener : listeners()) {
if (initial) {
listener.onFileInit(file);
} else {
listener.onFileCreated(file);
try {
if (initial) {
listener.onFileInit(file);
} else {
listener.onFileCreated(file);
}
} catch (Throwable t) {
logger.warn("cannot notify file changes listener", t);
}
}
}

private void onFileDeleted() {
for (FileChangesListener listener : listeners()) {
listener.onFileDeleted(file);
try {
listener.onFileDeleted(file);
} catch (Throwable t) {
logger.warn("cannot notify file changes listener", t);
}
}
}

private void onFileChanged() {
for (FileChangesListener listener : listeners()) {
listener.onFileChanged(file);
try {
listener.onFileChanged(file);
} catch (Throwable t) {
logger.warn("cannot notify file changes listener", t);
}

}
}

private void onDirectoryCreated(boolean initial) {
for (FileChangesListener listener : listeners()) {
if (initial) {
listener.onDirectoryInit(file);
} else {
listener.onDirectoryCreated(file);
try {
if (initial) {
listener.onDirectoryInit(file);
} else {
listener.onDirectoryCreated(file);
}
} catch (Throwable t) {
logger.warn("cannot notify file changes listener", t);
}
}
children = listChildren(initial);
Expand All @@ -265,7 +287,11 @@ private void onDirectoryDeleted() {
deleteChild(child);
}
for (FileChangesListener listener : listeners()) {
listener.onDirectoryDeleted(file);
try {
listener.onDirectoryDeleted(file);
} catch (Throwable t) {
logger.warn("cannot notify file changes listener", t);
}
}
}

Expand Down
Expand Up @@ -137,6 +137,26 @@ public <W extends ResourceWatcher> WatcherHandle<W> add(W watcher, Frequency fre
}
}

public void notifyNow() {
notifyNow(Frequency.MEDIUM);
}

public void notifyNow(Frequency frequency) {
switch (frequency) {
case LOW:
lowMonitor.run();
break;
case MEDIUM:
mediumMonitor.run();
break;
case HIGH:
highMonitor.run();
break;
default:
throw new ElasticsearchIllegalArgumentException("Unknown frequency [" + frequency + "]");
}
}

static class ResourceMonitor implements Runnable {

final TimeValue interval;
Expand All @@ -155,7 +175,7 @@ private <W extends ResourceWatcher> WatcherHandle<W> add(W watcher) {
}

@Override
public void run() {
public synchronized void run() {
for(ResourceWatcher watcher : watchers) {
watcher.checkAndNotify();
}
Expand Down
137 changes: 137 additions & 0 deletions src/test/java/org/elasticsearch/script/ScriptServiceTests.java
@@ -0,0 +1,137 @@
/*
* Licensed to Elasticsearch under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
package org.elasticsearch.script;

import com.carrotsearch.ant.tasks.junit4.dependencies.com.google.common.collect.ImmutableSet;
import org.elasticsearch.ElasticsearchIllegalArgumentException;
import org.elasticsearch.common.Nullable;
import org.elasticsearch.common.io.Streams;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.env.Environment;
import org.elasticsearch.search.lookup.SearchLookup;
import org.elasticsearch.test.ElasticsearchTestCase;
import org.elasticsearch.watcher.ResourceWatcherService;
import org.junit.Test;

import java.io.File;
import java.io.IOException;
import java.util.Map;

import static org.elasticsearch.common.settings.ImmutableSettings.settingsBuilder;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.equalTo;

/**
*
*/
public class ScriptServiceTests extends ElasticsearchTestCase {

@Test
public void testScriptsWithoutExtensions() throws IOException {
File homeFolder = newTempDir();
File genericConfigFolder = newTempDir();

Settings settings = settingsBuilder()
.put("path.conf", genericConfigFolder)
.put("path.home", homeFolder)
.build();
Environment environment = new Environment(settings);

ResourceWatcherService resourceWatcherService = new ResourceWatcherService(settings, null);

logger.info("--> setup script service");
ScriptService scriptService = new ScriptService(settings, environment, ImmutableSet.of(new TestEngineService()), resourceWatcherService);
File scriptsFile = new File(genericConfigFolder, "scripts");
assertThat(scriptsFile.mkdir(), equalTo(true));
resourceWatcherService.notifyNow();

logger.info("--> setup two test files one with extension and another without");
File testFileNoExt = new File(scriptsFile, "test_no_ext");
File testFileWithExt = new File(scriptsFile, "test_script.tst");
Streams.copy("test_file_no_ext".getBytes("UTF-8"), testFileNoExt);
Streams.copy("test_file".getBytes("UTF-8"), testFileWithExt);
resourceWatcherService.notifyNow();

logger.info("--> verify that file with extension was correctly processed");
CompiledScript compiledScript = scriptService.compile("test", "test_script", ScriptService.ScriptType.FILE);
assertThat(compiledScript.compiled(), equalTo((Object) "compiled_test_file"));

logger.info("--> delete both files");
assertThat(testFileNoExt.delete(), equalTo(true));
assertThat(testFileWithExt.delete(), equalTo(true));
resourceWatcherService.notifyNow();

logger.info("--> verify that file with extension was correctly removed");
try {
scriptService.compile("test", "test_script", ScriptService.ScriptType.FILE);
fail("the script test_script should no longe exist");
} catch (ElasticsearchIllegalArgumentException ex) {
assertThat(ex.getMessage(), containsString("Unable to find on disk script test_script"));
}
}

public static class TestEngineService implements ScriptEngineService {

@Override
public String[] types() {
return new String[] {"test"};
}

@Override
public String[] extensions() {
return new String[] {"test", "tst"};
}

@Override
public boolean sandboxed() {
return false;
}

@Override
public Object compile(String script) {
return "compiled_" + script;
}

@Override
public ExecutableScript executable(final Object compiledScript, @Nullable Map<String, Object> vars) {
return null;
}

@Override
public SearchScript search(Object compiledScript, SearchLookup lookup, @Nullable Map<String, Object> vars) {
return null;
}

@Override
public Object execute(Object compiledScript, Map<String, Object> vars) {
return null;
}

@Override
public Object unwrap(Object value) {
return null;
}

@Override
public void close() {

}
}

}

0 comments on commit f8398fe

Please sign in to comment.