Skip to content

Commit

Permalink
Cleanup HTTP/2 tests for Http2FrameCodec and Http2MultiplexCodec (#8646)
Browse files Browse the repository at this point in the history
Motiviation:

Http2FrameCodecTest and Http2MultiplexCodecTest were quite fragile and often not went through the whole pipeline which made testing sometimes hard and error-prone.

Modification:

- Refactor tests to have data flow through the whole pipeline and so made the test more robust (by testing the while implementation).

Result:

Easier to write tests for the codecs in the future and more robust testing in general.

Beside this it also fixes #6036.
  • Loading branch information
normanmaurer committed Dec 28, 2018
1 parent 66ccd14 commit fa84e2b
Show file tree
Hide file tree
Showing 8 changed files with 1,030 additions and 643 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ public class Http2FrameCodec extends Http2ConnectionHandler {

private final Integer initialFlowControlWindowSize;

private ChannelHandlerContext ctx;
ChannelHandlerContext ctx;

/** Number of buffered streams if the {@link StreamBufferingEncoder} is used. **/
private int numBufferedStreams;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -414,28 +414,11 @@ final void onChannelReadComplete(ChannelHandlerContext ctx) {
}
}

// Allow to override for testing
void flush0(ChannelHandlerContext ctx) {
final void flush0(ChannelHandlerContext ctx) {
flush(ctx);
}

/**
* Return bytes to flow control.
* <p>
* Package private to allow to override for testing
* @param ctx The {@link ChannelHandlerContext} associated with the parent channel.
* @param stream The object representing the HTTP/2 stream.
* @param bytes The number of bytes to return to flow control.
* @return {@code true} if a frame has been written as a result of this method call.
* @throws Http2Exception If this operation violates the flow control limits.
*/
boolean onBytesConsumed(@SuppressWarnings("unused") ChannelHandlerContext ctx,
Http2FrameStream stream, int bytes) throws Http2Exception {
return consumeBytes(stream.id(), bytes);
}

// Allow to extend for testing
static class Http2MultiplexCodecStream extends DefaultHttp2FrameStream {
static final class Http2MultiplexCodecStream extends DefaultHttp2FrameStream {
DefaultHttp2StreamChannel channel;
}

Expand Down Expand Up @@ -1084,7 +1067,7 @@ void doRead0(Http2Frame frame, Handle allocHandle) {
allocHandle.lastBytesRead(numBytesToBeConsumed);
if (numBytesToBeConsumed != 0) {
try {
writeDoneAndNoFlush |= onBytesConsumed(ctx, stream, numBytesToBeConsumed);
writeDoneAndNoFlush |= consumeBytes(stream.id(), numBytesToBeConsumed);
} catch (Http2Exception e) {
pipeline().fireExceptionCaught(e);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
@UnstableApi
public class Http2MultiplexCodecBuilder
extends AbstractHttp2ConnectionHandlerBuilder<Http2MultiplexCodec, Http2MultiplexCodecBuilder> {
private Http2FrameWriter frameWriter;

final ChannelHandler childHandler;
private ChannelHandler upgradeStreamHandler;
Expand All @@ -44,6 +45,12 @@ private static ChannelHandler checkSharable(ChannelHandler handler) {
return handler;
}

// For testing only.
Http2MultiplexCodecBuilder frameWriter(Http2FrameWriter frameWriter) {
this.frameWriter = checkNotNull(frameWriter, "frameWriter");
return this;
}

/**
* Creates a builder for a HTTP/2 client.
*
Expand Down Expand Up @@ -160,6 +167,28 @@ public Http2MultiplexCodecBuilder initialHuffmanDecodeCapacity(int initialHuffma

@Override
public Http2MultiplexCodec build() {
Http2FrameWriter frameWriter = this.frameWriter;
if (frameWriter != null) {
// This is to support our tests and will never be executed by the user as frameWriter(...)
// is package-private.
DefaultHttp2Connection connection = new DefaultHttp2Connection(isServer(), maxReservedStreams());
Long maxHeaderListSize = initialSettings().maxHeaderListSize();
Http2FrameReader frameReader = new DefaultHttp2FrameReader(maxHeaderListSize == null ?
new DefaultHttp2HeadersDecoder(true) :
new DefaultHttp2HeadersDecoder(true, maxHeaderListSize));

if (frameLogger() != null) {
frameWriter = new Http2OutboundFrameLogger(frameWriter, frameLogger());
frameReader = new Http2InboundFrameLogger(frameReader, frameLogger());
}
Http2ConnectionEncoder encoder = new DefaultHttp2ConnectionEncoder(connection, frameWriter);
if (encoderEnforceMaxConcurrentStreams()) {
encoder = new StreamBufferingEncoder(encoder);
}
Http2ConnectionDecoder decoder = new DefaultHttp2ConnectionDecoder(connection, encoder, frameReader);

return build(decoder, encoder, initialSettings());
}
return super.build();
}

Expand Down
Loading

0 comments on commit fa84e2b

Please sign in to comment.