ReceivePack: clear advertised .haves if application changes refs
An application can choose to invoke setAdvertisedRefs multiple times, for example several AdvertiseRefsHook installed in a chain. Each of these invocations populates the advertisedHaves collection with the unique set of ObjectIds. This can lead to a server over-advertising with ".have" lines if the first hook pushes in a lot of references, and the second hook filters this to a subset. ReceivePack will advertise the unique objects from the first hook using ".have" lines, which may lead to a huge advertisement sent to the client. This can also contribute to a very slow connectivity check after the pack is parsed as ReceivePack calls markUninteresting on every commit in advertisedHaves. This may require expanding a lot of subtrees to mark all trees as uninteresting as well. On a very big repository this can lead to a many-second stall. Clear the advertisedHaves collection any time the refs are updated. Add a test to verify the correct set of objects was sent. Change-Id: I97f6998d0597251444a2e846a3ea1f461bae96f9
This commit is contained in:
parent
d0d15c3848
commit
e68a9b3ed8
|
@ -58,6 +58,8 @@
|
||||||
import java.util.Collections;
|
import java.util.Collections;
|
||||||
import java.util.HashMap;
|
import java.util.HashMap;
|
||||||
import java.util.Map;
|
import java.util.Map;
|
||||||
|
import java.util.Set;
|
||||||
|
import java.util.concurrent.atomic.AtomicReference;
|
||||||
import java.util.zip.Deflater;
|
import java.util.zip.Deflater;
|
||||||
|
|
||||||
import org.eclipse.jgit.errors.MissingObjectException;
|
import org.eclipse.jgit.errors.MissingObjectException;
|
||||||
|
@ -158,6 +160,45 @@ ReceivePack createReceivePack(final Repository db) {
|
||||||
assertEquals(B, master.getObjectId());
|
assertEquals(B, master.getObjectId());
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
public void resetsHaves() throws Exception {
|
||||||
|
AtomicReference<Set<ObjectId>> haves = new AtomicReference<>();
|
||||||
|
try (TransportLocal t = new TransportLocal(src, uriOf(dst),
|
||||||
|
dst.getDirectory()) {
|
||||||
|
@Override
|
||||||
|
ReceivePack createReceivePack(Repository db) {
|
||||||
|
dst.incrementOpen();
|
||||||
|
|
||||||
|
ReceivePack rp = super.createReceivePack(dst);
|
||||||
|
rp.setAdvertiseRefsHook(new AdvertiseRefsHook() {
|
||||||
|
@Override
|
||||||
|
public void advertiseRefs(BaseReceivePack rp2)
|
||||||
|
throws ServiceMayNotContinueException {
|
||||||
|
rp.setAdvertisedRefs(rp.getRepository().getAllRefs(),
|
||||||
|
null);
|
||||||
|
new HidePrivateHook().advertiseRefs(rp);
|
||||||
|
haves.set(rp.getAdvertisedObjects());
|
||||||
|
}
|
||||||
|
|
||||||
|
@Override
|
||||||
|
public void advertiseRefs(UploadPack uploadPack)
|
||||||
|
throws ServiceMayNotContinueException {
|
||||||
|
throw new UnsupportedOperationException();
|
||||||
|
}
|
||||||
|
});
|
||||||
|
return rp;
|
||||||
|
}
|
||||||
|
}) {
|
||||||
|
try (PushConnection c = t.openPush()) {
|
||||||
|
// Just has to open/close for advertisement.
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
assertEquals(1, haves.get().size());
|
||||||
|
assertTrue(haves.get().contains(B));
|
||||||
|
assertFalse(haves.get().contains(P));
|
||||||
|
}
|
||||||
|
|
||||||
@Test
|
@Test
|
||||||
public void testSuccess() throws Exception {
|
public void testSuccess() throws Exception {
|
||||||
// Manually force a delta of an object so we reuse it later.
|
// Manually force a delta of an object so we reuse it later.
|
||||||
|
|
|
@ -447,6 +447,7 @@ public final Map<String, Ref> getAdvertisedRefs() {
|
||||||
public void setAdvertisedRefs(Map<String, Ref> allRefs, Set<ObjectId> additionalHaves) {
|
public void setAdvertisedRefs(Map<String, Ref> allRefs, Set<ObjectId> additionalHaves) {
|
||||||
refs = allRefs != null ? allRefs : db.getAllRefs();
|
refs = allRefs != null ? allRefs : db.getAllRefs();
|
||||||
refs = refFilter.filter(refs);
|
refs = refFilter.filter(refs);
|
||||||
|
advertisedHaves.clear();
|
||||||
|
|
||||||
Ref head = refs.get(Constants.HEAD);
|
Ref head = refs.get(Constants.HEAD);
|
||||||
if (head != null && head.isSymbolic())
|
if (head != null && head.isSymbolic())
|
||||||
|
|
Loading…
Reference in New Issue