-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
http/2: fix deferred reset behavior #1145
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 { | ||
|
||
|
@@ -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_; | ||
})); | ||
} | ||
|
@@ -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) { | ||
|
@@ -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); | ||
} | ||
|
@@ -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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can this be made explicit by verifying decodeHeaders is not called on a response decoder? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Implicitly it is because of the |
||
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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why don't you need to flush SETTINGS as in the client case above? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will add more comments. |
||
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, \ | ||
|
@@ -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) { | ||
{ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we made the "pending deferred reset" a parameter to
sendPendingFrames()
now? It seems likepending_deferred_reset_
is acting as effectively a parameter for the pending case behavior,sendPendingFrames()
is the only consumer.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately we can't, because sendPendingFrames() will return immediately if we are currently dispatching read data (interleaving read/write is not possible w/ nghttp2). So we still need the flag for the currently dispatching case.