Skip to content

Commit

Permalink
http2: Remove RELEASE_ASSERTs in sendPendingFrames() error handling (#…
Browse files Browse the repository at this point in the history 
…13546)

Signed-off-by: Yan Avlasov <yavlasov@google.com>
  • Loading branch information
@yanavlasov
yanavlasov authored Oct 16, 2020
1 parent 499f46a commit 11289b9
Show file tree
Hide file tree
Showing 3 changed files with 98 additions and 160 deletions.
181 changes: 72 additions & 109 deletions source/common/http/http2/codec_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -177,17 +177,10 @@ void ConnectionImpl::StreamImpl::encodeHeadersBase(const std::vector<nghttp2_nv>

local_end_stream_ = end_stream;
submitHeaders(final_headers, end_stream ? nullptr : &provider);
auto status = parent_.sendPendingFrames();
// The RELEASE_ASSERT below does not change the existing behavior of `sendPendingFrames()`.
// The `sendPendingFrames()` used to throw on errors and the only method that was catching
// these exceptions was the `dispatch()`. The `dispatch()` method still checks and handles
// errors returned by the `sendPendingFrames()`.
// Other callers of `sendPendingFrames()` do not catch exceptions from this method and
// would cause abnormal process termination in error cases. This change replaces abnormal
// process termination from unhandled exception with the RELEASE_ASSERT.
// Further work will replace this RELEASE_ASSERT with proper error handling.
RELEASE_ASSERT(status.ok(), "sendPendingFrames() failure in non dispatching context");
parent_.checkProtocolConstraintViolation();
if (parent_.sendPendingFramesAndHandleError()) {
// Intended to check through coverage that this error case is tested
return;
}
}

void ConnectionImpl::ClientStreamImpl::encodeHeaders(const RequestHeaderMap& headers,
Expand Down Expand Up @@ -255,10 +248,10 @@ void ConnectionImpl::StreamImpl::encodeTrailersBase(const HeaderMap& trailers) {
}
} else {
submitTrailers(trailers);
auto status = parent_.sendPendingFrames();
// See comment in the `encodeHeadersBase()` method about this RELEASE_ASSERT.
RELEASE_ASSERT(status.ok(), "sendPendingFrames() failure in non dispatching context");
parent_.checkProtocolConstraintViolation();
if (parent_.sendPendingFramesAndHandleError()) {
// Intended to check through coverage that this error case is tested
return;
}
}
}

Expand All @@ -271,10 +264,10 @@ void ConnectionImpl::StreamImpl::encodeMetadata(const MetadataMapVector& metadat
for (uint8_t flags : metadata_encoder.payloadFrameFlagBytes()) {
submitMetadata(flags);
}
auto status = parent_.sendPendingFrames();
// See comment in the `encodeHeadersBase()` method about this RELEASE_ASSERT.
RELEASE_ASSERT(status.ok(), "sendPendingFrames() failure in non dispatching context");
parent_.checkProtocolConstraintViolation();
if (parent_.sendPendingFramesAndHandleError()) {
// Intended to check through coverage that this error case is tested
return;
}
}

void ConnectionImpl::StreamImpl::readDisable(bool disable) {
Expand All @@ -289,10 +282,10 @@ void ConnectionImpl::StreamImpl::readDisable(bool disable) {
if (!buffersOverrun()) {
nghttp2_session_consume(parent_.session_, stream_id_, unconsumed_bytes_);
unconsumed_bytes_ = 0;
auto status = parent_.sendPendingFrames();
// See comment in the `encodeHeadersBase()` method about this RELEASE_ASSERT.
RELEASE_ASSERT(status.ok(), "sendPendingFrames() failure in non dispatching context");
parent_.checkProtocolConstraintViolation();
if (parent_.sendPendingFramesAndHandleError()) {
// Intended to check through coverage that this error case is tested
return;
}
}
}
}
Expand Down Expand Up @@ -418,7 +411,7 @@ ssize_t ConnectionImpl::StreamImpl::onDataSourceRead(uint64_t length, uint32_t*
}
}

