Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 26 additions & 1 deletion source/common/http/http1/codec_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1143,13 +1143,38 @@ void ServerConnectionImpl::onBody(Buffer::Instance& data) {
}
}

Http::Status ServerConnectionImpl::dispatch(Buffer::Instance& data) {
if (active_request_ != nullptr && active_request_->remote_complete_) {
// Active downstream request remote complete but there is some new data comming then try to
// disable the connection reading. Connection reading will be re-enabled after the current
// active downstream request has completed.
// This ensures that the new comming data can be consumed after the current active downstream
// request has completed.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit hard to grok, perhaps:
Read disable the connection if the downstream is sending additional data while we are working on an existing request. Reading from the connection will be re-enabled after the active request is completed.

active_request_->response_encoder_.readDisable(true);
return okStatus();
}

Http::Status status = ConnectionImpl::dispatch(data);

if (active_request_ != nullptr && active_request_->remote_complete_) {
// Active downstream request remote complete but there is some remaining data in the read buffer
// then try to disable the connection reading. Connection reading will be re-enabled after the
// current active downstream request has completed.
// This ensures that the remaining data can be consumed after the current active downstream
// request has completed.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit hard to grok, perhaps:
Eagerly read disable the connection if the downstream is sending pipelined requests as we serially process them. Reading from the connection will be re-enabled after the active request is completed.

if (data.length() > 0) {
active_request_->response_encoder_.readDisable(true);
}
}
return status;
}

ParserStatus ServerConnectionImpl::onMessageCompleteBase() {
ASSERT(!handling_upgrade_);
if (active_request_) {

// The request_decoder should be non-null after we've called the newStream on callbacks.
ASSERT(active_request_->request_decoder_);
active_request_->response_encoder_.readDisable(true);
active_request_->remote_complete_ = true;

if (deferred_end_stream_headers_) {
Expand Down
3 changes: 1 addition & 2 deletions source/common/http/http1/codec_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -81,8 +81,6 @@ class StreamEncoderImpl : public virtual StreamEncoder,
void setIsResponseToConnectRequest(bool value) { is_response_to_connect_request_ = value; }
void setDetails(absl::string_view details) { details_ = details; }

void clearReadDisableCallsForTests() { read_disable_calls_ = 0; }

const StreamInfo::BytesMeterSharedPtr& bytesMeter() override { return bytes_meter_; }

protected:
Expand Down Expand Up @@ -477,6 +475,7 @@ class ServerConnectionImpl : public ServerConnection, public ConnectionImpl {
Status onUrl(const char* data, size_t length) override;
Status onStatus(const char*, size_t) override { return okStatus(); }
// ConnectionImpl
Http::Status dispatch(Buffer::Instance& data) override;
void onEncodeComplete() override;
StreamInfo::BytesMeter& getBytesMeter() override {
if (active_request_) {
Expand Down
20 changes: 0 additions & 20 deletions test/integration/fake_upstream.cc
Original file line number Diff line number Diff line change
Expand Up @@ -298,29 +298,9 @@ void FakeStream::finishGrpcStream(Grpc::Status::GrpcStatus status) {
{"grpc-status", std::to_string(static_cast<uint32_t>(status))}});
}

// The TestHttp1ServerConnectionImpl outlives its underlying Network::Connection
// so must not access the Connection on teardown. To achieve this, clear the
// read disable calls to avoid checking / editing the Connection blocked state.
class TestHttp1ServerConnectionImpl : public Http::Http1::ServerConnectionImpl {
public:
using Http::Http1::ServerConnectionImpl::ServerConnectionImpl;

Http::Http1::ParserStatus onMessageCompleteBase() override {
auto rc = ServerConnectionImpl::onMessageCompleteBase();

if (activeRequest() && activeRequest()->request_decoder_) {
// Undo the read disable from the base class - we have many tests which
// waitForDisconnect after a full request has been read which will not
// receive the disconnect if reading is disabled.
activeRequest()->response_encoder_.readDisable(false);
}
return rc;
}
~TestHttp1ServerConnectionImpl() override {
if (activeRequest()) {
activeRequest()->response_encoder_.clearReadDisableCallsForTests();
}
}
};

class TestHttp2ServerConnectionImpl : public Http::Http2::ServerConnectionImpl {
Expand Down