From 2b3e0e675a62c8dfaaf735d4b57df17a45a46422 Mon Sep 17 00:00:00 2001 From: Scott Mitchell Date: Wed, 25 Mar 2015 12:57:14 -0700 Subject: [PATCH] HTTP/2 Closed Streams Conditional Priority Tree Removal Motivation: The HTTP/2 specification allows for closed (and streams in any state) to exist in the priority tree. The current code removes streams from the priority tree as soon as they are closed (subject to the removal policy). This may lead to undesired distribution of resources from the peer's perspective. Modifications: - We should only remove streams from the priority tree when they have no descendant streams in a viable state. - We should track when tree edges change or nodes are removed if inviable nodes can then be removed. Result: Priority tree doesn't remove closed streams until descendant are all closed, or there are no descendants. --- .../codec/http2/DefaultHttp2Connection.java | 132 +++++++-- .../handler/codec/http2/Http2Stream.java | 7 + .../http2/DefaultHttp2ConnectionTest.java | 269 +++++++++++++++++- .../DefaultHttp2RemoteFlowControllerTest.java | 81 +++++- 4 files changed, 452 insertions(+), 37 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 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();