Skip to content
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.

Sign up for GitHub

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

http2: Remove RELEASE_ASSERTs in sendPendingFrames() error handling #13546

Merged
merged 5 commits into from
Oct 16, 2020
Merged

http2: Remove RELEASE_ASSERTs in sendPendingFrames() error handling #13546

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9076165
Remove RELEASE_ASSERTs in sendPendingFrames() error handling
yanavlasov Oct 11, 2020
94e5fe0
Address comments
yanavlasov Oct 13, 2020
303e430
Merge branch 'master' into remove-release-assert
yanavlasov Oct 15, 2020
b6004ee
Merge branch 'master' into remove-release-assert
yanavlasov Oct 15, 2020
fe520ad
Merge branch 'master' into remove-release-assert
yanavlasov Oct 16, 2020
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
148 changes: 40 additions & 108 deletions source/common/http/http2/codec_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,17 +166,7 @@ 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();
parent_.sendPendingFramesAndHandleError();
}

void ConnectionImpl::ClientStreamImpl::encodeHeaders(const RequestHeaderMap& headers,
Expand Down Expand Up @@ -244,10 +234,7 @@ 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();
parent_.sendPendingFramesAndHandleError();
}
}

Expand All @@ -260,10 +247,7 @@ 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();
parent_.sendPendingFramesAndHandleError();
}

void ConnectionImpl::StreamImpl::readDisable(bool disable) {
Expand All @@ -278,10 +262,7 @@ 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();
parent_.sendPendingFramesAndHandleError();
}
}
}
Expand Down Expand Up @@ -407,7 +388,7 @@ ssize_t ConnectionImpl::StreamImpl::onDataSourceRead(uint64_t length, uint32_t*
}
}

Status ConnectionImpl::StreamImpl::onDataSourceSend(const uint8_t* framehd, size_t length) {
int 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 @@ -416,18 +397,17 @@ 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);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should there be an error return for this case, or should this be void and the call site always return 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The error will be handled in the sendPendingFrames(). I've made this function void.

return status;
}

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

void ConnectionImpl::ClientStreamImpl::submitHeaders(const std::vector<nghttp2_nv>& final_headers,
Expand Down Expand Up @@ -463,10 +443,7 @@ 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();
parent_.sendPendingFramesAndHandleError();
}

void ConnectionImpl::StreamImpl::encodeData(Buffer::Instance& data, bool end_stream) {
Expand All @@ -490,10 +467,7 @@ 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();
parent_.sendPendingFramesAndHandleError();
Copy link
Contributor

Choose a reason for hiding this comment

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

question before I review farther, does it make sense to continue processing if an error handler has been scheduled? Most of these are the last function called.

Also, where we sendPendingFramesAndHandleError, are we confident we have tests for error handling at each call site? Would it make sense to always
if (sendPendingFramesAndHandleError()) {
return;
}
so we can coverage-verify the error handling is tested? Or given these replace release asserts are we asserting they can't happen and mainly hoping they don't cause issues?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I think in some cases additional processing, like scheduling timers is not needed. I have changed these paths.
I have added in last few PRs 16 new integration test cases for all invocations of the sendPendingFrames(). To the best of my analysis all call sites in all possible contexts are covered. It does not mean that I'm right of course. So I've added the if clause as you have suggested and will check the coverage output.


if (local_end_stream_ && pending_send_data_.length() > 0) {
createPendingFlushTimer();
Expand All @@ -518,10 +492,7 @@ 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();
parent_.sendPendingFramesAndHandleError();
}

void ConnectionImpl::StreamImpl::resetStreamWorker(StreamResetReason reason) {
Expand Down Expand Up @@ -602,11 +573,7 @@ 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();

sendPendingFramesAndHandleError();
keepalive_timeout_timer_->enableTimer(keepalive_timeout_);
}
void ConnectionImpl::onKeepaliveResponse() {
Expand Down Expand Up @@ -698,20 +665,14 @@ 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();
sendPendingFramesAndHandleError();
}

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();
sendPendingFramesAndHandleError();
}

Status ConnectionImpl::onBeforeFrameReceived(const nghttp2_frame_hd* hd) {
Expand Down Expand Up @@ -929,31 +890,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 @@ -1087,15 +1038,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 @@ -1120,7 +1062,21 @@ 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;
}

void ConnectionImpl::sendPendingFramesAndHandleError() {
if (!sendPendingFrames().ok()) {
scheduleProtocolConstraintViolationCallback();
}
}

void ConnectionImpl::sendSettings(
Expand Down Expand Up @@ -1212,23 +1168,15 @@ 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));
return static_cast<StreamImpl*>(source->ptr)->onDataSourceSend(framehd, length);
});

nghttp2_session_callbacks_set_on_begin_headers_callback(
Expand Down Expand Up @@ -1505,20 +1453,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 @@ -1530,12 +1468,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