andreacavalli/netty5
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

HTTP/2 Priority tree circular link

Browse Source 
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.
This commit is contained in:
Scott Mitchell 2015-04-06 18:42:18 -07:00
parent 485c17d911
commit d17fcd7805
2 changed files with 195 additions and 27 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

86
codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2Connection.java
View File

@ -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<Http2LocalFlowController> localEndpoint;
final DefaultEndpoint<Http2RemoteFlowController> 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.<DefaultStream>emptyIntObjectMap()) {
children = new IntObjectHashMap<DefaultStream>(4);
initChildren();
}
}
private void initChildren() {
children = new IntObjectHashMap<DefaultStream>(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<DefaultStream> removeAllChildren() {
/**
* 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<DefaultStream> retain(DefaultStream streamToRetain) {
streamToRetain = children.remove(streamToRetain.id());
IntObjectMap<DefaultStream> prevChildren = children;
// 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;
IntObjectMap<DefaultStream> prevChildren = children;
children = PrimitiveCollections.emptyIntObjectMap();
} 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<ParentChangedEvent> events) {
DefaultStream oldParent = child.parent();
if (oldParent != this) {
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);
// 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();
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);

124
codec-http2/src/test/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionTest.java
View File

@ -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);