From 799350c369e68462b61c6aef97db2a33ea937434 Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Fri, 21 Apr 2017 17:05:30 -0700 Subject: [PATCH] Fix HTTP/2 dependency tree corruption Motivation: Chrome was randomly getting stuck loading the tiles examples. Investigation showed that the Netty flow controller thought it had nothing to send for the connection even though some streams has queued data and window available. Modifications: Fixed an accounting error where an implicitly created parent was not being added to the dependency tree, thus it and all of its children were orphaned from the connection's tree and would never have data written. Result: Fixes #6621 --- .../WeightedFairQueueByteDistributor.java | 4 +++ ...ueueByteDistributorDependencyTreeTest.java | 34 +++++++++++++++++++ 2 files changed, 38 insertions(+) diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/WeightedFairQueueByteDistributor.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/WeightedFairQueueByteDistributor.java index d61c24366e..7c94a3c1f5 100644 --- a/codec-http2/src/main/java/io/netty/handler/codec/http2/WeightedFairQueueByteDistributor.java +++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/WeightedFairQueueByteDistributor.java @@ -229,6 +229,10 @@ public final class WeightedFairQueueByteDistributor implements StreamByteDistrib newParent = new State(parentStreamId); stateOnlyRemovalQueue.add(newParent); stateOnlyMap.put(parentStreamId, newParent); + // Only the stream which was just added will change parents. So we only need an array of size 1. + List events = new ArrayList(1); + connectionState.takeChild(newParent, false, events); + notifyParentChanged(events); } // if activeCountForTree == 0 then it will not be in its parent's pseudoTimeQueue and thus should not be counted diff --git a/codec-http2/src/test/java/io/netty/handler/codec/http2/WeightedFairQueueByteDistributorDependencyTreeTest.java b/codec-http2/src/test/java/io/netty/handler/codec/http2/WeightedFairQueueByteDistributorDependencyTreeTest.java index cb4f8e2f93..164d6529cb 100644 --- a/codec-http2/src/test/java/io/netty/handler/codec/http2/WeightedFairQueueByteDistributorDependencyTreeTest.java +++ b/codec-http2/src/test/java/io/netty/handler/codec/http2/WeightedFairQueueByteDistributorDependencyTreeTest.java @@ -899,4 +899,38 @@ public class WeightedFairQueueByteDistributorDependencyTreeTest extends assertTrue(distributor.isChild(streamE.id(), streamC.id(), DEFAULT_PRIORITY_WEIGHT)); assertEquals(0, distributor.numChildren(streamE.id())); } + + // Unknown parent streams can come about in two ways: + // 1. Because the stream is old and its state was purged + // 2. This is the first reference to the stream, as implied at least by RFC7540§5.3.1: + // > A dependency on a stream that is not currently in the tree — such as a stream in the + // > "idle" state — results in that stream being given a default priority + @Test + public void unknownParentShouldBeCreatedUnderConnection() throws Exception { + setup(5); + + // Purposefully avoid creating streamA's Http2Stream so that is it completely unknown. + // It shouldn't matter whether the ID is before or after streamB.id() + int streamAId = 1; + Http2Stream streamB = connection.local().createStream(3, false); + + assertEquals(1, distributor.numChildren(connection.connectionStream().id())); + assertEquals(0, distributor.numChildren(streamB.id())); + + // Build the tree + setPriority(streamB.id(), streamAId, DEFAULT_PRIORITY_WEIGHT, false); + + assertEquals(1, connection.numActiveStreams()); + + // Level 0 + assertEquals(1, distributor.numChildren(connection.connectionStream().id())); + + // Level 1 + assertTrue(distributor.isChild(streamAId, connection.connectionStream().id(), DEFAULT_PRIORITY_WEIGHT)); + assertEquals(1, distributor.numChildren(streamAId)); + + // Level 2 + assertTrue(distributor.isChild(streamB.id(), streamAId, DEFAULT_PRIORITY_WEIGHT)); + assertEquals(0, distributor.numChildren(streamB.id())); + } }