-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
connection: propagate I/O errors to network filters. #13941
Changes from 1 commit
232c0ff
b62641f
8f58ce7
848af7d
ae3fc5b
4593733
8c5a5da
c46d41e
6e04b67
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 |
---|---|---|
|
@@ -25,6 +25,10 @@ namespace { | |
|
||
constexpr absl::string_view kTransportSocketConnectTimeoutTerminationDetails = | ||
"transport socket timeout was reached"; | ||
constexpr absl::string_view kDownstreamConnectionTerminationDetails = | ||
"downstream connection was terminated"; | ||
constexpr absl::string_view kUpstreamConnectionTerminationDetails = | ||
"upstream connection was terminated"; | ||
|
||
} | ||
|
||
|
@@ -588,6 +592,20 @@ void ConnectionImpl::onReadReady() { | |
result.action_ = PostIoAction::Close; | ||
} | ||
|
||
if (result.io_error_.has_value()) { | ||
ASSERT(result.action_ == PostIoAction::Close); | ||
if (dynamic_cast<ServerConnectionImpl*>(this)) { | ||
stream_info_.setConnectionTerminationDetails(kDownstreamConnectionTerminationDetails); | ||
stream_info_.setResponseFlag(StreamInfo::ResponseFlag::DownstreamConnectionTermination); | ||
} else { | ||
stream_info_.setUpstreamTransportFailureReason(kUpstreamConnectionTerminationDetails); | ||
stream_info_.setConnectionTerminationDetails(kUpstreamConnectionTerminationDetails); | ||
stream_info_.setResponseFlag(StreamInfo::ResponseFlag::UpstreamConnectionTermination); | ||
} | ||
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. Those values are set on the upstream connection's stream info, and they are never propagated to downstream connection's stream info or access logs. I'm not sure what's the best way to solve that. 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. Could the high level request object query the connection as part of the termination process to determine if termination was graceful or abrupt? |
||
// Force "end_stream" so that filters can process this error. | ||
result.end_stream_read_ = true; | ||
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 usually associate end_stream_read_ with graceful termination. Is it appropriate to set this? Do we need alternate signaling to notify filters about abrupt terminations via either additional arguments to push pipeline or a new virtual method which is called on abrupt termination. 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 agree; I think of end_stream as always a graceful operation. I think a different signal for error is more appropriate. 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. The issue is that there is no different signal right now, and if we don't set this, then network filters won't be executed at all. Note that this PR is meant to be a temporary fix until we have a proper solution for #13940, but that's going to require a lot more changes and time. 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. This seems like a massive hack. How much effort would it be to implement the real fix? 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. That depends on what do you consider a real fix. I'm not too familiar with that part of the codebase, but if we want to propagate upstream connection events to network filters, then I imagine it's quite a lot of changes, so I think it would be better if one of maintainers could work on this. 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 don't know what to suggest. It would be good to figure out how to do a catch-all cleanup of WASM resources on abnormal connection termination. Could WASM detect connection termination based on an L4 filter that does the relevant signaling on destruction? 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. FWIW, we have a workaround in #13836 that works for Wasm. |
||
} | ||
|
||
read_end_stream_ |= result.end_stream_read_; | ||
if (result.bytes_processed_ != 0 || result.end_stream_read_ || | ||
(latched_dispatch_buffered_data && read_buffer_.length() > 0)) { | ||
|
@@ -651,6 +669,18 @@ void ConnectionImpl::onWriteReady() { | |
uint64_t new_buffer_size = write_buffer_->length(); | ||
updateWriteBufferStats(result.bytes_processed_, new_buffer_size); | ||
|
||
if (result.io_error_.has_value()) { | ||
ASSERT(result.action_ == PostIoAction::Close); | ||
if (dynamic_cast<ServerConnectionImpl*>(this)) { | ||
stream_info_.setConnectionTerminationDetails(kDownstreamConnectionTerminationDetails); | ||
stream_info_.setResponseFlag(StreamInfo::ResponseFlag::DownstreamConnectionTermination); | ||
} else { | ||
stream_info_.setUpstreamTransportFailureReason(kUpstreamConnectionTerminationDetails); | ||
stream_info_.setConnectionTerminationDetails(kUpstreamConnectionTerminationDetails); | ||
stream_info_.setResponseFlag(StreamInfo::ResponseFlag::UpstreamConnectionTermination); | ||
} | ||
} | ||
|
||
// NOTE: If the delayed_close_timer_ is set, it must only trigger after a delayed_close_timeout_ | ||
// period of inactivity from the last write event. Therefore, the timer must be reset to its | ||
// original timeout value unless the socket is going to be closed as a result of the doWrite(). | ||
|
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.
nit: delegate stats update to virtual method so you can avoid the dynamic cast.