Skip to content

Commit 384114f

Browse files
imotovimotov
committed
Fix NPE in ScriptService when script file with no extension is deleted
Fixes #7689
1 parent 349b7a3 commit 384114f

File tree

4 files changed

+199
-14
lines changed

4 files changed

+199
-14
lines changed

‎src/main/java/org/elasticsearch/script/ScriptService.java

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -537,8 +537,10 @@ public void onFileCreated(File file) {
537537
@Override
538538
public void onFileDeleted(File file) {
539539
Tuple<String, String> scriptNameExt = scriptNameExt(file);
540-
logger.info("removing script file [{}]", file.getAbsolutePath());
541-
staticCache.remove(scriptNameExt.v1());
540+
if (scriptNameExt != null) {
541+
logger.info("removing script file [{}]", file.getAbsolutePath());
542+
staticCache.remove(scriptNameExt.v1());
543+
}
542544
}
543545

544546
@Override

‎src/main/java/org/elasticsearch/watcher/FileWatcher.java

Lines changed: 37 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,9 @@
1818
*/
1919
package org.elasticsearch.watcher;
2020

21+
import org.elasticsearch.common.logging.ESLogger;
22+
import org.elasticsearch.common.logging.Loggers;
23+
2124
import java.io.File;
2225
import java.util.Arrays;
2326

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

3134
private FileObserver rootFileObserver;
3235

36+
private static final ESLogger logger = Loggers.getLogger(FileWatcher.class);
37+
3338
/**
3439
* Creates new file watcher on the given directory
3540
*/
@@ -228,32 +233,49 @@ private void deleteChild(int child) {
228233

229234
private void onFileCreated(boolean initial) {
230235
for (FileChangesListener listener : listeners()) {
231-
if (initial) {
232-
listener.onFileInit(file);
233-
} else {
234-
listener.onFileCreated(file);
236+
try {
237+
if (initial) {
238+
listener.onFileInit(file);
239+
} else {
240+
listener.onFileCreated(file);
241+
}
242+
} catch (Throwable t) {
243+
logger.warn("cannot notify file changes listener", t);
235244
}
236245
}
237246
}
238247

239248
private void onFileDeleted() {
240249
for (FileChangesListener listener : listeners()) {
241-
listener.onFileDeleted(file);
250+
try {
251+
listener.onFileDeleted(file);
252+
} catch (Throwable t) {
253+
logger.warn("cannot notify file changes listener", t);
254+
}
242255
}
243256
}
244257

245258
private void onFileChanged() {
246259
for (FileChangesListener listener : listeners()) {
247-
listener.onFileChanged(file);
260+
try {
261+
listener.onFileChanged(file);
262+
} catch (Throwable t) {
263+
logger.warn("cannot notify file changes listener", t);
264+
}
265+
248266
}
249267
}
250268

251269
private void onDirectoryCreated(boolean initial) {
252270
for (FileChangesListener listener : listeners()) {
253-
if (initial) {
254-
listener.onDirectoryInit(file);
255-
} else {
256-
listener.onDirectoryCreated(file);
271+
try {
272+
if (initial) {
273+
listener.onDirectoryInit(file);
274+
} else {
275+
listener.onDirectoryCreated(file);
276+
}
277+
} catch (Throwable t) {
278+
logger.warn("cannot notify file changes listener", t);
257279
}
258280
}
259281
children = listChildren(initial);
@@ -265,7 +287,11 @@ private void onDirectoryDeleted() {
265287
deleteChild(child);
266288
}
267289
for (FileChangesListener listener : listeners()) {
268-
listener.onDirectoryDeleted(file);
290+
try {
291+
listener.onDirectoryDeleted(file);
292+
} catch (Throwable t) {
293+
logger.warn("cannot notify file changes listener", t);
294+
}
269295
}
270296
}
271297

‎src/main/java/org/elasticsearch/watcher/ResourceWatcherService.java

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -137,6 +137,26 @@ public <W extends ResourceWatcher> WatcherHandle<W> add(W watcher, Frequency fre
137137
}
138138
}
139139

140+
public void notifyNow() {
141+
notifyNow(Frequency.MEDIUM);
142+
}
143+
144+
public void notifyNow(Frequency frequency) {
145+
switch (frequency) {
146+
case LOW:
147+
lowMonitor.run();
148+
break;
149+
case MEDIUM:
150+
mediumMonitor.run();
151+
break;
152+
case HIGH:
153+
highMonitor.run();
154+
break;
155+
default:
156+
throw new ElasticsearchIllegalArgumentException("Unknown frequency [" + frequency + "]");
157+
}
158+
}
159+
140160
static class ResourceMonitor implements Runnable {
141161

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

157177
@Override
158-
public void run() {
178+
public synchronized void run() {
159179
for(ResourceWatcher watcher : watchers) {
160180
watcher.checkAndNotify();
161181
}

‎src/test/java/org/elasticsearch/script/ScriptServiceTests.java

Lines changed: 137 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,137 @@
1+
/*
2+
* Licensed to Elasticsearch under one or more contributor
3+
* license agreements. See the NOTICE file distributed with
4+
* this work for additional information regarding copyright
5+
* ownership. Elasticsearch licenses this file to you under
6+
* the Apache License, Version 2.0 (the "License"); you may
7+
* not use this file except in compliance with the License.
8+
* You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing,
13+
* software distributed under the License is distributed on an
14+
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
15+
* KIND, either express or implied. See the License for the
16+
* specific language governing permissions and limitations
17+
* under the License.
18+
*/
19+
package org.elasticsearch.script;
20+
21+
import com.carrotsearch.ant.tasks.junit4.dependencies.com.google.common.collect.ImmutableSet;
22+
import org.elasticsearch.ElasticsearchIllegalArgumentException;
23+
import org.elasticsearch.common.Nullable;
24+
import org.elasticsearch.common.io.Streams;
25+
import org.elasticsearch.common.settings.Settings;
26+
import org.elasticsearch.env.Environment;
27+
import org.elasticsearch.search.lookup.SearchLookup;
28+
import org.elasticsearch.test.ElasticsearchTestCase;
29+
import org.elasticsearch.watcher.ResourceWatcherService;
30+
import org.junit.Test;
31+
32+
import java.io.File;
33+
import java.io.IOException;
34+
import java.util.Map;
35+
36+
import static org.elasticsearch.common.settings.ImmutableSettings.settingsBuilder;
37+
import static org.hamcrest.Matchers.containsString;
38+
import static org.hamcrest.Matchers.equalTo;
39+
40+
/**
41+
*
42+
*/
43+
public class ScriptServiceTests extends ElasticsearchTestCase {
44+
45+
@Test
46+
public void testScriptsWithoutExtensions() throws IOException {
47+
File homeFolder = newTempDir();
48+
File genericConfigFolder = newTempDir();
49+
50+
Settings settings = settingsBuilder()
51+
.put("path.conf", genericConfigFolder)
52+
.put("path.home", homeFolder)
53+
.build();
54+
Environment environment = new Environment(settings);
55+
56+
ResourceWatcherService resourceWatcherService = new ResourceWatcherService(settings, null);
57+
58+
logger.info("--> setup script service");
59+
ScriptService scriptService = new ScriptService(settings, environment, ImmutableSet.of(new TestEngineService()), resourceWatcherService);
60+
File scriptsFile = new File(genericConfigFolder, "scripts");
61+
assertThat(scriptsFile.mkdir(), equalTo(true));
62+
resourceWatcherService.notifyNow();
63+
64+
logger.info("--> setup two test files one with extension and another without");
65+
File testFileNoExt = new File(scriptsFile, "test_no_ext");
66+
File testFileWithExt = new File(scriptsFile, "test_script.tst");
67+
Streams.copy("test_file_no_ext".getBytes("UTF-8"), testFileNoExt);
68+
Streams.copy("test_file".getBytes("UTF-8"), testFileWithExt);
69+
resourceWatcherService.notifyNow();
70+
71+
logger.info("--> verify that file with extension was correctly processed");
72+
CompiledScript compiledScript = scriptService.compile("test", "test_script", ScriptService.ScriptType.FILE);
73+
assertThat(compiledScript.compiled(), equalTo((Object) "compiled_test_file"));
74+
75+
logger.info("--> delete both files");
76+
assertThat(testFileNoExt.delete(), equalTo(true));
77+
assertThat(testFileWithExt.delete(), equalTo(true));
78+
resourceWatcherService.notifyNow();
79+
80+
logger.info("--> verify that file with extension was correctly removed");
81+
try {
82+
scriptService.compile("test", "test_script", ScriptService.ScriptType.FILE);
83+
fail("the script test_script should no longe exist");
84+
} catch (ElasticsearchIllegalArgumentException ex) {
85+
assertThat(ex.getMessage(), containsString("Unable to find on disk script test_script"));
86+
}
87+
}
88+
89+
public static class TestEngineService implements ScriptEngineService {
90+
91+
@Override
92+
public String[] types() {
93+
return new String[] {"test"};
94+
}
95+
96+
@Override
97+
public String[] extensions() {
98+
return new String[] {"test", "tst"};
99+
}
100+
101+
@Override
102+
public boolean sandboxed() {
103+
return false;
104+
}
105+
106+
@Override
107+
public Object compile(String script) {
108+
return "compiled_" + script;
109+
}
110+
111+
@Override
112+
public ExecutableScript executable(final Object compiledScript, @Nullable Map<String, Object> vars) {
113+
return null;
114+
}
115+
116+
@Override
117+
public SearchScript search(Object compiledScript, SearchLookup lookup, @Nullable Map<String, Object> vars) {
118+
return null;
119+
}
120+
121+
@Override
122+
public Object execute(Object compiledScript, Map<String, Object> vars) {
123+
return null;
124+
}
125+
126+
@Override
127+
public Object unwrap(Object value) {
128+
return null;
129+
}
130+
131+
@Override
132+
public void close() {
133+
134+
}
135+
}
136+
137+
}

0 commit comments

Comments
 (0)