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 cd507c953d..a10bc3f0cd 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 @@ -81,7 +81,6 @@ public class DefaultHttp2Connection implements Http2Connection { * the policy to be used for removal of closed stream. */ public DefaultHttp2Connection(boolean server, Http2StreamRemovalPolicy removalPolicy) { - this.removalPolicy = checkNotNull(removalPolicy, "removalPolicy"); localEndpoint = new DefaultEndpoint(server); remoteEndpoint = new DefaultEndpoint(!server); @@ -184,15 +183,25 @@ public class DefaultHttp2Connection implements Http2Connection { } } - private void removeStream(DefaultStream stream) { - // Notify the listeners of the event first. - for (Listener listener : listeners) { - listener.onStreamRemoved(stream); - } + /** + * Closed streams may stay in the priority tree if they have dependents that are in prioritizable states. + * When a stream is requested to be removed we can only actually remove that stream when there are no more + * prioritizable children. + * (see [1] {@link Http2Stream#prioritizableForTree()} and [2] {@link DefaultStream#removeChild(DefaultStream)}). + * When a priority tree edge changes we also have to re-evaluate viable nodes + * (see [3] {@link DefaultStream#takeChild(DefaultStream, boolean, List)}). + * @param stream The stream to remove. + */ + void removeStream(DefaultStream stream) { + // [1] Check if this stream can be removed because it has no prioritizable descendants. + if (stream.parent().removeChild(stream)) { + // Remove it from the map and priority tree. + streamMap.remove(stream.id()); - // Remove it from the map and priority tree. - streamMap.remove(stream.id()); - stream.parent().removeChild(stream); + for (Listener listener : listeners) { + listener.onStreamRemoved(stream); + } + } } /** @@ -205,6 +214,7 @@ public class DefaultHttp2Connection implements Http2Connection { private DefaultStream parent; private IntObjectMap children = newChildMap(); private int totalChildWeights; + private int prioritizableForTree = 1; private boolean resetSent; private PropertyMap data; @@ -235,17 +245,17 @@ public class DefaultHttp2Connection implements Http2Connection { } @Override - public Object setProperty(Object key, Object value) { + public final Object setProperty(Object key, Object value) { return data.put(key, value); } @Override - public V getProperty(Object key) { + public final V getProperty(Object key) { return data.get(key); } @Override - public V removeProperty(Object key) { + public final V removeProperty(Object key) { return data.remove(key); } @@ -269,6 +279,11 @@ public class DefaultHttp2Connection implements Http2Connection { return parent; } + @Override + public final int prioritizableForTree() { + return prioritizableForTree; + } + @Override public final boolean isDescendantOf(Http2Stream stream) { Http2Stream next = parent(); @@ -325,10 +340,10 @@ public class DefaultHttp2Connection implements Http2Connection { // Already have a priority. Re-prioritize the stream. weight(weight); - if (newParent != parent() || exclusive) { - List events; + if (newParent != parent() || (exclusive && newParent.numChildren() != 1)) { + final List events; if (newParent.isDescendantOf(this)) { - events = new ArrayList(2 + (exclusive ? newParent.numChildren(): 0)); + events = new ArrayList(2 + (exclusive ? newParent.numChildren() : 0)); parent.takeChild(newParent, false, events); } else { events = new ArrayList(1 + (exclusive ? newParent.numChildren() : 0)); @@ -375,6 +390,7 @@ public class DefaultHttp2Connection implements Http2Connection { } state = CLOSED; + decrementPrioritizableForTree(1); if (activeStreams.remove(this)) { try { // Update the number of active streams initiated by the endpoint. @@ -424,6 +440,59 @@ public class DefaultHttp2Connection implements Http2Connection { return this; } + /** + * Recursively increment the {@link #prioritizableForTree} for this object up the parent links until + * either we go past the root or {@code oldParent} is encountered. + * @param amt The amount to increment by. This must be positive. + * @param oldParent The previous parent for this stream. + */ + private void incrementPrioritizableForTree(int amt, Http2Stream oldParent) { + if (amt != 0) { + incrementPrioritizableForTree0(amt, oldParent); + } + } + + /** + * Direct calls to this method are discouraged. + * Instead use {@link #incrementPrioritizableForTree(int, Http2Stream)}. + */ + private void incrementPrioritizableForTree0(int amt, Http2Stream oldParent) { + assert amt > 0; + prioritizableForTree += amt; + if (parent != null && parent != oldParent) { + parent.incrementPrioritizableForTree(amt, oldParent); + } + } + + /** + * Recursively increment the {@link #prioritizableForTree} for this object up the parent links until + * either we go past the root. + * @param amt The amount to decrement by. This must be positive. + */ + private void decrementPrioritizableForTree(int amt) { + if (amt != 0) { + decrementPrioritizableForTree0(amt); + } + } + + /** + * Direct calls to this method are discouraged. Instead use {@link #decrementPrioritizableForTree(int)}. + */ + private void decrementPrioritizableForTree0(int amt) { + assert amt > 0; + prioritizableForTree -= amt; + if (parent != null) { + parent.decrementPrioritizableForTree(amt); + } + } + + /** + * Determine if this node by itself is considered to be valid in the priority tree. + */ + private boolean isPrioritizable() { + return state != CLOSED; + } + private void notifyHalfClosed(Http2Stream stream) { for (Listener listener : listeners) { listener.onStreamHalfClosed(stream); @@ -464,6 +533,7 @@ public class DefaultHttp2Connection implements Http2Connection { final IntObjectMap removeAllChildren() { totalChildWeights = 0; + prioritizableForTree = isPrioritizable() ? 1 : 0; IntObjectMap prevChildren = children; children = newChildMap(); return prevChildren; @@ -481,8 +551,7 @@ public class DefaultHttp2Connection implements Http2Connection { 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. + // move any previous children to the child node, becoming grand children of this node. for (DefaultStream grandchild : removeAllChildren().values()) { child.takeChild(grandchild, false, events); } @@ -490,31 +559,44 @@ public class DefaultHttp2Connection implements Http2Connection { if (children.put(child.id(), child) == null) { 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); + } + } } } /** * Removes the child priority and moves any of its dependencies to being direct dependencies on this node. */ - final void removeChild(DefaultStream child) { - if (children.remove(child.id()) != null) { - List events = new ArrayList(1 + child.children.size()); + final boolean removeChild(DefaultStream child) { + if (child.prioritizableForTree() == 0 && children.remove(child.id()) != null) { + List events = new ArrayList(1 + child.numChildren()); events.add(new ParentChangedEvent(child, child.parent())); notifyParentChanging(child, null); child.parent = null; totalChildWeights -= child.weight(); + decrementPrioritizableForTree(child.prioritizableForTree()); // Move up any grand children to be directly dependent on this node. for (DefaultStream grandchild : child.children.values()) { takeChild(grandchild, false, events); } + if (prioritizableForTree() == 0) { + removeStream(this); + } notifyParentChanged(events); + return true; } + return false; } } @@ -644,6 +726,16 @@ public class DefaultHttp2Connection implements Http2Connection { super(CONNECTION_STREAM_ID); } + @Override + public boolean isResetSent() { + return false; + } + + @Override + public Http2Stream resetSent() { + throw new UnsupportedOperationException(); + } + @Override public Http2Stream setPriority(int parentStreamId, short weight, boolean exclusive) { throw new UnsupportedOperationException(); diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2Stream.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2Stream.java index 676213d341..0e741a5aaa 100644 --- a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2Stream.java +++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2Stream.java @@ -157,6 +157,13 @@ public interface Http2Stream { */ Http2Stream parent(); + /** + * Get the number of streams in the priority tree rooted at this node that are OK to exist in the priority + * tree on their own right. Some streams may be in the priority tree because their dependents require them to + * remain. + */ + int prioritizableForTree(); + /** * Indicates whether or not this stream is a descendant in the priority tree from the given stream. */ 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 6fe7c5a28f..197be34bae 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 @@ -15,13 +15,13 @@ package io.netty.handler.codec.http2; -import static io.netty.handler.codec.http2.Http2CodecUtil.MIN_WEIGHT; import static io.netty.handler.codec.http2.Http2CodecUtil.DEFAULT_PRIORITY_WEIGHT; +import static io.netty.handler.codec.http2.Http2CodecUtil.MIN_WEIGHT; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; -import static org.junit.Assert.assertTrue; import static org.junit.Assert.assertSame; +import static org.junit.Assert.assertTrue; import static org.mockito.Matchers.any; import static org.mockito.Matchers.anyShort; import static org.mockito.Matchers.eq; @@ -242,6 +242,7 @@ public class DefaultHttp2ConnectionTest { public void prioritizeShouldUseDefaults() throws Exception { Http2Stream stream = client.local().createStream(1).open(false); assertEquals(1, client.connectionStream().numChildren()); + assertEquals(2, client.connectionStream().prioritizableForTree()); assertEquals(stream, client.connectionStream().child(1)); assertEquals(DEFAULT_PRIORITY_WEIGHT, stream.weight()); assertEquals(0, stream.parent().id()); @@ -253,6 +254,7 @@ public class DefaultHttp2ConnectionTest { Http2Stream stream = client.local().createStream(1).open(false); stream.setPriority(0, DEFAULT_PRIORITY_WEIGHT, false); assertEquals(1, client.connectionStream().numChildren()); + assertEquals(2, client.connectionStream().prioritizableForTree()); assertEquals(stream, client.connectionStream().child(1)); assertEquals(DEFAULT_PRIORITY_WEIGHT, stream.weight()); assertEquals(0, stream.parent().id()); @@ -275,6 +277,7 @@ public class DefaultHttp2ConnectionTest { // Level 0 Http2Stream p = client.connectionStream(); assertEquals(1, p.numChildren()); + assertEquals(5, p.prioritizableForTree()); assertEquals(p.numChildren() * DEFAULT_PRIORITY_WEIGHT, p.totalChildWeights()); // Level 1 @@ -282,6 +285,7 @@ public class DefaultHttp2ConnectionTest { 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 @@ -289,6 +293,7 @@ public class DefaultHttp2ConnectionTest { 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 @@ -296,11 +301,13 @@ public class DefaultHttp2ConnectionTest { assertNotNull(p); assertEquals(streamD.id(), p.parent().id()); assertEquals(0, p.numChildren()); + assertEquals(1, p.prioritizableForTree()); assertEquals(p.numChildren() * DEFAULT_PRIORITY_WEIGHT, p.totalChildWeights()); p = p.parent().child(streamC.id()); assertNotNull(p); assertEquals(streamD.id(), p.parent().id()); assertEquals(0, p.numChildren()); + assertEquals(1, p.prioritizableForTree()); assertEquals(p.numChildren() * DEFAULT_PRIORITY_WEIGHT, p.totalChildWeights()); } @@ -327,10 +334,108 @@ public class DefaultHttp2ConnectionTest { any(Http2Stream.class)); verify(clientListener, never()).onPriorityTreeParentChanged(any(Http2Stream.class), any(Http2Stream.class)); + assertEquals(5, client.connectionStream().prioritizableForTree()); + assertEquals(4, streamA.prioritizableForTree()); + assertEquals(1, streamB.prioritizableForTree()); + assertEquals(1, streamC.prioritizableForTree()); + assertEquals(3, streamD.prioritizableForTree()); } @Test - public void removeShouldRestructureTree() throws Exception { + public void sameNodeDependentShouldNotStackOverflowNorChangePrioritizableForTree() 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(streamA.id(), DEFAULT_PRIORITY_WEIGHT, true); + + boolean[] exclusive = new boolean[] {true, false}; + short[] weights = new short[] { DEFAULT_PRIORITY_WEIGHT, 100, 200, streamD.weight() }; + + assertEquals(4, client.numActiveStreams()); + + Http2Stream connectionStream = client.connectionStream(); + assertEquals(5, connectionStream.prioritizableForTree()); + assertEquals(4, streamA.prioritizableForTree()); + assertEquals(1, streamB.prioritizableForTree()); + assertEquals(1, streamC.prioritizableForTree()); + assertEquals(3, streamD.prioritizableForTree()); + + // The goal is to call setPriority with the same parent and vary the parameters + // we were at one point adding a circular depends to the tree and then throwing + // a StackOverflow due to infinite recursive operation. + for (int j = 0; j < weights.length; ++j) { + for (int i = 0; i < exclusive.length; ++i) { + streamD.setPriority(streamA.id(), weights[j], exclusive[i]); + assertEquals(5, connectionStream.prioritizableForTree()); + assertEquals(4, streamA.prioritizableForTree()); + assertEquals(1, streamB.prioritizableForTree()); + assertEquals(1, streamC.prioritizableForTree()); + assertEquals(3, streamD.prioritizableForTree()); + } + } + } + + @Test + public void multipleCircularDependencyShouldUpdatePrioritizable() 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(streamA.id(), DEFAULT_PRIORITY_WEIGHT, true); + + assertEquals(4, client.numActiveStreams()); + + Http2Stream connectionStream = client.connectionStream(); + assertEquals(5, connectionStream.prioritizableForTree()); + assertEquals(4, streamA.prioritizableForTree()); + assertEquals(1, streamB.prioritizableForTree()); + assertEquals(1, streamC.prioritizableForTree()); + assertEquals(3, streamD.prioritizableForTree()); + + // Bring B to the root + streamA.setPriority(streamB.id(), DEFAULT_PRIORITY_WEIGHT, true); + assertEquals(5, connectionStream.prioritizableForTree()); + assertEquals(3, streamA.prioritizableForTree()); + assertEquals(4, streamB.prioritizableForTree()); + assertEquals(1, streamC.prioritizableForTree()); + assertEquals(2, streamD.prioritizableForTree()); + + // Move all streams to be children of B + streamC.setPriority(streamB.id(), DEFAULT_PRIORITY_WEIGHT, false); + streamD.setPriority(streamB.id(), DEFAULT_PRIORITY_WEIGHT, false); + assertEquals(5, connectionStream.prioritizableForTree()); + assertEquals(1, streamA.prioritizableForTree()); + assertEquals(4, streamB.prioritizableForTree()); + assertEquals(1, streamC.prioritizableForTree()); + assertEquals(1, streamD.prioritizableForTree()); + + // Move A back to the root + streamB.setPriority(streamA.id(), DEFAULT_PRIORITY_WEIGHT, true); + assertEquals(5, connectionStream.prioritizableForTree()); + assertEquals(4, streamA.prioritizableForTree()); + assertEquals(3, streamB.prioritizableForTree()); + assertEquals(1, streamC.prioritizableForTree()); + assertEquals(1, streamD.prioritizableForTree()); + + // Move all streams to be children of A + streamC.setPriority(streamA.id(), DEFAULT_PRIORITY_WEIGHT, false); + streamD.setPriority(streamA.id(), DEFAULT_PRIORITY_WEIGHT, false); + assertEquals(5, connectionStream.prioritizableForTree()); + assertEquals(4, streamA.prioritizableForTree()); + assertEquals(1, streamB.prioritizableForTree()); + assertEquals(1, streamC.prioritizableForTree()); + assertEquals(1, streamD.prioritizableForTree()); + } + + @Test + public void removeWithPrioritizableDependentsShouldNotRestructureTree() throws Exception { Http2Stream streamA = client.local().createStream(1).open(false); Http2Stream streamB = client.local().createStream(3).open(false); Http2Stream streamC = client.local().createStream(5).open(false); @@ -345,29 +450,168 @@ public class DefaultHttp2ConnectionTest { // Level 0 Http2Stream p = client.connectionStream(); + assertEquals(4, p.prioritizableForTree()); assertEquals(1, p.numChildren()); assertEquals(p.numChildren() * DEFAULT_PRIORITY_WEIGHT, p.totalChildWeights()); // Level 1 p = p.child(streamA.id()); assertNotNull(p); + assertEquals(3, p.prioritizableForTree()); assertEquals(0, p.parent().id()); - assertEquals(2, p.numChildren()); + assertEquals(1, p.numChildren()); assertEquals(p.numChildren() * DEFAULT_PRIORITY_WEIGHT, p.totalChildWeights()); // Level 2 + p = p.child(streamB.id()); + assertNotNull(p); + assertEquals(2, p.prioritizableForTree()); + assertEquals(streamA.id(), p.parent().id()); + assertEquals(2, p.numChildren()); + assertEquals(p.numChildren() * DEFAULT_PRIORITY_WEIGHT, p.totalChildWeights()); + + // Level 3 p = p.child(streamC.id()); assertNotNull(p); - assertEquals(streamA.id(), p.parent().id()); + assertEquals(1, p.prioritizableForTree()); + assertEquals(streamB.id(), p.parent().id()); assertEquals(0, p.numChildren()); assertEquals(p.numChildren() * DEFAULT_PRIORITY_WEIGHT, p.totalChildWeights()); p = p.parent().child(streamD.id()); assertNotNull(p); - assertEquals(streamA.id(), p.parent().id()); + assertEquals(1, p.prioritizableForTree()); + assertEquals(streamB.id(), p.parent().id()); assertEquals(0, p.numChildren()); assertEquals(p.numChildren() * DEFAULT_PRIORITY_WEIGHT, p.totalChildWeights()); } + @Test + public void closeWithNoPrioritizableDependentsShouldRestructureTree() throws Exception { + 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(streamB.id(), DEFAULT_PRIORITY_WEIGHT, false); + streamD.setPriority(streamB.id(), DEFAULT_PRIORITY_WEIGHT, false); + streamE.setPriority(streamC.id(), DEFAULT_PRIORITY_WEIGHT, false); + streamF.setPriority(streamD.id(), DEFAULT_PRIORITY_WEIGHT, false); + + // Close internal nodes, leave 1 leaf node open, and ensure part of the tree (D & F) is cleaned up + streamA.close(); + streamB.close(); + streamC.close(); + streamD.close(); + streamF.close(); + + // Level 0 + Http2Stream p = client.connectionStream(); + assertEquals(1, p.numChildren()); + assertEquals(2, p.prioritizableForTree()); + assertEquals(p.numChildren() * DEFAULT_PRIORITY_WEIGHT, p.totalChildWeights()); + + // Level 1 + p = p.child(streamA.id()); + assertNotNull(p); + assertEquals(0, p.parent().id()); + assertEquals(1, p.numChildren()); + assertEquals(1, p.prioritizableForTree()); + assertEquals(p.numChildren() * DEFAULT_PRIORITY_WEIGHT, p.totalChildWeights()); + + // Level 2 + p = p.child(streamB.id()); + assertNotNull(p); + assertEquals(streamA.id(), p.parent().id()); + assertEquals(1, p.numChildren()); + assertEquals(1, p.prioritizableForTree()); + assertEquals(p.numChildren() * DEFAULT_PRIORITY_WEIGHT, p.totalChildWeights()); + + // Level 3 + p = p.child(streamC.id()); + assertNotNull(p); + assertEquals(streamB.id(), p.parent().id()); + assertEquals(1, p.numChildren()); + assertEquals(1, p.prioritizableForTree()); + assertEquals(p.numChildren() * DEFAULT_PRIORITY_WEIGHT, p.totalChildWeights()); + + // Level 4 + p = p.child(streamE.id()); + assertNotNull(p); + assertEquals(streamC.id(), p.parent().id()); + assertEquals(0, p.numChildren()); + assertEquals(1, p.prioritizableForTree()); + } + + @Test + public void priorityChangeWithNoPrioritizableDependentsShouldRestructureTree() throws Exception { + 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(streamB.id(), DEFAULT_PRIORITY_WEIGHT, false); + streamD.setPriority(streamB.id(), DEFAULT_PRIORITY_WEIGHT, false); + streamE.setPriority(streamC.id(), DEFAULT_PRIORITY_WEIGHT, false); + streamF.setPriority(streamD.id(), DEFAULT_PRIORITY_WEIGHT, false); + + // Leave leaf nodes open (E & F) + streamA.close(); + streamB.close(); + streamC.close(); + streamD.close(); + + // Move F to depend on C, this should close D + streamF.setPriority(streamC.id(), DEFAULT_PRIORITY_WEIGHT, false); + + // Level 0 + Http2Stream p = client.connectionStream(); + assertEquals(1, p.numChildren()); + assertEquals(3, p.prioritizableForTree()); + assertEquals(p.numChildren() * DEFAULT_PRIORITY_WEIGHT, p.totalChildWeights()); + + // Level 1 + p = p.child(streamA.id()); + assertNotNull(p); + assertEquals(0, p.parent().id()); + assertEquals(1, p.numChildren()); + assertEquals(2, p.prioritizableForTree()); + assertEquals(p.numChildren() * DEFAULT_PRIORITY_WEIGHT, p.totalChildWeights()); + + // Level 2 + p = p.child(streamB.id()); + assertNotNull(p); + assertEquals(streamA.id(), p.parent().id()); + assertEquals(1, p.numChildren()); + assertEquals(2, p.prioritizableForTree()); + assertEquals(p.numChildren() * DEFAULT_PRIORITY_WEIGHT, p.totalChildWeights()); + + // Level 3 + p = p.child(streamC.id()); + assertNotNull(p); + assertEquals(streamB.id(), p.parent().id()); + assertEquals(2, p.numChildren()); + assertEquals(2, p.prioritizableForTree()); + assertEquals(p.numChildren() * DEFAULT_PRIORITY_WEIGHT, p.totalChildWeights()); + + // Level 4 + p = p.child(streamE.id()); + assertNotNull(p); + assertEquals(streamC.id(), p.parent().id()); + assertEquals(0, p.numChildren()); + assertEquals(1, p.prioritizableForTree()); + p = p.parent().child(streamF.id()); + assertNotNull(p); + assertEquals(streamC.id(), p.parent().id()); + assertEquals(0, p.numChildren()); + assertEquals(1, p.prioritizableForTree()); + } + @Test public void circularDependencyShouldRestructureTree() throws Exception { // Using example from http://tools.ietf.org/html/draft-ietf-httpbis-http2-16#section-5.3.3 @@ -423,18 +667,21 @@ public class DefaultHttp2ConnectionTest { // Level 0 Http2Stream p = client.connectionStream(); assertEquals(1, p.numChildren()); + assertEquals(7, p.prioritizableForTree()); assertEquals(p.numChildren() * DEFAULT_PRIORITY_WEIGHT, p.totalChildWeights()); // Level 1 p = p.child(streamD.id()); assertNotNull(p); assertEquals(2, p.numChildren()); + assertEquals(6, p.prioritizableForTree()); assertEquals(p.numChildren() * DEFAULT_PRIORITY_WEIGHT, p.totalChildWeights()); // Level 2 p = p.child(streamF.id()); assertNotNull(p); assertEquals(0, p.numChildren()); + assertEquals(1, p.prioritizableForTree()); assertEquals(p.numChildren() * DEFAULT_PRIORITY_WEIGHT, p.totalChildWeights()); p = p.parent().child(streamA.id()); assertNotNull(p); @@ -445,16 +692,19 @@ public class DefaultHttp2ConnectionTest { p = p.child(streamB.id()); assertNotNull(p); assertEquals(0, p.numChildren()); + assertEquals(1, p.prioritizableForTree()); assertEquals(p.numChildren() * DEFAULT_PRIORITY_WEIGHT, p.totalChildWeights()); p = p.parent().child(streamC.id()); assertNotNull(p); assertEquals(1, p.numChildren()); + assertEquals(2, p.prioritizableForTree()); assertEquals(p.numChildren() * DEFAULT_PRIORITY_WEIGHT, p.totalChildWeights()); // Level 4; p = p.child(streamE.id()); assertNotNull(p); assertEquals(0, p.numChildren()); + assertEquals(1, p.prioritizableForTree()); assertEquals(p.numChildren() * DEFAULT_PRIORITY_WEIGHT, p.totalChildWeights()); } @@ -515,38 +765,45 @@ public class DefaultHttp2ConnectionTest { // Level 0 Http2Stream p = client.connectionStream(); assertEquals(1, p.numChildren()); + assertEquals(7, p.prioritizableForTree()); assertEquals(p.numChildren() * DEFAULT_PRIORITY_WEIGHT, p.totalChildWeights()); // Level 1 p = p.child(streamD.id()); assertNotNull(p); assertEquals(1, p.numChildren()); + assertEquals(6, p.prioritizableForTree()); assertEquals(p.numChildren() * DEFAULT_PRIORITY_WEIGHT, p.totalChildWeights()); // Level 2 p = p.child(streamA.id()); assertNotNull(p); assertEquals(3, p.numChildren()); + assertEquals(5, p.prioritizableForTree()); assertEquals(p.numChildren() * DEFAULT_PRIORITY_WEIGHT, p.totalChildWeights()); // Level 3 p = p.child(streamB.id()); assertNotNull(p); assertEquals(0, p.numChildren()); + assertEquals(1, p.prioritizableForTree()); assertEquals(p.numChildren() * DEFAULT_PRIORITY_WEIGHT, p.totalChildWeights()); p = p.parent().child(streamF.id()); assertNotNull(p); assertEquals(0, p.numChildren()); + assertEquals(1, p.prioritizableForTree()); assertEquals(p.numChildren() * DEFAULT_PRIORITY_WEIGHT, p.totalChildWeights()); p = p.parent().child(streamC.id()); assertNotNull(p); assertEquals(1, p.numChildren()); + assertEquals(2, p.prioritizableForTree()); assertEquals(p.numChildren() * DEFAULT_PRIORITY_WEIGHT, p.totalChildWeights()); // Level 4; p = p.child(streamE.id()); assertNotNull(p); assertEquals(0, p.numChildren()); + assertEquals(1, p.prioritizableForTree()); assertEquals(p.numChildren() * DEFAULT_PRIORITY_WEIGHT, p.totalChildWeights()); } diff --git a/codec-http2/src/test/java/io/netty/handler/codec/http2/DefaultHttp2RemoteFlowControllerTest.java b/codec-http2/src/test/java/io/netty/handler/codec/http2/DefaultHttp2RemoteFlowControllerTest.java index eeae7ca716..064520f97e 100644 --- a/codec-http2/src/test/java/io/netty/handler/codec/http2/DefaultHttp2RemoteFlowControllerTest.java +++ b/codec-http2/src/test/java/io/netty/handler/codec/http2/DefaultHttp2RemoteFlowControllerTest.java @@ -932,7 +932,7 @@ public class DefaultHttp2RemoteFlowControllerTest { } /** - * In this test, we block all streams and remove a node from the priority tree and verify + * In this test, we block all streams and close an internal stream in the priority tree but tree should not change * * 
      *        [0]
@@ -941,17 +941,9 @@ public class DefaultHttp2RemoteFlowControllerTest {
      *      / \
      *     C   D
      *
- * - * After the tree shift: - * - * 
-     *        [0]
-     *       / | \
-     *      C  D  B
-     *
*/ @Test - public void subTreeBytesShouldBeCorrectWithRemoval() throws Http2Exception { + public void subTreeBytesShouldBeCorrectWithInternalStreamClose() throws Http2Exception { // Block the connection exhaustStreamWindow(CONNECTION_STREAM_ID); @@ -987,7 +979,8 @@ public class DefaultHttp2RemoteFlowControllerTest { assertEquals(calculateStreamSizeSum(streamSizes, Arrays.asList(STREAM_B, STREAM_C, STREAM_D)), streamableBytesForTree(stream0)); - assertEquals(0, streamableBytesForTree(streamA)); + assertEquals(calculateStreamSizeSum(streamSizes, Arrays.asList(STREAM_C, STREAM_D)), + streamableBytesForTree(streamA)); assertEquals(calculateStreamSizeSum(streamSizes, Arrays.asList(STREAM_B)), streamableBytesForTree(streamB)); assertEquals(calculateStreamSizeSum(streamSizes, Arrays.asList(STREAM_C)), @@ -996,6 +989,72 @@ public class DefaultHttp2RemoteFlowControllerTest { streamableBytesForTree(streamD)); } + /** + * In this test, we block all streams and close a leaf stream in the priority tree and verify + * + * 
+     *        [0]
+     *        / \
+     *       A   B
+     *      / \
+     *     C   D
+     *
+ * + * After the close: + * 
+     *        [0]
+     *        / \
+     *       A   B
+     *       |
+     *       D
+     *
+ */ + @Test + public void subTreeBytesShouldBeCorrectWithLeafStreamClose() throws Http2Exception { + // Block the connection + exhaustStreamWindow(CONNECTION_STREAM_ID); + + Http2Stream stream0 = connection.connectionStream(); + Http2Stream streamA = connection.stream(STREAM_A); + Http2Stream streamB = connection.stream(STREAM_B); + Http2Stream streamC = connection.stream(STREAM_C); + Http2Stream streamD = connection.stream(STREAM_D); + + // Send a bunch of data on each stream. + final IntObjectMap streamSizes = new IntObjectHashMap(4); + streamSizes.put(STREAM_A, 400); + streamSizes.put(STREAM_B, 500); + streamSizes.put(STREAM_C, 600); + streamSizes.put(STREAM_D, 700); + + FakeFlowControlled dataA = new FakeFlowControlled(streamSizes.get(STREAM_A)); + FakeFlowControlled dataB = new FakeFlowControlled(streamSizes.get(STREAM_B)); + FakeFlowControlled dataC = new FakeFlowControlled(streamSizes.get(STREAM_C)); + FakeFlowControlled dataD = new FakeFlowControlled(streamSizes.get(STREAM_D)); + + sendData(STREAM_A, dataA); + sendData(STREAM_B, dataB); + sendData(STREAM_C, dataC); + sendData(STREAM_D, dataD); + + dataA.assertNotWritten(); + dataB.assertNotWritten(); + dataC.assertNotWritten(); + dataD.assertNotWritten(); + + streamC.close(); + + assertEquals(calculateStreamSizeSum(streamSizes, Arrays.asList(STREAM_A, STREAM_B, STREAM_D)), + streamableBytesForTree(stream0)); + assertEquals(calculateStreamSizeSum(streamSizes, Arrays.asList(STREAM_A, STREAM_D)), + streamableBytesForTree(streamA)); + assertEquals(calculateStreamSizeSum(streamSizes, Arrays.asList(STREAM_B)), + streamableBytesForTree(streamB)); + assertEquals(0, streamableBytesForTree(streamC)); + assertEquals(calculateStreamSizeSum(streamSizes, Arrays.asList(STREAM_D)), + streamableBytesForTree(streamD)); + } + @Test public void flowControlledWriteThrowsAnException() throws Exception { final Http2RemoteFlowController.FlowControlled flowControlled = mockedFlowControlledThatThrowsOnWrite();