HTTP/2 DefaultHttp2Connection NPE

Motivation:
If while iterating the active streams a close operation occurs this will be queued and process after the iteration has completed to avoid a concurrent modification exception. However it is possible that during the iteration the stream which was closed could have been removed from the priority tree and its parent would be set to null. Then after the iteration completes the close operation will attempt to dereference the parent and results in a NPE.

Modifications:
- pending close operations should verify the stream's parent is not null before processing the event

Result:
No More NPE.
This commit is contained in:
Scott Mitchell 2016-03-02 16:24:52 -08:00
parent 6536c7c4ef
commit bd6040a36e
3 changed files with 26 additions and 2 deletions

View File

@ -1181,6 +1181,14 @@ public class DefaultHttp2Connection implements Http2Connection {
pendingEvents.add(new Event() {
@Override
public void process() {
// When deactivate is called the stream state has already been set to CLOSE however
// it is possible that since this job has been queued other circumstances have caused
// it to be removed from the priority tree and thus have a null parent (i.e. reprioritization).
// If the parent is null this means it has already been removed from active streams and we
// should not process the removal any further as this will lead to a NPE.
if (stream.parent() == null) {
return;
}
removeFromActiveStreams(stream, itr);
}
});
@ -1233,8 +1241,8 @@ public class DefaultHttp2Connection implements Http2Connection {
if (streams.remove(stream)) {
// Update the number of active streams initiated by the endpoint.
stream.createdBy().numActiveStreams--;
notifyClosed(stream);
}
notifyClosed(stream);
removeStream(stream, itr);
}

View File

@ -127,7 +127,7 @@ public interface Http2Stream {
<V> V removeProperty(Http2Connection.PropertyKey key);
/**
* Updates an priority for this stream. Calling this method may affect the straucture of the
* Updates an priority for this stream. Calling this method may affect the structure of the
* priority tree.
*
* @param parentStreamId the parent stream that given stream should depend on. May be {@code 0},

View File

@ -221,6 +221,22 @@ public class DefaultHttp2ConnectionTest {
assertTrue(latch.await(2, TimeUnit.SECONDS));
}
@Test
public void closeWhileIteratingDoesNotNPE() throws Http2Exception {
final Http2Stream streamA = client.local().createStream(3, false);
final Http2Stream streamB = client.local().createStream(5, false);
final Http2Stream streamC = client.local().createStream(7, false);
streamB.setPriority(streamA.id(), Http2CodecUtil.DEFAULT_PRIORITY_WEIGHT, false);
client.forEachActiveStream(new Http2StreamVisitor() {
@Override
public boolean visit(Http2Stream stream) throws Http2Exception {
streamA.close();
streamB.setPriority(streamC.id(), Http2CodecUtil.DEFAULT_PRIORITY_WEIGHT, false);
return true;
}
});
}
@Test
public void goAwayReceivedShouldCloseStreamsGreaterThanLastStream() throws Exception {
Http2Stream stream1 = client.local().createStream(3, false);