opendev/gerrit
Code Issues Proposed changes

Don't leak ObjectReaders created from ObjectInserters

Browse Source 
We had been pretty lazy about calling new RevWalk(inserter.newReader()).
I can't say with certainty for any individual case, but there are
at least two possible loose justifications for this:

1. ObjectInserter#newReader() feels like it shouldn't allocate any new
   resources (other than GCable buffers).
2. It seems like the RevWalk might close the reader.

Both of these are bad arguments. For (1), WindowCursor acquires an
inflater from the inflater cache, so failing to release the inflater
results in more allocations. For (2), RevWalk does not close the reader
when using this constructor. While perhaps non-obvious, this behavior is
clearly documented and stable.

Change-Id: I712973ecfcca439245722c777abde129dbe3773d
This commit is contained in:
Dave Borowitz
2017-04-03 11:51:00 -04:00
parent 178b7174cc
commit d3239cced2
8 changed files with 38 additions and 19 deletions
Download Patch File Download Diff File Expand all files Collapse all files

1
gerrit-server/src/main/java/com/google/gerrit/server/update/BatchUpdate.java
View File

@@ -189,6 +189,7 @@ public abstract class BatchUpdate implements AutoCloseable {
@Override
public void close() {
if (closeRepo) {
revWalk.getObjectReader().close();
revWalk.close();
inserter.close();
repo.close();

19
gerrit-server/src/main/java/com/google/gerrit/server/update/ReviewDbBatchUpdate.java
View File

@@ -85,6 +85,7 @@ import org.eclipse.jgit.lib.BatchRefUpdate;
import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.NullProgressMonitor;
import org.eclipse.jgit.lib.ObjectInserter;
import org.eclipse.jgit.lib.ObjectReader;
import org.eclipse.jgit.lib.PersonIdent;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevWalk;
@@ -440,16 +441,14 @@ class ReviewDbBatchUpdate extends BatchUpdate {
}
if (onSubmitValidators != null && commands != null && !commands.isEmpty()) {
// Validation of refs has to take place here and not at the beginning
// executeRefUpdates. Otherwise failing validation in a second
// BatchUpdate object will happen *after* first object's
// executeRefUpdates has finished, hence after first repo's refs have
// been updated, which is too late.
onSubmitValidators.validate(
project,
new ReadOnlyRepository(getRepository()),
ctx.getInserter().newReader(),
commands.getCommands());
try (ObjectReader reader = ctx.getInserter().newReader()) {
// Validation of refs has to take place here and not at the beginning
// executeRefUpdates. Otherwise failing validation in a second BatchUpdate object will
// happen *after* first object's executeRefUpdates has finished, hence after first repo's
// refs have been updated, which is too late.
onSubmitValidators.validate(
project, new ReadOnlyRepository(getRepository()), reader, commands.getCommands());
}
}
if (inserter != null) {