MultiProgressMonitor: Allow to configure the cancellation timeout
The cancellation timeout defines how much time the receive task has to
finish gracefully after the receive timeout is exceeded and before the
task is cancelled forcefully.
So far we hard-coded this to 10 * maxIntervalNanos (= 5s).
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: I901c9dd5772e333ff71e526b11cf6e045d03050e
diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt
index 0cd4d62..a0d4b15 100644
--- a/Documentation/config-gerrit.txt
+++ b/Documentation/config-gerrit.txt
@@ -4294,12 +4294,29 @@
be specified using standard time unit abbreviations ('ms', 'sec',
'min', etc.).
+
+After the timeout is exceeded the task processing the receive gets a
+cancellation signal that allows the tast to finish gracefully.
+link:#receive.cancellationTimeout[receive.cancellationTimeout]
+defines how much time the task has to react to the cancellation signal
+before it is focefully cancelled.
++
The receive timeout cannot be overriden by setting a higher
link:user-upload#deadline[deadline] on the git push request.
+
Default is 4 minutes. If no unit is specified, milliseconds
is assumed.
+[[receive.cancellationTimeout]]receive.cancellationTimeout::
++
+Defines the time that a receive task has to react to a cancellation
+signal and finish gracefully after link:#receive.timeout[receive.timeout]
+is exceeded. If the receive task is still not terminated after the
+cancellation timeout is exceeded the task is forcefully cancelled.
+Values can be specified using standard time unit abbreviations ('ms',
+'sec', 'min', etc.).
++
+Default is 5 seconds. If no unit is specified, milliseconds is assumed.
+
[[receive.trustedKey]]receive.trustedKey::
+
List of GPG key fingerprints that should be considered trust roots by
diff --git a/java/com/google/gerrit/server/git/MultiProgressMonitor.java b/java/com/google/gerrit/server/git/MultiProgressMonitor.java
index e55f70b..22385c7 100644
--- a/java/com/google/gerrit/server/git/MultiProgressMonitor.java
+++ b/java/com/google/gerrit/server/git/MultiProgressMonitor.java
@@ -196,11 +196,16 @@
/**
* Wait for a task managed by a {@link Future}, with no timeout.
*
- * @see #waitFor(Future, long, TimeUnit)
+ * @see #waitFor(Future, long, TimeUnit, long, TimeUnit)
*/
public <T> T waitFor(Future<T> workerFuture) {
try {
- return waitFor(workerFuture, 0, null);
+ return waitFor(
+ workerFuture,
+ /* taskTimeoutTime= */ 0,
+ /* taskTimeoutUnit= */ null,
+ /* cancellationTimeoutTime= */ 0,
+ /* cancellationTimeoutUnit= */ null);
} catch (TimeoutException e) {
throw new IllegalStateException("timout exception without setting a timeout", e);
}
@@ -214,18 +219,31 @@
* forcefully cancelled and {@link ExecutionException} is thrown.
*
* @param workerFuture a future that returns when worker threads are finished.
- * @param timeoutTime overall timeout for the task; the future is forcefully cancelled if the task
- * exceeds the timeout. Non-positive values indicate no timeout.
- * @param timeoutUnit unit for overall task timeout.
+ * @param taskTimeoutTime overall timeout for the task; the future gets a cancellation signal
+ * after this timeout is exceeded; non-positive values indicate no timeout.
+ * @param taskTimeoutUnit unit for overall task timeout.
+ * @param cancellationTimeoutTime timeout for the task to react to the cancellation signal; if the
+ * task doesn't terminate within this time it is forcefully cancelled; non-positive values
+ * indicate no timeout.
+ * @param cancellationTimeoutUnit unit for the cancellation timeout.
* @throws TimeoutException if this thread or a worker thread was interrupted, the worker was
* cancelled, or timed out waiting for a worker to call {@link #end()}.
*/
- public <T> T waitFor(Future<T> workerFuture, long timeoutTime, TimeUnit timeoutUnit)
+ public <T> T waitFor(
+ Future<T> workerFuture,
+ long taskTimeoutTime,
+ TimeUnit taskTimeoutUnit,
+ long cancellationTimeoutTime,
+ TimeUnit cancellationTimeoutUnit)
throws TimeoutException {
long overallStart = System.nanoTime();
+ long cancellationNanos =
+ cancellationTimeoutTime > 0
+ ? NANOSECONDS.convert(cancellationTimeoutTime, cancellationTimeoutUnit)
+ : 0;
long deadline;
- if (timeoutTime > 0) {
- timeout = Optional.of(NANOSECONDS.convert(timeoutTime, timeoutUnit));
+ if (taskTimeoutTime > 0) {
+ timeout = Optional.of(NANOSECONDS.convert(taskTimeoutTime, taskTimeoutUnit));
deadline = overallStart + timeout.get();
} else {
deadline = 0;
@@ -252,12 +270,9 @@
MILLISECONDS.convert(now - deadline, NANOSECONDS));
deadlineExceeded = true;
- // After setting deadlineExceeded = true give the worker 10 x maxIntervalNanos to react
- // to the cancellation and return gracefully.
- // The factor of 10 is a magic number that is a best guess for long it takes
- // ReceiveCommits to react to a cancellation. We likely need to tweak this in follow-up
- // changes or make it configurable.
- if (now > deadline + 10 * maxIntervalNanos) {
+ // After setting deadlineExceeded = true give the cancellationNanos to react to the
+ // cancellation and return gracefully.
+ if (now > deadline + cancellationNanos) {
// The worker didn't react to the cancellation, cancel it forcefully by an interrupt.
workerFuture.cancel(true);
if (workerFuture.isCancelled()) {
diff --git a/java/com/google/gerrit/server/git/receive/AsyncReceiveCommits.java b/java/com/google/gerrit/server/git/receive/AsyncReceiveCommits.java
index 148f34a..5b26f61 100644
--- a/java/com/google/gerrit/server/git/receive/AsyncReceiveCommits.java
+++ b/java/com/google/gerrit/server/git/receive/AsyncReceiveCommits.java
@@ -93,7 +93,9 @@
public class AsyncReceiveCommits {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
- private static final String TIMEOUT_NAME = "ReceiveCommitsOverallTimeout";
+ private static final String RECEIVE_OVERALL_TIMEOUT_NAME = "ReceiveCommitsOverallTimeout";
+ private static final String RECEIVE_CANCELLATION_TIMEOUT_NAME =
+ "ReceiveCommitsCancellationTimeout";
public interface Factory {
AsyncReceiveCommits create(
@@ -118,11 +120,24 @@
@Provides
@Singleton
- @Named(TIMEOUT_NAME)
- long getTimeoutMillis(@GerritServerConfig Config cfg) {
+ @Named(RECEIVE_OVERALL_TIMEOUT_NAME)
+ long getReceiveTimeoutMillis(@GerritServerConfig Config cfg) {
return ConfigUtil.getTimeUnit(
cfg, "receive", null, "timeout", TimeUnit.MINUTES.toMillis(4), TimeUnit.MILLISECONDS);
}
+
+ @Provides
+ @Singleton
+ @Named(RECEIVE_CANCELLATION_TIMEOUT_NAME)
+ long getCancellationTimeoutMillis(@GerritServerConfig Config cfg) {
+ return ConfigUtil.getTimeUnit(
+ cfg,
+ "receive",
+ null,
+ "cancellationTimeout",
+ TimeUnit.SECONDS.toMillis(5),
+ TimeUnit.MILLISECONDS);
+ }
}
private static MultiProgressMonitor newMultiProgressMonitor(
@@ -216,7 +231,8 @@
private final RequestScopePropagator scopePropagator;
private final ReceiveConfig receiveConfig;
private final ContributorAgreementsChecker contributorAgreements;
- private final long timeoutMillis;
+ private final long receiveTimeoutMillis;
+ private final long cancellationTimeoutMillis;
private final ProjectState projectState;
private final IdentifiedUser user;
private final Repository repo;
@@ -238,7 +254,8 @@
QuotaBackend quotaBackend,
UsersSelfAdvertiseRefsHook usersSelfAdvertiseRefsHook,
AllUsersName allUsersName,
- @Named(TIMEOUT_NAME) long timeoutMillis,
+ @Named(RECEIVE_OVERALL_TIMEOUT_NAME) long receiveTimeoutMillis,
+ @Named(RECEIVE_CANCELLATION_TIMEOUT_NAME) long cancellationTimeoutMillis,
@Assisted ProjectState projectState,
@Assisted IdentifiedUser user,
@Assisted Repository repo,
@@ -249,7 +266,8 @@
this.scopePropagator = scopePropagator;
this.receiveConfig = receiveConfig;
this.contributorAgreements = contributorAgreements;
- this.timeoutMillis = timeoutMillis;
+ this.receiveTimeoutMillis = receiveTimeoutMillis;
+ this.cancellationTimeoutMillis = cancellationTimeoutMillis;
this.projectState = projectState;
this.user = user;
this.repo = repo;
@@ -386,7 +404,11 @@
ProjectRunnable.fromCallable(
callable, receiveCommits.getProject().getNameKey(), "receive-commits", null, false);
monitor.waitFor(
- executor.submit(scopePropagator.wrap(runnable)), timeoutMillis, TimeUnit.MILLISECONDS);
+ executor.submit(scopePropagator.wrap(runnable)),
+ receiveTimeoutMillis,
+ TimeUnit.MILLISECONDS,
+ cancellationTimeoutMillis,
+ TimeUnit.MILLISECONDS);
if (!runnable.isDone()) {
// At this point we are either done or have thrown a TimeoutException and bailed out.
throw new IllegalStateException("unable to get receive commits result");
