opendev/gerrit
Code Issues Proposed changes

AbstractChangeUpdate: Don't prematurely close Repository

Browse Source 
Using MetaDataUpdate in a try-with-resources block closes the repo as
soon as the BatchMetaDataUpdate is opened, which is not what we want.
In the traditional RepositoryCache case this doesn't hurt, since we
maintain a nonzero refcount for open repos so the close is a no-op,
but this is not always the case.

Instead, don't close the MetaDataUpdate implicitly, but rather
explicitly close it from the close method of the enclosing
BatchMetaDataUpdate.

Change-Id: I40871efd38d11375367a2909cf8eff4443e68f20
This commit is contained in:
Dave Borowitz
2015-12-28 13:58:03 -08:00
parent 1009c6bfb1
commit 7eba4815d2
2 changed files with 9 additions and 8 deletions
Download Patch File Download Diff File Expand all files Collapse all files

8
gerrit-server/src/main/java/com/google/gerrit/server/notedb/AbstractChangeUpdate.java
View File

@@ -117,14 +117,12 @@ public abstract class AbstractChangeUpdate extends VersionedMetaData {
throws IOException { throws IOException {
if (migration.writeChanges()) { if (migration.writeChanges()) {
load(); load();
try (MetaDataUpdate md = Project.NameKey p = getProjectName();
updateFactory.create(getProjectName(), MetaDataUpdate md = updateFactory.create(
repoManager.openMetadataRepository(getProjectName()), getUser(), p, repoManager.openMetadataRepository(p), getUser(), bru);
bru)) {
md.setAllowEmpty(true); md.setAllowEmpty(true);
return super.openUpdate(md); return super.openUpdate(md);
} }
}
return new BatchMetaDataUpdate() { return new BatchMetaDataUpdate() {
@Override @Override
public void write(CommitBuilder commit) { public void write(CommitBuilder commit) {

5
gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java
View File

@@ -408,7 +408,6 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
@Test @Test
public void topicChangeNotes() throws Exception { public void topicChangeNotes() throws Exception {
Change c = newChange(); Change c = newChange();
ChangeUpdate update = newUpdate(c, changeOwner);
// initially topic is not set // initially topic is not set
ChangeNotes notes = newNotes(c); ChangeNotes notes = newNotes(c);
@@ -417,12 +416,14 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
// set topic // set topic
String topic = "myTopic"; String topic = "myTopic";
ChangeUpdate update = newUpdate(c, changeOwner);
update.setTopic(topic); update.setTopic(topic);
update.commit(); update.commit();
notes = newNotes(c); notes = newNotes(c);
assertThat(notes.getChange().getTopic()).isEqualTo(topic); assertThat(notes.getChange().getTopic()).isEqualTo(topic);
// clear topic by setting empty string // clear topic by setting empty string
update = newUpdate(c, changeOwner);
update.setTopic(""); update.setTopic("");
update.commit(); update.commit();
notes = newNotes(c); notes = newNotes(c);
@@ -430,12 +431,14 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
// set other topic // set other topic
topic = "otherTopic"; topic = "otherTopic";
update = newUpdate(c, changeOwner);
update.setTopic(topic); update.setTopic(topic);
update.commit(); update.commit();
notes = newNotes(c); notes = newNotes(c);
assertThat(notes.getChange().getTopic()).isEqualTo(topic); assertThat(notes.getChange().getTopic()).isEqualTo(topic);
// clear topic by setting null // clear topic by setting null
update = newUpdate(c, changeOwner);
update.setTopic(null); update.setTopic(null);
update.commit(); update.commit();
notes = newNotes(c); notes = newNotes(c);