Skip to content

Commit

Permalink
Fixing watermark preconditions (#1509)
Browse files Browse the repository at this point in the history
This fixes a bug in my implementation of #150 where we were passing connection-level flow control blockage to new client side streams but not new server side streams.
  • Loading branch information
alyssawilk authored and htuch committed Aug 22, 2017
1 parent 7899547 commit 23b2809
Show file tree
Hide file tree
Showing 2 changed files with 10 additions and 3 deletions.
3 changes: 3 additions & 0 deletions source/common/http/http2/codec_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -802,6 +802,9 @@ int ServerConnectionImpl::onBeginHeaders(const nghttp2_frame* frame) {
}

StreamImplPtr stream(new ServerStreamImpl(*this, per_stream_buffer_limit_));
if (connection_.aboveHighWatermark()) {
stream->runHighWatermarkCallbacks();
}
stream->decoder_ = &callbacks_.newStream(*stream);
stream->stream_id_ = frame->hd.stream_id;
stream->moveIntoList(std::move(stream), active_streams_);
Expand Down
10 changes: 7 additions & 3 deletions test/common/http/http2/codec_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -399,6 +399,10 @@ TEST_P(Http2CodecImplFlowControlTest, TestFlowControlInPendingSendData) {
StreamEncoder* response_encoder2;
MockStreamCallbacks server_stream_callbacks2;
MockStreamDecoder request_decoder2;
// When the server stream is created it should check the status of the
// underlying connection. Pretend it is overrun.
EXPECT_CALL(server_connection_, aboveHighWatermark()).WillOnce(Return(true));
EXPECT_CALL(server_stream_callbacks2, onAboveWriteBufferHighWatermark());
EXPECT_CALL(server_callbacks_, newStream(_))
.WillOnce(Invoke([&](StreamEncoder& encoder) -> StreamDecoder& {
response_encoder2 = &encoder;
Expand All @@ -411,7 +415,7 @@ TEST_P(Http2CodecImplFlowControlTest, TestFlowControlInPendingSendData) {
// been noticed that the connection was backed up. Any new subscriber to
// stream callbacks should get a callback when they addCallbacks.
MockStreamCallbacks callbacks2;
EXPECT_CALL(callbacks2, onAboveWriteBufferHighWatermark()).Times(1);
EXPECT_CALL(callbacks2, onAboveWriteBufferHighWatermark());
request_encoder_->getStream().addCallbacks(callbacks2);

// Now unblock the server's stream. This will cause the bytes to be consumed, flow control
Expand Down Expand Up @@ -504,8 +508,8 @@ TEST_P(Http2CodecImplTest, WatermarkUnderEndStream) {
// ends.
EXPECT_CALL(callbacks, onAboveWriteBufferHighWatermark()).Times(0);
EXPECT_CALL(callbacks, onBelowWriteBufferLowWatermark()).Times(0);
EXPECT_CALL(server_stream_callbacks_, onAboveWriteBufferHighWatermark()).Times(1);
EXPECT_CALL(server_stream_callbacks_, onBelowWriteBufferLowWatermark()).Times(1);
EXPECT_CALL(server_stream_callbacks_, onAboveWriteBufferHighWatermark());
EXPECT_CALL(server_stream_callbacks_, onBelowWriteBufferLowWatermark());
EXPECT_CALL(request_decoder_, decodeData(_, true)).WillOnce(InvokeWithoutArgs([&]() -> void {
client_.onUnderlyingConnectionAboveWriteBufferHighWatermark();
client_.onUnderlyingConnectionBelowWriteBufferLowWatermark();
Expand Down

0 comments on commit 23b2809

Please sign in to comment.