Skip to content

Commit

Permalink
Fix HTTP/2 dependency tree corruption
Browse files Browse the repository at this point in the history
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 netty#6621
  • Loading branch information
ejona86 authored and Scottmitch committed Apr 22, 2017
1 parent c6ad933 commit 799350c
Show file tree
Hide file tree
Showing 2 changed files with 38 additions and 0 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,10 @@ public void updateDependencyTree(int childStreamId, int parentStreamId, short we
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<ParentChangedEvent> events = new ArrayList<ParentChangedEvent>(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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -899,4 +899,38 @@ public void circularDependencyWithExclusiveShouldRestructureTree() throws Except
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()));
}
}

0 comments on commit 799350c

Please sign in to comment.