Skip to content

Commit

Permalink
http/2: fix deferred reset behavior
Browse files Browse the repository at this point in the history 
The logic was broken and would not invoke the deferred reset stream
cleanup logic in all cases. This now does that and the test coverage
has been improved for both the client and server case.

Fixes #1138
  • Loading branch information
@mattklein123
mattklein123 committed Jun 20, 2017
1 parent 9e950eb commit cae252a
Show file tree
Hide file tree
Showing 2 changed files with 87 additions and 22 deletions.
7 changes: 6 additions & 1 deletion source/common/http/http2/codec_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -206,10 +206,15 @@ void ConnectionImpl::StreamImpl::resetStream(StreamResetReason reason) {
if (local_end_stream_ && !local_end_stream_sent_) {
parent_.pending_deferred_reset_ = true;
deferred_reset_.value(reason);
ENVOY_CONN_LOG(trace, "deferred reset stream", parent_.connection_);
} else {
resetStreamWorker(reason);
parent_.sendPendingFrames();
}

// We must still call sendPendingFrames() in both the deferred and not deferred path. This forces
// the cleanup logic to run which will reset the stream in all cases if all data frames could not
// be sent.
parent_.sendPendingFrames();
}

void ConnectionImpl::StreamImpl::resetStreamWorker(StreamResetReason reason) {
Expand Down
102 changes: 81 additions & 21 deletions test/common/http/http2/codec_impl_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,14 @@
#include "gmock/gmock.h"
#include "gtest/gtest.h"

namespace Envoy {

using testing::_;
using testing::AtLeast;
using testing::InSequence;
using testing::Invoke;
using testing::InvokeWithoutArgs;
using testing::NiceMock;

namespace Envoy {
namespace Http {
namespace Http2 {

Expand Down Expand Up @@ -65,11 +65,14 @@ class Http2CodecImplTest : public testing::TestWithParam<Http2SettingsTestParam>
server_(server_connection_, server_callbacks_, stats_store_, server_http2settings_),
request_encoder_(client_.newStream(response_decoder_)) {
setupDefaultConnectionMocks();
expectServerStream();
}

void expectServerStream() {
EXPECT_CALL(server_callbacks_, newStream(_))
.WillOnce(Invoke([&](StreamEncoder& encoder) -> StreamDecoder& {
response_encoder_ = &encoder;
encoder.getStream().addCallbacks(callbacks_);
encoder.getStream().addCallbacks(server_stream_callbacks_);
return request_decoder_;
}));
}
Expand Down Expand Up @@ -98,7 +101,7 @@ class Http2CodecImplTest : public testing::TestWithParam<Http2SettingsTestParam>
StreamEncoder& request_encoder_;
MockStreamDecoder request_decoder_;
StreamEncoder* response_encoder_{};
MockStreamCallbacks callbacks_;
MockStreamCallbacks server_stream_callbacks_;
};

TEST_P(Http2CodecImplTest, ExpectContinueHeadersOnlyResponse) {
Expand Down Expand Up @@ -168,7 +171,7 @@ TEST_P(Http2CodecImplTest, RefusedStreamReset) {

MockStreamCallbacks callbacks;
request_encoder_.getStream().addCallbacks(callbacks);
EXPECT_CALL(callbacks_, onResetStream(StreamResetReason::LocalRefusedStreamReset));
EXPECT_CALL(server_stream_callbacks_, onResetStream(StreamResetReason::LocalRefusedStreamReset));
EXPECT_CALL(callbacks, onResetStream(StreamResetReason::RemoteRefusedStreamReset));
response_encoder_->getStream().resetStream(StreamResetReason::LocalRefusedStreamReset);
}
Expand Down Expand Up @@ -232,42 +235,99 @@ TEST_P(Http2CodecImplTest, TrailingHeadersLargeBody) {
response_encoder_->encodeTrailers(TestHeaderMapImpl{{"trailing", "header"}});
}

TEST_P(Http2CodecImplTest, DeferredReset) {
class Http2CodecImplDeferredResetTest : public Http2CodecImplTest {};

TEST_P(Http2CodecImplDeferredResetTest, DeferredResetClient) {
InSequence s;

// Buffer server data so we can make sure we don't get any window updates.
// Do a basic request/response to flush settings.
TestHeaderMapImpl request_headers;
HttpTestUtility::addDefaultHeaders(request_headers);
EXPECT_CALL(request_decoder_, decodeHeaders_(_, true));
request_encoder_.encodeHeaders(request_headers, true);
TestHeaderMapImpl response_headers{{":status", "200"}};
EXPECT_CALL(response_decoder_, decodeHeaders_(_, true));
response_encoder_->encodeHeaders(response_headers, true);

// Do a 2nd request, but pause server dispatch so we don't send window updates. This will
// result in a deferred reset, followed by a flush which will cause the stream to actually
// be reset.
ON_CALL(client_connection_, write(_))
.WillByDefault(
Invoke([&](Buffer::Instance& data) -> void { server_wrapper_.buffer_.add(data); }));
StreamEncoder& request_encoder2 = client_.newStream(response_decoder_);
request_encoder2.encodeHeaders(request_headers, false);
Buffer::OwnedImpl body(std::string(1024 * 1024, 'a'));
request_encoder2.encodeData(body, true);
request_encoder2.getStream().resetStream(StreamResetReason::LocalReset);

// Dispatch server. We expect to see some data.
expectServerStream();
EXPECT_CALL(request_decoder_, decodeHeaders_(_, false))
.WillOnce(InvokeWithoutArgs([&]() -> void {
// Start a response inside the headers callback. This should not result in the client
// seeing any headers as the stream should already be reset on the other side.
TestHeaderMapImpl response_headers{{":status", "200"}};
response_encoder_->encodeHeaders(response_headers, false);
}));
EXPECT_CALL(request_decoder_, decodeData(_, false)).Times(AtLeast(1));
EXPECT_CALL(server_stream_callbacks_, onResetStream(StreamResetReason::RemoteReset));

setupDefaultConnectionMocks();
server_wrapper_.dispatch(Buffer::OwnedImpl(), server_);
}

TEST_P(Http2CodecImplDeferredResetTest, DeferredResetServer) {
InSequence s;

// This will send a request that is bigger than the initial window size. When we then reset it,
// after attempting to flush we should reset the stream and drop the data we could not send.
TestHeaderMapImpl request_headers;
HttpTestUtility::addDefaultHeaders(request_headers);
EXPECT_CALL(request_decoder_, decodeHeaders_(_, false));
request_encoder_.encodeHeaders(request_headers, false);

// In this case we do the same thing as DeferredResetClient but on the server side.
ON_CALL(server_connection_, write(_))
.WillByDefault(
Invoke([&](Buffer::Instance& data) -> void { client_wrapper_.buffer_.add(data); }));
TestHeaderMapImpl response_headers{{":status", "200"}};
response_encoder_->encodeHeaders(response_headers, false);
Buffer::OwnedImpl body(std::string(1024 * 1024, 'a'));
request_encoder_.encodeData(body, true);
request_encoder_.getStream().resetStream(StreamResetReason::LocalReset);
response_encoder_->encodeData(body, true);
EXPECT_CALL(server_stream_callbacks_, onResetStream(StreamResetReason::LocalReset));
response_encoder_->getStream().resetStream(StreamResetReason::LocalReset);

EXPECT_CALL(request_decoder_, decodeHeaders_(_, false));
EXPECT_CALL(request_decoder_, decodeData(_, false)).Times(AtLeast(1));
EXPECT_CALL(callbacks_, onResetStream(StreamResetReason::RemoteReset));
server_wrapper_.dispatch(Buffer::OwnedImpl(), server_);
MockStreamCallbacks client_stream_callbacks;
request_encoder_.getStream().addCallbacks(client_stream_callbacks);
EXPECT_CALL(response_decoder_, decodeHeaders_(_, false));
EXPECT_CALL(response_decoder_, decodeData(_, false)).Times(AtLeast(1));
EXPECT_CALL(client_stream_callbacks, onResetStream(StreamResetReason::RemoteReset));
setupDefaultConnectionMocks();
client_wrapper_.dispatch(Buffer::OwnedImpl(), client_);
}

// we seperate default/edge cases here to avoid combinatorial explosion
// Deferred reset tests use only small windows so that we can test certain conditions.
#define HTTP2SETTINGS_DEFERRED_RESET_COMBINE \
::testing::Combine(::testing::Values(Http2Settings::DEFAULT_HPACK_TABLE_SIZE), \
::testing::Values(Http2Settings::DEFAULT_MAX_CONCURRENT_STREAMS), \
::testing::Values(Http2Settings::MIN_INITIAL_STREAM_WINDOW_SIZE), \
::testing::Values(Http2Settings::MIN_INITIAL_CONNECTION_WINDOW_SIZE))

#define HTTP2SETTINGS_DEFAULT_COMBIME \
INSTANTIATE_TEST_CASE_P(Http2CodecImplDeferredResetTest, Http2CodecImplDeferredResetTest,
::testing::Combine(HTTP2SETTINGS_DEFERRED_RESET_COMBINE,
HTTP2SETTINGS_DEFERRED_RESET_COMBINE));

// we seperate default/edge cases here to avoid combinatorial explosion
#define HTTP2SETTINGS_DEFAULT_COMBINE \
::testing::Combine(::testing::Values(Http2Settings::DEFAULT_HPACK_TABLE_SIZE), \
::testing::Values(Http2Settings::DEFAULT_MAX_CONCURRENT_STREAMS), \
::testing::Values(Http2Settings::DEFAULT_INITIAL_STREAM_WINDOW_SIZE), \
::testing::Values(Http2Settings::DEFAULT_INITIAL_CONNECTION_WINDOW_SIZE))

INSTANTIATE_TEST_CASE_P(Http2CodecImplTestDefaultSettings, Http2CodecImplTest,
::testing::Combine(HTTP2SETTINGS_DEFAULT_COMBIME,
HTTP2SETTINGS_DEFAULT_COMBIME));
::testing::Combine(HTTP2SETTINGS_DEFAULT_COMBINE,
HTTP2SETTINGS_DEFAULT_COMBINE));

#define HTTP2SETTINGS_EDGE_COMBIME \
#define HTTP2SETTINGS_EDGE_COMBINE \
::testing::Combine( \
::testing::Values(Http2Settings::MIN_HPACK_TABLE_SIZE, Http2Settings::MAX_HPACK_TABLE_SIZE), \
::testing::Values(Http2Settings::MIN_MAX_CONCURRENT_STREAMS, \
Expand All @@ -278,7 +338,7 @@ INSTANTIATE_TEST_CASE_P(Http2CodecImplTestDefaultSettings, Http2CodecImplTest,
Http2Settings::MAX_INITIAL_CONNECTION_WINDOW_SIZE))

INSTANTIATE_TEST_CASE_P(Http2CodecImplTestEdgeSettings, Http2CodecImplTest,
::testing::Combine(HTTP2SETTINGS_EDGE_COMBIME, HTTP2SETTINGS_EDGE_COMBIME));
::testing::Combine(HTTP2SETTINGS_EDGE_COMBINE, HTTP2SETTINGS_EDGE_COMBINE));

TEST(Http2CodecUtility, reconstituteCrumbledCookies) {
{
Expand Down

0 comments on commit cae252a

Please sign in to comment.