Status ConnectionImpl::StreamImpl::onDataSourceSend(const uint8_t* framehd, size_t length) {
void ConnectionImpl::StreamImpl::onDataSourceSend(const uint8_t* framehd, size_t length) {
// In this callback we are writing out a raw DATA frame without copying. nghttp2 assumes that we
// "just know" that the frame header is 9 bytes.
// https://nghttp2.org/documentation/types.html#c.nghttp2_send_data_callback
Expand All @@ -427,18 +420,16 @@ Status ConnectionImpl::StreamImpl::onDataSourceSend(const uint8_t* framehd, size
parent_.protocol_constraints_.incrementOutboundDataFrameCount();

Buffer::OwnedImpl output;
auto status = parent_.addOutboundFrameFragment(output, framehd, FRAME_HEADER_SIZE);
if (!status.ok()) {
parent_.addOutboundFrameFragment(output, framehd, FRAME_HEADER_SIZE);
if (!parent_.protocol_constraints_.checkOutboundFrameLimits().ok()) {
ENVOY_CONN_LOG(debug, "error sending data frame: Too many frames in the outbound queue",
parent_.connection_);
setDetails(Http2ResponseCodeDetails::get().outbound_frame_flood);
return status;
}

parent_.stats_.pending_send_bytes_.sub(length);
output.move(pending_send_data_, length);
parent_.connection_.write(output, false);
return status;
}

void ConnectionImpl::ClientStreamImpl::submitHeaders(const std::vector<nghttp2_nv>& final_headers,
Expand Down Expand Up @@ -474,10 +465,10 @@ void ConnectionImpl::StreamImpl::onPendingFlushTimer() {
// This will emit a reset frame for this stream and close the stream locally. No reset callbacks
// will be run because higher layers think the stream is already finished.
resetStreamWorker(StreamResetReason::LocalReset);
auto status = parent_.sendPendingFrames();
// See comment in the `encodeHeadersBase()` method about this RELEASE_ASSERT.
RELEASE_ASSERT(status.ok(), "sendPendingFrames() failure in non dispatching context");
parent_.checkProtocolConstraintViolation();
if (parent_.sendPendingFramesAndHandleError()) {
// Intended to check through coverage that this error case is tested
return;
}
}

void ConnectionImpl::StreamImpl::encodeData(Buffer::Instance& data, bool end_stream) {
Expand All @@ -501,11 +492,10 @@ void ConnectionImpl::StreamImpl::encodeDataHelper(Buffer::Instance& data, bool e
data_deferred_ = false;
}

auto status = parent_.sendPendingFrames();
// See comment in the `encodeHeadersBase()` method about this RELEASE_ASSERT.
RELEASE_ASSERT(status.ok(), "sendPendingFrames() failure in non dispatching context");
parent_.checkProtocolConstraintViolation();

if (parent_.sendPendingFramesAndHandleError()) {
// Intended to check through coverage that this error case is tested
return;
}
if (local_end_stream_ && pending_send_data_.length() > 0) {
createPendingFlushTimer();
}
Expand All @@ -529,10 +519,10 @@ void ConnectionImpl::StreamImpl::resetStream(StreamResetReason reason) {
// 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.
auto status = parent_.sendPendingFrames();
// See comment in the `encodeHeadersBase()` method about this RELEASE_ASSERT.
RELEASE_ASSERT(status.ok(), "sendPendingFrames() failure in non dispatching context");
parent_.checkProtocolConstraintViolation();
if (parent_.sendPendingFramesAndHandleError()) {
// Intended to check through coverage that this error case is tested
return;
}
}

void ConnectionImpl::StreamImpl::resetStreamWorker(StreamResetReason reason) {
Expand Down Expand Up @@ -611,11 +601,10 @@ void ConnectionImpl::sendKeepalive() {
int rc = nghttp2_submit_ping(session_, 0 /*flags*/, reinterpret_cast<uint8_t*>(&ms_since_epoch));
ASSERT(rc == 0);

auto status = sendPendingFrames();
// See comment in the `encodeHeadersBase()` method about this RELEASE_ASSERT.
RELEASE_ASSERT(status.ok(), "sendPendingFrames() failure in non dispatching context");
checkProtocolConstraintViolation();

if (sendPendingFramesAndHandleError()) {
// Intended to check through coverage that this error case is tested
return;
}
keepalive_timeout_timer_->enableTimer(keepalive_timeout_);
}
void ConnectionImpl::onKeepaliveResponse() {
Expand Down Expand Up @@ -707,20 +696,20 @@ void ConnectionImpl::goAway() {
NGHTTP2_NO_ERROR, nullptr, 0);
ASSERT(rc == 0);

auto status = sendPendingFrames();
// See comment in the `encodeHeadersBase()` method about this RELEASE_ASSERT.
RELEASE_ASSERT(status.ok(), "sendPendingFrames() failure in non dispatching context");
checkProtocolConstraintViolation();
if (sendPendingFramesAndHandleError()) {
// Intended to check through coverage that this error case is tested
return;
}
}

void ConnectionImpl::shutdownNotice() {
int rc = nghttp2_submit_shutdown_notice(session_);
ASSERT(rc == 0);

auto status = sendPendingFrames();
// See comment in the `encodeHeadersBase()` method about this RELEASE_ASSERT.
RELEASE_ASSERT(status.ok(), "sendPendingFrames() failure in non dispatching context");
checkProtocolConstraintViolation();
if (sendPendingFramesAndHandleError()) {
// Intended to check through coverage that this error case is tested
return;
}
}

Status ConnectionImpl::onBeforeFrameReceived(const nghttp2_frame_hd* hd) {
Expand Down Expand Up @@ -938,31 +927,21 @@ int ConnectionImpl::onBeforeFrameSend(const nghttp2_frame* frame) {
return 0;
}

Status ConnectionImpl::addOutboundFrameFragment(Buffer::OwnedImpl& output, const uint8_t* data,
size_t length) {
void ConnectionImpl::addOutboundFrameFragment(Buffer::OwnedImpl& output, const uint8_t* data,
size_t length) {
// Reset the outbound frame type (set in the onBeforeFrameSend callback) since the
// onBeforeFrameSend callback is not called for DATA frames.
bool is_outbound_flood_monitored_control_frame = false;
std::swap(is_outbound_flood_monitored_control_frame, is_outbound_flood_monitored_control_frame_);
auto status_or_releasor = trackOutboundFrames(is_outbound_flood_monitored_control_frame);
if (!status_or_releasor.ok()) {
return status_or_releasor.status();
}

auto releasor = trackOutboundFrames(is_outbound_flood_monitored_control_frame);
output.add(data, length);
output.addDrainTracker(status_or_releasor.value());
return okStatus();
output.addDrainTracker(releasor);
}

StatusOr<ssize_t> ConnectionImpl::onSend(const uint8_t* data, size_t length) {
ssize_t ConnectionImpl::onSend(const uint8_t* data, size_t length) {
ENVOY_CONN_LOG(trace, "send data: bytes={}", connection_, length);
Buffer::OwnedImpl buffer;
auto status = addOutboundFrameFragment(buffer, data, length);
if (!status.ok()) {
ENVOY_CONN_LOG(debug, "error sending frame: Too many frames in the outbound queue.",
connection_);
return status;
}
addOutboundFrameFragment(buffer, data, length);

// While the buffer is transient the fragment it contains will be moved into the
// write_buffer_ of the underlying connection_ by the write method below.
Expand Down Expand Up @@ -1100,15 +1079,6 @@ Status ConnectionImpl::sendPendingFrames() {
const int rc = nghttp2_session_send(session_);
if (rc != 0) {
ASSERT(rc == NGHTTP2_ERR_CALLBACK_FAILURE);

if (!nghttp2_callback_status_.ok()) {
return nghttp2_callback_status_;
}

// Protocol constrain violations should set the nghttp2_callback_status_ error, and return at
// the statement above.
ASSERT(protocol_constraints_.status().ok());

return codecProtocolError(nghttp2_strerror(rc));
}

Expand All @@ -1133,7 +1103,23 @@ Status ConnectionImpl::sendPendingFrames() {
}
RETURN_IF_ERROR(sendPendingFrames());
}
return okStatus();

// After all pending frames have been written into the outbound buffer check if any of
// protocol constraints had been violated.
Status status = protocol_constraints_.checkOutboundFrameLimits();
if (!status.ok()) {
ENVOY_CONN_LOG(debug, "error sending frames: Too many frames in the outbound queue.",
connection_);
}
return status;
}

bool ConnectionImpl::sendPendingFramesAndHandleError() {
if (!sendPendingFrames().ok()) {
scheduleProtocolConstraintViolationCallback();
return true;
}
return false;
}

void ConnectionImpl::sendSettings(
Expand Down Expand Up @@ -1225,23 +1211,16 @@ ConnectionImpl::Http2Callbacks::Http2Callbacks() {
nghttp2_session_callbacks_set_send_callback(
callbacks_,
[](nghttp2_session*, const uint8_t* data, size_t length, int, void* user_data) -> ssize_t {
auto status_or_len = static_cast<ConnectionImpl*>(user_data)->onSend(data, length);
if (status_or_len.ok()) {
return status_or_len.value();
}
auto status = status_or_len.status();
return static_cast<ConnectionImpl*>(user_data)->setAndCheckNghttp2CallbackStatus(
std::move(status));
return static_cast<ConnectionImpl*>(user_data)->onSend(data, length);
});

nghttp2_session_callbacks_set_send_data_callback(
callbacks_,
[](nghttp2_session*, nghttp2_frame* frame, const uint8_t* framehd, size_t length,
nghttp2_data_source* source, void*) -> int {
ASSERT(frame->data.padlen == 0);
auto status = static_cast<StreamImpl*>(source->ptr)->onDataSourceSend(framehd, length);
return static_cast<StreamImpl*>(source->ptr)
->parent_.setAndCheckNghttp2CallbackStatus(std::move(status));
static_cast<StreamImpl*>(source->ptr)->onDataSourceSend(framehd, length);
return 0;
});

nghttp2_session_callbacks_set_on_begin_headers_callback(
Expand Down Expand Up @@ -1518,20 +1497,10 @@ Status ServerConnectionImpl::trackInboundFrames(const nghttp2_frame_hd* hd,
return result;
}

StatusOr<ProtocolConstraints::ReleasorProc>
ProtocolConstraints::ReleasorProc
ServerConnectionImpl::trackOutboundFrames(bool is_outbound_flood_monitored_control_frame) {
auto releasor =
protocol_constraints_.incrementOutboundFrameCount(is_outbound_flood_monitored_control_frame);
if (dispatching_downstream_data_ && !protocol_constraints_.checkOutboundFrameLimits().ok()) {
return protocol_constraints_.status();
}
return releasor;
}

void ServerConnectionImpl::checkProtocolConstraintViolation() {
if (!protocol_constraints_.checkOutboundFrameLimits().ok()) {
scheduleProtocolConstraintViolationCallback();
}
return protocol_constraints_.incrementOutboundFrameCount(
is_outbound_flood_monitored_control_frame);
}

Http::Status ServerConnectionImpl::dispatch(Buffer::Instance& data) {
Expand All @@ -1543,12 +1512,6 @@ Http::Status ServerConnectionImpl::dispatch(Buffer::Instance& data) {
}

Http::Status ServerConnectionImpl::innerDispatch(Buffer::Instance& data) {
ASSERT(!dispatching_downstream_data_);
dispatching_downstream_data_ = true;

// Make sure the dispatching_downstream_data_ is set to false when innerDispatch ends.
Cleanup cleanup([this]() { dispatching_downstream_data_ = false; });

// Make sure downstream outbound queue was not flooded by the upstream frames.
RETURN_IF_ERROR(protocol_constraints_.checkOutboundFrameLimits());
return ConnectionImpl::innerDispatch(data);
Expand Down
Loading

0 comments on commit 11289b9

Please sign in to comment.