Refactor the StopWatch API for easy safe exception handling
The previous StopWatch API was similar to the google commons API in that
it used calls to public start() and stop() methods. This approach is
easy to get wrong if the call to stop() is not placed in a finally{}
clause. Since the task plugin code base did not call stop() from
finally{} clauses, it was succeptible to timers not being stopped, and
other bugs when execeptions were encountered. Since 3.5 uses more
RuntimeExceptions than 2.7, its code base is even more vulnerable to
this. The --task--applicable timings in 3.5 have been suspicious at
times, showing times much longer than possible. This is believed to be
due to accumulated overlapping StopWatch timings due to some of them not
being stopped when RuntimeExceptions are encountered with INVALID tasks.
Redesign the StopWatch as an AutoCloseable and provide easy APIs to use
it safely in a try-with-resource, or with SAMs, and adapt the callers to
use the new APIs.
Change-Id: Ie3073c9f3d69435b67b12678f8c0f255339c76cc
diff --git a/src/main/java/com/googlesource/gerrit/plugins/task/HitHashMap.java b/src/main/java/com/googlesource/gerrit/plugins/task/HitHashMap.java
index 453c118..7a23fc6 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/task/HitHashMap.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/task/HitHashMap.java
@@ -28,8 +28,6 @@
public int size;
public long sumNanosecondsLoading;
public List<Object> elements;
-
- protected transient StopWatch loadingStopWatch;
}
public static final long serialVersionUID = 1;
@@ -53,14 +51,6 @@
return v;
}
- public V getOrStartLoad(K key) {
- V v = get(key);
- if (v == null) {
- startLoad();
- }
- return v;
- }
-
@Override
public V getOrDefault(Object key, V dv) {
V v = get(key);
@@ -71,14 +61,15 @@
}
@Override
+ @SuppressWarnings("try")
public V computeIfAbsent(K key, Function<? super K, ? extends V> mappingFunction) {
- V v = getOrStartLoad(key);
+ V v = get(key);
if (v == null) {
- v = mappingFunction.apply(key);
+ try (StopWatch stopWatch = createLoadingStopWatch()) {
+ v = mappingFunction.apply(key);
+ }
if (v != null) {
put(key, v);
- } else {
- stopLoad(key);
}
}
return v;
@@ -86,7 +77,6 @@
@Override
public V put(K key, V value) {
- stopLoad(key);
if (statistics != null && value instanceof TracksStatistics) {
((TracksStatistics) value).ensureStatistics();
}
@@ -138,23 +128,9 @@
throw new UnsupportedOperationException(); // Todo if needed
}
- public void startLoad() {
- if (statistics != null && statistics.loadingStopWatch != null) {
- statistics.loadingStopWatch.start();
- }
- }
-
- public void stopLoad(K key) {
- if (statistics != null && statistics.loadingStopWatch != null) {
- statistics.loadingStopWatch.stop();
- }
- }
-
@Override
public void initStatistics() {
statistics = new Statistics();
- statistics.loadingStopWatch =
- new StopWatch().enable().setConsumer(ns -> statistics.sumNanosecondsLoading += ns);
}
@Override
@@ -164,6 +140,13 @@
}
}
+ public StopWatch createLoadingStopWatch() {
+ if (statistics == null) {
+ return StopWatch.DISABLED;
+ }
+ return new StopWatch.Enabled().setNanosConsumer(ns -> statistics.sumNanosecondsLoading += ns);
+ }
+
@Override
public Object getStatistics() {
statistics.size = size();
@@ -175,7 +158,6 @@
if (!elementStatistics.isEmpty()) {
statistics.elements = elementStatistics;
}
- statistics.loadingStopWatch = null;
return statistics;
}
}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/task/PredicateCache.java b/src/main/java/com/googlesource/gerrit/plugins/task/PredicateCache.java
index 53e2ac8..39c99b4 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/task/PredicateCache.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/task/PredicateCache.java
@@ -75,14 +75,15 @@
return statistics;
}
+ @SuppressWarnings("try")
public Predicate<ChangeData> getPredicate(String query) throws QueryParseException {
ThrowingProvider<Predicate<ChangeData>, QueryParseException> predProvider =
- predicatesByQuery.getOrStartLoad(query);
+ predicatesByQuery.get(query);
if (predProvider != null) {
return predProvider.get();
}
// never seen 'query' before
- try {
+ try (StopWatch stopWatch = predicatesByQuery.createLoadingStopWatch()) {
Predicate<ChangeData> pred = cqb.parse(query);
predicatesByQuery.put(query, new ThrowingProvider.Entry<>(pred));
return pred;
diff --git a/src/main/java/com/googlesource/gerrit/plugins/task/SamTryWrapper.java b/src/main/java/com/googlesource/gerrit/plugins/task/SamTryWrapper.java
new file mode 100644
index 0000000..39679ff
--- /dev/null
+++ b/src/main/java/com/googlesource/gerrit/plugins/task/SamTryWrapper.java
@@ -0,0 +1,84 @@
+// Copyright (C) 2023 The Android Open Source Project
+//
+// Licensed 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 com.googlesource.gerrit.plugins.task;
+
+import java.util.function.BiConsumer;
+import java.util.function.BiFunction;
+import java.util.function.Consumer;
+import java.util.function.Function;
+import java.util.function.Supplier;
+
+/**
+ * Class designed to make SAM calls wrapped by an AutoCloseable. This is usefull with AutoCloseables
+ * which do not provide resources which are directly needed during the SAM call, but rather with
+ * AutoCloseables which likely manage external resources or state such as a locks or timers.
+ */
+public abstract class SamTryWrapper<A extends AutoCloseable> {
+ protected abstract A getAutoCloseable();
+
+ @SuppressWarnings("try")
+ public void run(Runnable runnable) {
+ try (A autoCloseable = getAutoCloseable()) {
+ runnable.run();
+ } catch (Exception e) {
+ throw new RuntimeException(e);
+ }
+ }
+
+ @SuppressWarnings("try")
+ public <T> T get(Supplier<T> supplier) {
+ try (A autoCloseable = getAutoCloseable()) {
+ return supplier.get();
+ } catch (Exception e) {
+ throw new RuntimeException(e);
+ }
+ }
+
+ @SuppressWarnings("try")
+ public <T> void accept(Consumer<T> consumer, T t) {
+ try (A autoCloseable = getAutoCloseable()) {
+ consumer.accept(t);
+ } catch (Exception e) {
+ throw new RuntimeException(e);
+ }
+ }
+
+ @SuppressWarnings("try")
+ public <T, U> void accept(BiConsumer<T, U> consumer, T t, U u) {
+ try (A autoCloseable = getAutoCloseable()) {
+ consumer.accept(t, u);
+ } catch (Exception e) {
+ throw new RuntimeException(e);
+ }
+ }
+
+ @SuppressWarnings("try")
+ public <T, R> R apply(Function<T, R> func, T t) {
+ try (A autoCloseable = getAutoCloseable()) {
+ return func.apply(t);
+ } catch (Exception e) {
+ throw new RuntimeException(e);
+ }
+ }
+
+ @SuppressWarnings("try")
+ public <T, U, R> R apply(BiFunction<T, U, R> func, T t, U u) {
+ try (A autoCloseable = getAutoCloseable()) {
+ return func.apply(t, u);
+ } catch (Exception e) {
+ throw new RuntimeException(e);
+ }
+ }
+}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/task/StopWatch.java b/src/main/java/com/googlesource/gerrit/plugins/task/StopWatch.java
index f7d36e7..11fe588 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/task/StopWatch.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/task/StopWatch.java
@@ -18,65 +18,122 @@
import java.util.concurrent.TimeUnit;
import java.util.function.LongConsumer;
-public class StopWatch {
- protected Stopwatch stopwatch;
- protected LongConsumer consumer;
- protected long nanoseconds;
+/**
+ * StopWatches with APIs designed to make it easy to disable, and to make robust in the face of
+ * exceptions.
+ *
+ * <p>The Stopwatch class from google commons is used by placing start() and stop() calls around
+ * code sections which need to be timed. This approach can be problematic since the code being timed
+ * could throw an exception and if the stop() is not in a finally clause, then it will likely never
+ * get called, potentially causing bad timings, or worse programatic issues elsewhere due to double
+ * calls to start(). The need for a finally clause to make things safe is an obvious hint that using
+ * an AutoCloseable approach is likely going to be safer. With that in mind, the two API approaches
+ * provided by these StopWatch classes are:
+ *
+ * <ol>
+ * <li>The timed try-with-resource API. Use the StopWatch.Enabled class for this API.
+ * <li>The timed SAM evaluation API. Use the StopWatch.Runner.Enabled class for these APIs.
+ * </ol>
+ *
+ * <p>Finally, the commons stopwatch API also does not provide an easy way to disable timings at
+ * runtime when they are not desired, so the DISABLED classes can be used for this with both
+ * approaches above, and thus provide low cost runtime substitutes for either the StopWatch.Enabled
+ * or StopWatch.Runner.Enabled classes.
+ */
+public interface StopWatch extends AutoCloseable {
+ /** Designed for the greatest simplicity to time SAM executions. */
+ public abstract static class Runner extends SamTryWrapper<AutoCloseable> {
+ public static class Enabled extends Runner {
+ protected LongConsumer nanosConsumer = EMPTY_LONG_CONSUMER;
- public StopWatch enableIfNonNull(Object statistics) {
- if (statistics != null) {
- enable();
- }
- return this;
- }
+ @Override
+ protected AutoCloseable getAutoCloseable() {
+ return new StopWatch.Enabled().setNanosConsumer(nanosConsumer);
+ }
- public StopWatch enable() {
- stopwatch = Stopwatch.createUnstarted();
- return this;
- }
-
- public StopWatch run(Runnable runnable) {
- start();
- runnable.run();
- stop();
- return this;
- }
-
- public StopWatch start() {
- if (stopwatch != null && !stopwatch.isRunning()) {
- stopwatch.start();
- }
- return this;
- }
-
- public StopWatch stop() {
- if (stopwatch != null && stopwatch.isRunning()) {
- stopwatch.stop();
- if (consumer != null) {
- consume(consumer);
+ @Override
+ public Runner setNanosConsumer(LongConsumer nanosConsumer) {
+ this.nanosConsumer = nanosConsumer;
+ return this;
}
}
- return this;
- }
- public StopWatch setConsumer(LongConsumer consumer) {
- if (consumer != null) {
- stopwatch = Stopwatch.createUnstarted();
+ /** May be used anywhere that Enabled can be used */
+ public static final Runner DISABLED =
+ new Runner() {
+ @Override
+ protected AutoCloseable getAutoCloseable() {
+ return () -> {};
+ }
+ };
+
+ public Runner setNanosConsumer(LongConsumer nanosConsumer) {
+ return this;
}
- this.consumer = consumer;
- return this;
}
- public StopWatch consume(LongConsumer consumer) {
- if (stopwatch != null) {
- consumer.accept(get());
+ /** Should be created and used only within a try-with-resource */
+ public static class Enabled implements StopWatch {
+ protected LongConsumer nanosConsumer = EMPTY_LONG_CONSUMER;
+ protected Stopwatch stopwatch = Stopwatch.createStarted();
+
+ @Override
+ public StopWatch setNanosConsumer(LongConsumer nanosConsumer) {
+ this.nanosConsumer = nanosConsumer;
+ return this;
}
+
+ @Override
+ public void close() {
+ stopwatch.stop();
+ nanosConsumer.accept(stopwatch.elapsed(TimeUnit.NANOSECONDS));
+ }
+ }
+
+ /**
+ * A easy way to build a timer which needes to be enabled/disabled based on a runtime boolean.
+ *
+ * <p>Example Usage:
+ *
+ * <p><code>
+ * try (StopWatch stopWatch =
+ * StopWatch.builder().enabled(myBoolean).build().setNanosConsumer(myConsumer)) {
+ * // Code to be timed here...
+ * }
+ * </code>
+ */
+ public static class Builder {
+ protected static class Enabled extends Builder {
+ @Override
+ public StopWatch build() {
+ return new StopWatch.Enabled();
+ }
+ }
+
+ protected static final Builder ENABLED = new Enabled();
+ protected static final Builder DISABLED = new Builder();
+
+ public Builder enabled(boolean enabled) {
+ return enabled ? ENABLED : this;
+ }
+
+ public StopWatch build() {
+ return StopWatch.DISABLED;
+ }
+ }
+
+ /** May be used anywhere that Enabled can be used */
+ public static final StopWatch DISABLED = new StopWatch() {};
+
+ public static final LongConsumer EMPTY_LONG_CONSUMER = l -> {};
+
+ public static Builder builder() {
+ return Builder.DISABLED;
+ }
+
+ default StopWatch setNanosConsumer(LongConsumer nanosConsumer) {
return this;
}
- public long get() {
- nanoseconds += stopwatch.elapsed(TimeUnit.NANOSECONDS);
- stopwatch.reset();
- return nanoseconds;
- }
+ default void close() {}
}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/task/TaskTree.java b/src/main/java/com/googlesource/gerrit/plugins/task/TaskTree.java
index 172208a..26f7260 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/task/TaskTree.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/task/TaskTree.java
@@ -592,14 +592,17 @@
return BranchNameKey.create(allUsers.get(), RefNames.refsUsers(acct));
}
+ @SuppressWarnings("try")
public List<ChangeData> query(String query) throws StorageException, QueryParseException {
- List<ChangeData> changeDataList = changesByNamesFactoryQuery.getOrStartLoad(query);
+ List<ChangeData> changeDataList = changesByNamesFactoryQuery.get(query);
if (changeDataList == null) {
- changeDataList =
- changeQueryProcessorProvider
- .get()
- .query(changeQueryBuilderProvider.get().parse(query))
- .entities();
+ try (StopWatch stopWatch = changesByNamesFactoryQuery.createLoadingStopWatch()) {
+ changeDataList =
+ changeQueryProcessorProvider
+ .get()
+ .query(changeQueryBuilderProvider.get().parse(query))
+ .entities();
+ }
changesByNamesFactoryQuery.put(query, changeDataList);
}
return changeDataList;
diff --git a/src/main/java/com/googlesource/gerrit/plugins/task/properties/CopyOnWrite.java b/src/main/java/com/googlesource/gerrit/plugins/task/properties/CopyOnWrite.java
index 7d94438..947dbf4 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/task/properties/CopyOnWrite.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/task/properties/CopyOnWrite.java
@@ -69,7 +69,7 @@
}
protected Function<T, T> copier;
- protected StopWatch stopWatch = new StopWatch();
+ protected StopWatch.Runner stopWatch = StopWatch.Runner.DISABLED;
protected T original;
protected T copy;
@@ -79,7 +79,7 @@
}
protected void setNanosecondsConsumer(LongConsumer nanosConsumer) {
- stopWatch.setConsumer(nanosConsumer);
+ stopWatch = new StopWatch.Runner.Enabled().setNanosConsumer(nanosConsumer);
}
public T getOriginal() {
diff --git a/src/main/java/com/googlesource/gerrit/plugins/task/properties/Matcher.java b/src/main/java/com/googlesource/gerrit/plugins/task/properties/Matcher.java
index dc7bc18..932f984 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/task/properties/Matcher.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/task/properties/Matcher.java
@@ -41,8 +41,8 @@
protected int cursor;
protected Statistics statistics;
- protected StopWatch appendNanoseconds = new StopWatch();
- protected StopWatch findNanoseconds = new StopWatch();
+ protected StopWatch.Runner appendNanoseconds = StopWatch.Runner.DISABLED;
+ protected StopWatch.Runner findNanoseconds = StopWatch.Runner.DISABLED;
public Matcher(String text) {
this.text = text;
@@ -52,22 +52,25 @@
if (statisticsConsumer != null) {
statistics = new Statistics();
statisticsConsumer.accept(statistics);
- appendNanoseconds.setConsumer(ns -> statistics.appendNanoseconds = ns);
- findNanoseconds.setConsumer(ns -> statistics.findNanoseconds = ns);
+ appendNanoseconds =
+ new StopWatch.Runner.Enabled().setNanosConsumer(ns -> statistics.appendNanoseconds = ns);
+ findNanoseconds =
+ new StopWatch.Runner.Enabled().setNanosConsumer(ns -> statistics.findNanoseconds = ns);
}
}
public boolean find() {
- findNanoseconds.start();
+ return findNanoseconds.get(() -> findUntimed());
+ }
+
+ protected boolean findUntimed() {
start = text.indexOf("${", cursor);
nameStart = start + 2;
if (start < 0 || text.length() < nameStart + 1) {
- findNanoseconds.stop();
return false;
}
end = text.indexOf('}', nameStart);
boolean found = end >= 0;
- findNanoseconds.stop();
return found;
}
@@ -76,21 +79,25 @@
}
public void appendValue(StringBuffer buffer, String value) {
- appendNanoseconds.start();
+ appendNanoseconds.accept((b, v) -> appendValueUntimed(b, v), buffer, value);
+ }
+
+ protected void appendValueUntimed(StringBuffer buffer, String value) {
if (start > cursor) {
buffer.append(text.substring(cursor, start));
}
buffer.append(value);
cursor = end + 1;
- appendNanoseconds.stop();
}
public void appendTail(StringBuffer buffer) {
- appendNanoseconds.start();
+ appendNanoseconds.accept(b -> appendTailUntimed(b), buffer);
+ }
+
+ protected void appendTailUntimed(StringBuffer buffer) {
if (cursor < text.length()) {
buffer.append(text.substring(cursor));
cursor = text.length();
}
- appendNanoseconds.stop();
}
}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/task/properties/Properties.java b/src/main/java/com/googlesource/gerrit/plugins/task/properties/Properties.java
index 5fc6c12..cb2d46e 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/task/properties/Properties.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/task/properties/Properties.java
@@ -33,6 +33,12 @@
public long copierNanoseconds;
public Matcher.Statistics matcher;
+ public static void setNanoseconds(Statistics stats, long nanos) {
+ if (stats != null) {
+ stats.getTaskNanoseconds = nanos;
+ }
+ }
+
public Statistics sum(Statistics other) {
if (other == null) {
return this;
@@ -43,8 +49,6 @@
statistics.matcher = matcher == null ? other.matcher : matcher.sum(other.matcher);
return statistics;
}
-
- protected StopWatch getTask;
}
public static final Properties EMPTY =
@@ -80,32 +84,34 @@
}
/** Use to expand properties specifically for Tasks. */
+ @SuppressWarnings("try")
public Task getTask(ChangeData changeData) throws StorageException {
- if (statistics != null) {
- statistics.getTask = new StopWatch().enable().start();
- }
- loader = new Loader(origTask, changeData, getParentMapper());
- expander = new Expander(n -> loader.load(n));
- expander.setStatisticsConsumer(matcherStatisticsConsumer);
- if (isTaskRefreshRequired || init) {
- expander.expand(task, TaskConfig.KEY_APPLICABLE);
- isApplicableRefreshRequired = loader.isNonTaskDefinedPropertyLoaded();
+ try (StopWatch stopWatch =
+ StopWatch.builder()
+ .enabled(statistics != null)
+ .build()
+ .setNanosConsumer(l -> Statistics.setNanoseconds(statistics, l))) {
+ loader = new Loader(origTask, changeData, getParentMapper());
+ expander = new Expander(n -> loader.load(n));
+ expander.setStatisticsConsumer(matcherStatisticsConsumer);
+ if (isTaskRefreshRequired || init) {
+ expander.expand(task, TaskConfig.KEY_APPLICABLE);
+ isApplicableRefreshRequired = loader.isNonTaskDefinedPropertyLoaded();
- expander.expand(task, ImmutableSet.of(TaskConfig.KEY_APPLICABLE, TaskConfig.KEY_NAME));
+ expander.expand(task, ImmutableSet.of(TaskConfig.KEY_APPLICABLE, TaskConfig.KEY_NAME));
- Map<String, String> exported = expander.expand(origTask.exported);
- if (exported != origTask.exported) {
- task.getForWrite().exported = exported;
- }
+ Map<String, String> exported = expander.expand(origTask.exported);
+ if (exported != origTask.exported) {
+ task.getForWrite().exported = exported;
+ }
- if (init) {
- init = false;
- isTaskRefreshRequired = loader.isNonTaskDefinedPropertyLoaded();
+ if (init) {
+ init = false;
+ isTaskRefreshRequired = loader.isNonTaskDefinedPropertyLoaded();
+ }
}
}
if (statisticsConsumer != null) {
- statistics.getTaskNanoseconds = statistics.getTask.stop().get();
- statistics.getTask = null;
statisticsConsumer.accept(statistics);
}
return task.getForRead();
