From 970529e1a80850b30262917b4a1e134f1c2e5aee Mon Sep 17 00:00:00 2001 From: Scott Mitchell Date: Mon, 6 Apr 2015 18:42:18 -0700 Subject: [PATCH] HTTP/2 Priority tree circular link Motivation: If an exclusive dependency change stream B should be an exclusive dependency of stream A is requested and stream B is already a child of stream A...then we will add B to B's own children map and create a circular link in the priority tree. This leads to an infinite recursive loop and a stack overflow exception. Modifications: -when removeAllChildren is called it should not remove the exclusive dependency. -unit test to ensure this case is covered. Result: No more circular link in the priority tree. --- .../codec/http2/DefaultHttp2Connection.java | 98 ++++++++++---- .../http2/DefaultHttp2ConnectionTest.java | 124 ++++++++++++++++++ 2 files changed, 195 insertions(+), 27 deletions(-) diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2Connection.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2Connection.java index 7c2fe82864..6ddf71ee78 100644 --- a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2Connection.java +++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2Connection.java @@ -32,12 +32,12 @@ import static io.netty.handler.codec.http2.Http2Stream.State.OPEN; import static io.netty.handler.codec.http2.Http2Stream.State.RESERVED_LOCAL; import static io.netty.handler.codec.http2.Http2Stream.State.RESERVED_REMOTE; import static io.netty.util.internal.ObjectUtil.checkNotNull; - import io.netty.buffer.ByteBuf; import io.netty.util.collection.IntObjectHashMap; import io.netty.util.collection.IntObjectMap; import io.netty.util.collection.PrimitiveCollections; import io.netty.util.internal.PlatformDependent; +import io.netty.util.internal.SystemPropertyUtil; import io.netty.util.internal.logging.InternalLogger; import io.netty.util.internal.logging.InternalLoggerFactory; @@ -61,6 +61,15 @@ public class DefaultHttp2Connection implements Http2Connection { final ConnectionStream connectionStream = new ConnectionStream(); final DefaultEndpoint localEndpoint; final DefaultEndpoint remoteEndpoint; + /** + * The initial size of the children map is chosen to be conservative on initial memory allocations under + * the assumption that most streams will have a small number of children. This choice may be + * sub-optimal if when children are present there are many children (i.e. a web page which has many + * dependencies to load). + */ + private static final int INITIAL_CHILDREN_MAP_SIZE = + Math.max(1, SystemPropertyUtil.getInt("io.netty.http2.childrenMapSize", 4)); + /** * We chose a {@link List} over a {@link Set} to avoid allocating an {@link Iterator} objects when iterating over * the listeners. @@ -453,7 +462,7 @@ public class DefaultHttp2Connection implements Http2Connection { * Instead use {@link #incrementPrioritizableForTree(int, Http2Stream)}. */ private void incrementPrioritizableForTree0(int amt, Http2Stream oldParent) { - assert amt > 0; + assert amt > 0 && Integer.MAX_VALUE - amt >= prioritizableForTree; prioritizableForTree += amt; if (parent != null && parent != oldParent) { parent.incrementPrioritizableForTree0(amt, oldParent); @@ -475,7 +484,7 @@ public class DefaultHttp2Connection implements Http2Connection { * Direct calls to this method are discouraged. Instead use {@link #decrementPrioritizableForTree(int)}. */ private void decrementPrioritizableForTree0(int amt) { - assert amt > 0; + assert amt > 0 && prioritizableForTree >= amt; prioritizableForTree -= amt; if (parent != null) { parent.decrementPrioritizableForTree0(amt); @@ -501,10 +510,14 @@ public class DefaultHttp2Connection implements Http2Connection { private void initChildrenIfEmpty() { if (children == PrimitiveCollections.emptyIntObjectMap()) { - children = new IntObjectHashMap(4); + initChildren(); } } + private void initChildren() { + children = new IntObjectHashMap(INITIAL_CHILDREN_MAP_SIZE); + } + @Override public final boolean remoteSideOpen() { return state == HALF_CLOSED_LOCAL || state == OPEN || state == RESERVED_REMOTE; @@ -541,11 +554,26 @@ public class DefaultHttp2Connection implements Http2Connection { } } - final IntObjectMap removeAllChildren() { - totalChildWeights = 0; - prioritizableForTree = isPrioritizable() ? 1 : 0; + /** + * Remove all children with the exception of {@code streamToRetain}. + * This method is intended to be used to support an exclusive priority dependency operation. + * @return The map of children prior to this operation, excluding {@code streamToRetain} if present. + */ + private IntObjectMap retain(DefaultStream streamToRetain) { + streamToRetain = children.remove(streamToRetain.id()); IntObjectMap prevChildren = children; - children = PrimitiveCollections.emptyIntObjectMap(); + // This map should be re-initialized in anticipation for the 1 exclusive child which will be added. + // It will either be added directly in this method, or after this method is called...but it will be added. + initChildren(); + if (streamToRetain == null) { + totalChildWeights = 0; + prioritizableForTree = isPrioritizable() ? 1 : 0; + } else { + totalChildWeights = streamToRetain.weight(); + // prioritizableForTree does not change because it is assumed all children node will still be + // descendants through an exclusive priority tree operation. + children.put(streamToRetain.id(), streamToRetain); + } return prevChildren; } @@ -555,33 +583,44 @@ public class DefaultHttp2Connection implements Http2Connection { */ final void takeChild(DefaultStream child, boolean exclusive, List events) { DefaultStream oldParent = child.parent(); - events.add(new ParentChangedEvent(child, oldParent)); - notifyParentChanging(child, this); - child.parent = this; - if (exclusive && !children.isEmpty()) { - // If it was requested that this child be the exclusive dependency of this node, - // move any previous children to the child node, becoming grand children of this node. - for (DefaultStream grandchild : removeAllChildren().values()) { - child.takeChild(grandchild, false, events); + if (oldParent != this) { + events.add(new ParentChangedEvent(child, oldParent)); + notifyParentChanging(child, this); + child.parent = this; + // We need the removal operation to happen first so the prioritizableForTree for the old parent to root + // path is updated with the correct child.prioritizableForTree() value. Note that the removal operation + // may not be successful and may return null. This is because when an exclusive dependency is processed + // the children are removed in a previous recursive call but the child's parent link is updated here. + if (oldParent != null && oldParent.children.remove(child.id()) != null) { + oldParent.totalChildWeights -= child.weight(); + if (!child.isDescendantOf(oldParent)) { + oldParent.decrementPrioritizableForTree(child.prioritizableForTree()); + if (oldParent.prioritizableForTree() == 0) { + // There are a few risks with immediately removing nodes from the priority tree: + // 1. We are removing nodes while we are potentially shifting the tree. There are no + // concrete cases known but is risky because it could invalidate the data structure. + // 2. We are notifying listeners of the removal while the tree is in flux. Currently the + // codec listeners make no assumptions about priority tree structure when being notified. + removeStream(oldParent); + } + } } - } - // Lazily initialize the children to save object allocations. - initChildrenIfEmpty(); + // Lazily initialize the children to save object allocations. + initChildrenIfEmpty(); - if (children.put(child.id(), child) == null) { + final Http2Stream oldChild = children.put(child.id(), child); + assert oldChild == null : "A stream with the same stream ID was already in the child map."; totalChildWeights += child.weight(); incrementPrioritizableForTree(child.prioritizableForTree(), oldParent); } - if (oldParent != null && oldParent.children.remove(child.id()) != null) { - oldParent.totalChildWeights -= child.weight(); - if (!child.isDescendantOf(oldParent)) { - oldParent.decrementPrioritizableForTree(child.prioritizableForTree()); - if (oldParent.prioritizableForTree() == 0) { - removeStream(oldParent); - } + if (exclusive && !children.isEmpty()) { + // If it was requested that this child be the exclusive dependency of this node, + // move any previous children to the child node, becoming grand children of this node. + for (DefaultStream grandchild : retain(child).values()) { + child.takeChild(grandchild, false, events); } } } @@ -604,6 +643,11 @@ public class DefaultHttp2Connection implements Http2Connection { } if (prioritizableForTree() == 0) { + // There are a few risks with immediately removing nodes from the priority tree: + // 1. We are removing nodes while we are potentially shifting the tree. There are no + // concrete cases known but is risky because it could invalidate the data structure. + // 2. We are notifying listeners of the removal while the tree is in flux. Currently the + // codec listeners make no assumptions about priority tree structure when being notified. removeStream(this); } notifyParentChanged(events); diff --git a/codec-http2/src/test/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionTest.java b/codec-http2/src/test/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionTest.java index e145b8eb42..2569d5d5c1 100644 --- a/codec-http2/src/test/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionTest.java +++ b/codec-http2/src/test/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionTest.java @@ -362,6 +362,130 @@ public class DefaultHttp2ConnectionTest { assertEquals(p.numChildren() * DEFAULT_PRIORITY_WEIGHT, p.totalChildWeights()); } + @Test + public void existingChildMadeExclusiveShouldNotCreateTreeCycle() throws Http2Exception { + Http2Stream streamA = client.local().createStream(1).open(false); + Http2Stream streamB = client.local().createStream(3).open(false); + Http2Stream streamC = client.local().createStream(5).open(false); + Http2Stream streamD = client.local().createStream(7).open(false); + + streamB.setPriority(streamA.id(), DEFAULT_PRIORITY_WEIGHT, false); + streamC.setPriority(streamA.id(), DEFAULT_PRIORITY_WEIGHT, false); + streamD.setPriority(streamC.id(), DEFAULT_PRIORITY_WEIGHT, false); + + // Stream C is already dependent on Stream A, but now make that an exclusive dependency + streamC.setPriority(streamA.id(), DEFAULT_PRIORITY_WEIGHT, true); + + assertEquals(4, client.numActiveStreams()); + + // Level 0 + Http2Stream p = client.connectionStream(); + assertEquals(1, p.numChildren()); + assertEquals(5, p.prioritizableForTree()); + assertEquals(p.numChildren() * DEFAULT_PRIORITY_WEIGHT, p.totalChildWeights()); + + // Level 1 + p = child(p, streamA.id()); + assertNotNull(p); + assertEquals(0, p.parent().id()); + assertEquals(1, p.numChildren()); + assertEquals(4, p.prioritizableForTree()); + assertEquals(p.numChildren() * DEFAULT_PRIORITY_WEIGHT, p.totalChildWeights()); + + // Level 2 + p = child(p, streamC.id()); + assertNotNull(p); + assertEquals(streamA.id(), p.parent().id()); + assertEquals(2, p.numChildren()); + assertEquals(3, p.prioritizableForTree()); + assertEquals(p.numChildren() * DEFAULT_PRIORITY_WEIGHT, p.totalChildWeights()); + + // Level 3 + p = child(p, streamB.id()); + assertNotNull(p); + assertEquals(streamC.id(), p.parent().id()); + assertEquals(0, p.numChildren()); + assertEquals(1, p.prioritizableForTree()); + assertEquals(p.numChildren() * DEFAULT_PRIORITY_WEIGHT, p.totalChildWeights()); + p = child(p.parent(), streamD.id()); + assertNotNull(p); + assertEquals(streamC.id(), p.parent().id()); + assertEquals(0, p.numChildren()); + assertEquals(1, p.prioritizableForTree()); + assertEquals(p.numChildren() * DEFAULT_PRIORITY_WEIGHT, p.totalChildWeights()); + } + + @Test + public void newExclusiveChildShouldUpdateOldParentCorrectly() throws Http2Exception { + Http2Stream streamA = client.local().createStream(1).open(false); + Http2Stream streamB = client.local().createStream(3).open(false); + Http2Stream streamC = client.local().createStream(5).open(false); + Http2Stream streamD = client.local().createStream(7).open(false); + Http2Stream streamE = client.local().createStream(9).open(false); + Http2Stream streamF = client.local().createStream(11).open(false); + + streamB.setPriority(streamA.id(), DEFAULT_PRIORITY_WEIGHT, false); + streamC.setPriority(streamA.id(), DEFAULT_PRIORITY_WEIGHT, false); + streamD.setPriority(streamC.id(), DEFAULT_PRIORITY_WEIGHT, false); + streamF.setPriority(streamE.id(), DEFAULT_PRIORITY_WEIGHT, false); + + // F is now going to be exclusively dependent on A, after this we should check that stream E + // prioritizableForTree is not over decremented. + streamF.setPriority(streamA.id(), DEFAULT_PRIORITY_WEIGHT, true); + + assertEquals(6, client.numActiveStreams()); + + // Level 0 + Http2Stream p = client.connectionStream(); + assertEquals(2, p.numChildren()); + assertEquals(7, p.prioritizableForTree()); + assertEquals(p.numChildren() * DEFAULT_PRIORITY_WEIGHT, p.totalChildWeights()); + + // Level 1 + p = child(p, streamE.id()); + assertNotNull(p); + assertEquals(0, p.parent().id()); + assertEquals(0, p.numChildren()); + assertEquals(1, p.prioritizableForTree()); + assertEquals(p.numChildren() * DEFAULT_PRIORITY_WEIGHT, p.totalChildWeights()); + p = child(p.parent(), streamA.id()); + assertNotNull(p); + assertEquals(0, p.parent().id()); + assertEquals(1, p.numChildren()); + assertEquals(5, p.prioritizableForTree()); + assertEquals(p.numChildren() * DEFAULT_PRIORITY_WEIGHT, p.totalChildWeights()); + + // Level 2 + p = child(p, streamF.id()); + assertNotNull(p); + assertEquals(streamA.id(), p.parent().id()); + assertEquals(2, p.numChildren()); + assertEquals(4, p.prioritizableForTree()); + assertEquals(p.numChildren() * DEFAULT_PRIORITY_WEIGHT, p.totalChildWeights()); + + // Level 3 + p = child(p, streamB.id()); + assertNotNull(p); + assertEquals(streamF.id(), p.parent().id()); + assertEquals(0, p.numChildren()); + assertEquals(1, p.prioritizableForTree()); + assertEquals(p.numChildren() * DEFAULT_PRIORITY_WEIGHT, p.totalChildWeights()); + p = child(p.parent(), streamC.id()); + assertNotNull(p); + assertEquals(streamF.id(), p.parent().id()); + assertEquals(1, p.numChildren()); + assertEquals(2, p.prioritizableForTree()); + assertEquals(p.numChildren() * DEFAULT_PRIORITY_WEIGHT, p.totalChildWeights()); + + // Level 4 + p = child(p, streamD.id()); + assertNotNull(p); + assertEquals(streamC.id(), p.parent().id()); + assertEquals(0, p.numChildren()); + assertEquals(1, p.prioritizableForTree()); + assertEquals(p.numChildren() * DEFAULT_PRIORITY_WEIGHT, p.totalChildWeights()); + } + @Test public void weightChangeWithNoTreeChangeShouldNotifyListeners() throws Http2Exception { Http2Stream streamA = client.local().createStream(1).open(false);