From c633b919ab77ddef6012364b6edd58089aabf33e Mon Sep 17 00:00:00 2001 From: Adrien Grand Date: Tue, 13 Feb 2018 11:27:11 +0100 Subject: [PATCH] Watcher's ExecutionService is not thread-safe. (elastic/x-pack-elasticsearch#3861) If start, stop and/or clearExecutions are called concurrently, you could end up in an inconsistent state where `currentExecutions.sealAndAwaitEmpty` is called twice on the same instance. These `synchronized` keywords should make it a bit better, though I suspect there are other issues and thread safety of this class should be reviewed. This might have caused https://internal-ci.elastic.co/job/elastic+x-pack-elasticsearch+5.6+multijob-unix-compatibility/932/consoleFull Original commit: elastic/x-pack-elasticsearch@2f7e7a34d983b5c86e57bdb5aa43649d09bed469 --- .../xpack/watcher/execution/ExecutionService.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/execution/ExecutionService.java b/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/execution/ExecutionService.java index cf7ef56d296..9a936010758 100644 --- a/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/execution/ExecutionService.java +++ b/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/execution/ExecutionService.java @@ -109,7 +109,7 @@ public class ExecutionService extends AbstractComponent { this.indexDefaultTimeout = settings.getAsTime("xpack.watcher.internal.ops.index.default_timeout", TimeValue.timeValueSeconds(30)); } - public void start() throws Exception { + public synchronized void start() throws Exception { if (started.get()) { return; } @@ -133,7 +133,7 @@ public class ExecutionService extends AbstractComponent { return triggeredWatchStore.validate(state) && HistoryStore.validate(state); } - public void stop() { + public synchronized void stop() { if (started.compareAndSet(true, false)) { logger.debug("stopping execution service"); // We could also rely on the shutdown in #updateSettings call, but @@ -541,7 +541,7 @@ public class ExecutionService extends AbstractComponent { * This clears out the current executions and sets new empty current executions * This is needed, because when this method is called, watcher keeps running, so sealing executions would be a bad idea */ - public void clearExecutions() { + public synchronized void clearExecutions() { currentExecutions.sealAndAwaitEmpty(maxStopTimeout); currentExecutions = new CurrentExecutions(); }