opendev/gerrit
Code Issues Proposed changes

Assign unused Future return value to a variable

Browse Source 
Error Prone plans to make this pattern into a compile error; this avoids
future breakage.

In some of these cases, it's not obvious that ignoring the return value
is the correct thing to do; the hope is that the construct is ugly
enough to make the author think twice. In many cases though, it's clear
that a locally-created Runnable or Callable already does its own error
handling, so we can leave a comment alongside the ignored return value.

Eventually many of these should be audited again, and we can replace
some of the @SuppressWarnings with Error Prone's @CanIgnoreReturnValue.
That much is left for future cleanup.

Change-Id: Ia989214d85e0d6c387e388a77178e0b5c4bf2498
This commit is contained in:
Dave Borowitz
2017-02-02 15:49:50 -05:00
parent a00f42a52f
commit 9189e67c3d
19 changed files with 160 additions and 103 deletions
Download Patch File Download Diff File Expand all files Collapse all files

9
gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/GerritServer.java
View File

@@ -49,6 +49,7 @@ import java.util.concurrent.Callable;
import java.util.concurrent.CyclicBarrier;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.Future;
import java.util.concurrent.TimeUnit;
import org.apache.log4j.Level;
import org.apache.log4j.Logger;
@@ -169,6 +170,8 @@ public class GerritServer {
} else {
site = initSite(cfg);
daemonService = Executors.newSingleThreadExecutor();
@SuppressWarnings("unused")
Future<?> possiblyIgnoredError =
daemonService.submit(
new Callable<Void>() {
@Override
@@ -176,7 +179,11 @@ public class GerritServer {
int rc =
daemon.main(
new String[] {
"-d", site.getPath(), "--headless", "--console-log", "--show-stack-trace",
"-d",
site.getPath(),
"--headless",
"--console-log",
"--show-stack-trace",
});
if (rc != 0) {
System.err.println("Failed to start Gerrit daemon");

3
gerrit-cache-h2/src/main/java/com/google/gerrit/server/cache/h2/H2CacheFactory.java vendored
View File

@@ -39,6 +39,7 @@ import java.util.List;
import java.util.Map;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.Future;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.TimeUnit;
import org.eclipse.jgit.lib.Config;
@@ -116,6 +117,8 @@ class H2CacheFactory implements PersistentCacheFactory, LifecycleListener {
if (executor != null) {
for (final H2CacheImpl<?, ?> cache : caches) {
executor.execute(cache::start);
@SuppressWarnings("unused")
Future<?> possiblyIgnoredError =
cleanup.schedule(() -> cache.prune(cleanup), 30, TimeUnit.SECONDS);
}
}

3
gerrit-cache-h2/src/main/java/com/google/gerrit/server/cache/h2/H2CacheImpl.java vendored
View File

@@ -46,6 +46,7 @@ import java.util.concurrent.BlockingQueue;
import java.util.concurrent.Callable;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.Executor;
import java.util.concurrent.Future;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicLong;
@@ -196,6 +197,8 @@ public class H2CacheImpl<K, V> extends AbstractLoadingCache<K, V> implements Per
cal.add(Calendar.DAY_OF_MONTH, 1);
long delay = cal.getTimeInMillis() - TimeUtil.nowMs();
@SuppressWarnings("unused")
Future<?> possiblyIgnoredError =
service.schedule(() -> prune(service), delay, TimeUnit.MILLISECONDS);
}

6
gerrit-lucene/src/main/java/com/google/gerrit/lucene/AbstractLuceneIndex.java
View File

@@ -40,6 +40,7 @@ import java.util.concurrent.Callable;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.Executor;
import java.util.concurrent.Executors;
import java.util.concurrent.Future;
import java.util.concurrent.ScheduledThreadPoolExecutor;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.TimeoutException;
@@ -116,6 +117,8 @@ public abstract class AbstractLuceneIndex<K, V> implements Index<K, V> {
.setNameFormat(index + " Commit-%d")
.setDaemon(true)
.build());
@SuppressWarnings("unused") // Error handling within Runnable.
Future<?> possiblyIgnoredError =
autoCommitExecutor.scheduleAtFixedRate(
new Runnable() {
@Override
@@ -135,7 +138,8 @@ public abstract class AbstractLuceneIndex<K, V> implements Index<K, V> {
log.error(
"SEVERE: Error closing "
+ index
+ " Lucene index after OOM; index may be corrupted.",
+ " Lucene index after OOM;"
+ " index may be corrupted.",
e);
}
}

6
gerrit-pgm/src/main/java/com/google/gerrit/pgm/util/LogFileCompressor.java
View File

@@ -31,6 +31,7 @@ import java.nio.file.Files;
import java.nio.file.Path;
import java.time.LocalDate;
import java.time.ZoneId;
import java.util.concurrent.Future;
import java.util.zip.GZIPOutputStream;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -63,9 +64,12 @@ public class LogFileCompressor implements Runnable {
ZoneId zone = ZoneId.systemDefault();
LocalDate now = LocalDate.now(zone);
long milliSecondsUntil11pm = now.atStartOfDay(zone).plusHours(23).toInstant().toEpochMilli();
@SuppressWarnings("unused")
Future<?> possiblyIgnoredError =
queue
.getDefaultQueue()
.scheduleAtFixedRate(compressor, milliSecondsUntil11pm, HOURS.toMillis(24), MILLISECONDS);
.scheduleAtFixedRate(
compressor, milliSecondsUntil11pm, HOURS.toMillis(24), MILLISECONDS);
}
@Override

7
gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeCleanupRunner.java
View File

@@ -25,6 +25,7 @@ import com.google.gerrit.server.util.ManualRequestContext;
import com.google.gerrit.server.util.OneOffRequestContext;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import java.util.concurrent.Future;
import java.util.concurrent.TimeUnit;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -64,7 +65,11 @@ public class ChangeCleanupRunner implements Runnable {
String.format(
"Ignoring invalid changeCleanup schedule configuration: %s", scheduleConfig));
} else {
queue.getDefaultQueue().scheduleAtFixedRate(runner, delay, interval, TimeUnit.MILLISECONDS);
@SuppressWarnings("unused")
Future<?> possiblyIgnoredError =
queue
.getDefaultQueue()
.scheduleAtFixedRate(runner, delay, interval, TimeUnit.MILLISECONDS);
}
}

3
gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeInserter.java
View File

@@ -70,6 +70,7 @@ import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Future;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.transport.ReceiveCommand;
@@ -449,6 +450,8 @@ public class ChangeInserter extends BatchUpdate.InsertChangeOp {
}
};
if (requestScopePropagator != null) {
@SuppressWarnings("unused")
Future<?> possiblyIgnoredError =
sendEmailExecutor.submit(requestScopePropagator.wrap(sender));
} else {
sender.run();

4
gerrit-server/src/main/java/com/google/gerrit/server/change/EmailReviewComments.java
View File

@@ -42,6 +42,7 @@ import com.google.inject.ProvisionException;
import com.google.inject.assistedinject.Assisted;
import java.util.List;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Future;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -111,7 +112,8 @@ public class EmailReviewComments implements Runnable, RequestContext {
}
void sendAsync() {
sendEmailsExecutor.submit(this);
@SuppressWarnings("unused")
Future<?> possiblyIgnoredError = sendEmailsExecutor.submit(this);
}
@Override

4
gerrit-server/src/main/java/com/google/gerrit/server/git/EmailMerge.java
View File

@@ -35,6 +35,7 @@ import com.google.inject.Provider;
import com.google.inject.ProvisionException;
import com.google.inject.assistedinject.Assisted;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Future;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -89,7 +90,8 @@ public class EmailMerge implements Runnable, RequestContext {
}
public void sendAsync() {
sendEmailsExecutor.submit(this);
@SuppressWarnings("unused")
Future<?> possiblyIgnoredError = sendEmailsExecutor.submit(this);
}
@Override

3
gerrit-server/src/main/java/com/google/gerrit/server/git/GarbageCollectionRunner.java
View File

@@ -22,6 +22,7 @@ import com.google.gerrit.server.config.GcConfig;
import com.google.gerrit.server.config.ScheduleConfig;
import com.google.gerrit.server.project.ProjectCache;
import com.google.inject.Inject;
import java.util.concurrent.Future;
import java.util.concurrent.TimeUnit;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -52,6 +53,8 @@ public class GarbageCollectionRunner implements Runnable {
} else if (delay < 0 || interval <= 0) {
gcLog.warn(String.format("Ignoring invalid gc schedule configuration: %s", scheduleConfig));
} else {
@SuppressWarnings("unused")
Future<?> possiblyIgnoredError =
queue
.getDefaultQueue()
.scheduleAtFixedRate(gcRunner, delay, interval, TimeUnit.MILLISECONDS);

3
gerrit-server/src/main/java/com/google/gerrit/server/git/MergedByPushOp.java
View File

@@ -39,6 +39,7 @@ import com.google.inject.assistedinject.AssistedInject;
import java.io.IOException;
import java.util.Collections;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Future;
import org.eclipse.jgit.lib.Constants;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.Repository;
@@ -164,6 +165,8 @@ public class MergedByPushOp extends BatchUpdate.Op {
if (!correctBranch) {
return;
}
@SuppressWarnings("unused") // Runnable already handles errors
Future<?> possiblyIgnoredError =
sendEmailExecutor.submit(
requestScopePropagator.wrap(
new Runnable() {

6
gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
View File

@@ -138,6 +138,7 @@ import java.util.Map;
import java.util.Optional;
import java.util.Set;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.Future;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
import org.eclipse.jgit.errors.IncorrectObjectTypeException;
@@ -1959,8 +1960,9 @@ public class ReceiveCommits {
Change change = notes.getChange();
if (change.getDest().equals(magicBranch.dest)) {
logDebug("Found change {} from existing refs.", change.getKey());
// reindex the change asynchronously
indexer.indexAsync(project.getNameKey(), change.getId());
// Reindex the change asynchronously, ignoring errors.
@SuppressWarnings("unused")
Future<?> possiblyIgnoredError = indexer.indexAsync(project.getNameKey(), change.getId());
return true;
}
}

8
gerrit-server/src/main/java/com/google/gerrit/server/git/RenameGroupOp.java
View File

@@ -23,6 +23,7 @@ import com.google.inject.assistedinject.Assisted;
import java.io.IOException;
import java.util.ArrayList;
import java.util.List;
import java.util.concurrent.Future;
import java.util.concurrent.TimeUnit;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.errors.RepositoryNotFoundException;
@@ -92,11 +93,12 @@ public class RenameGroupOp extends DefaultQueueOp {
}
}
// If one or more projects did not update, wait 5 minutes
// and give it another attempt.
// If one or more projects did not update, wait 5 minutes and give it
// another attempt. If it doesn't update after that, give up.
if (!retryOn.isEmpty() && !tryingAgain) {
tryingAgain = true;
start(5, TimeUnit.MINUTES);
@SuppressWarnings("unused")
Future<?> possiblyIgnoredError = start(5, TimeUnit.MINUTES);
}
}

3
gerrit-server/src/main/java/com/google/gerrit/server/git/ReplaceOp.java
View File

@@ -61,6 +61,7 @@ import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Future;
import org.eclipse.jgit.lib.Constants;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.Ref;
@@ -412,6 +413,8 @@ public class ReplaceOp extends BatchUpdate.Op {
};
if (requestScopePropagator != null) {
@SuppressWarnings("unused")
Future<?> possiblyIgnoredError =
sendEmailExecutor.submit(requestScopePropagator.wrap(sender));
} else {
sender.run();

3
gerrit-server/src/main/java/com/google/gerrit/server/group/PutName.java
View File

@@ -39,6 +39,7 @@ import java.io.IOException;
import java.util.Collections;
import java.util.Date;
import java.util.TimeZone;
import java.util.concurrent.Future;
import java.util.concurrent.TimeUnit;
@Singleton
@@ -125,6 +126,8 @@ public class PutName implements RestModifyView<GroupResource, Input> {
groupCache.evict(group);
groupCache.evictAfterRename(old, key);
@SuppressWarnings("unused")
Future<?> possiblyIgnoredError =
renameGroupOpFactory
.create(
currentUser.get().newCommitterIdent(new Date(), TimeZone.getDefault()),

5
gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeIndexer.java
View File

@@ -52,6 +52,7 @@ import java.util.Collections;
import java.util.List;
import java.util.concurrent.Callable;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.Future;
import java.util.concurrent.atomic.AtomicReference;
import org.eclipse.jgit.lib.Config;
import org.slf4j.Logger;
@@ -313,7 +314,9 @@ public class ChangeIndexer {
private void reindexAfterIndexUpdate(Project.NameKey project, Change.Id id) {
if (reindexAfterIndexUpdate) {
reindexIfStale(project, id);
// Don't retry indefinitely; if this fails the change will be stale.
@SuppressWarnings("unused")
Future<?> possiblyIgnoredError = reindexIfStale(project, id);
}
}

5
gerrit-server/src/main/java/com/google/gerrit/server/index/change/ReindexAfterUpdate.java
View File

@@ -39,6 +39,7 @@ import com.google.inject.Provider;
import java.io.IOException;
import java.util.List;
import java.util.concurrent.Callable;
import java.util.concurrent.Future;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -81,7 +82,9 @@ public class ReindexAfterUpdate implements GitReferenceUpdatedListener {
@Override
public void onSuccess(List<Change> changes) {
for (Change c : changes) {
executor.submit(new Index(event, c.getId()));
// Don't retry indefinitely; if this fails changes may be stale.
@SuppressWarnings("unused")
Future<?> possiblyIgnoredError = executor.submit(new Index(event, c.getId()));
}
}

4
gerrit-server/src/main/java/com/google/gerrit/server/mail/receive/MailReceiver.java
View File

@@ -28,6 +28,7 @@ import java.util.Set;
import java.util.Timer;
import java.util.TimerTask;
import java.util.concurrent.Callable;
import java.util.concurrent.Future;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -133,7 +134,8 @@ public abstract class MailReceiver implements LifecycleListener {
}
return null;
};
workQueue.getDefaultQueue().submit(task);
@SuppressWarnings("unused")
Future<?> possiblyIgnoredError = workQueue.getDefaultQueue().submit(task);
} else {
// Synchronous processing is used only in tests.
try {

12
gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectCacheClock.java
View File

@@ -20,6 +20,7 @@ import com.google.gerrit.server.config.GerritServerConfig;
import com.google.inject.Inject;
import com.google.inject.Singleton;
import java.util.concurrent.Executors;
import java.util.concurrent.Future;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.TimeUnit;
import org.eclipse.jgit.lib.Config;
@@ -50,14 +51,13 @@ public class ProjectCacheClock {
.setDaemon(true)
.setPriority(Thread.MIN_PRIORITY)
.build());
@SuppressWarnings("unused") // Runnable already handles errors
Future<?> possiblyIgnoredError =
executor.scheduleAtFixedRate(
new Runnable() {
@Override
public void run() {
// This is not exactly thread-safe, but is OK for our use.
// The only thread that writes the volatile is this task.
() -> {
// This is not exactly thread-safe, but is OK for our use. The only
// thread that writes the volatile is this task.
generation = generation + 1;
}
},
checkFrequencyMillis,
checkFrequencyMillis,