-
Notifications
You must be signed in to change notification settings - Fork 5.3k
http: global connection manager per-stream idle timeouts. #3879
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
Changes from all commits
9ecc481
78dc7fe
a308189
1407f36
f0736fc
efdaff1
3f039a5
528b32b
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 |
|---|---|---|
|
|
@@ -42,3 +42,27 @@ table <arch_overview_http_routing>`. The route table can be specified in one of | |
|
|
||
| * Statically. | ||
| * Dynamically via the :ref:`RDS API <config_http_conn_man_rds>`. | ||
|
|
||
| Timeouts | ||
| -------- | ||
|
|
||
| Various configurable timeouts apply to an HTTP connection and its constituent streams: | ||
|
Contributor
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. Thank you for this!
Member
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. +1 |
||
|
|
||
| * Connection-level :ref:`idle timeout | ||
| <envoy_api_field_config.filter.network.http_connection_manager.v2.HttpConnectionManager.idle_timeout>`: | ||
| this applies to the idle period where no streams are active. | ||
| * Connection-level :ref:`drain timeout | ||
| <envoy_api_field_config.filter.network.http_connection_manager.v2.HttpConnectionManager.drain_timeout>`: | ||
| this spans between an Envoy originated GOAWAY and connection termination. | ||
| * Stream-level idle timeout: this applies to each individual stream. It may be configured at both | ||
| the :ref:`connection manager | ||
| <envoy_api_field_config.filter.network.http_connection_manager.v2.HttpConnectionManager.stream_idle_timeout>` | ||
| and :ref:`per-route <envoy_api_field_route.RouteAction.idle_timeout>` granularity. | ||
| Header/data/trailer events on the stream reset the idle timeout. | ||
| * Stream-level :ref:`per-route upstream timeout <envoy_api_field_route.RouteAction.timeout>`: this | ||
| applies to the upstream response, i.e. a maximum bound on the time from the end of the downstream | ||
| request until the end of the upstream response. This may also be specified at the :ref:`per-retry | ||
| <envoy_api_field_route.RouteAction.RetryPolicy.per_try_timeout>` granularity. | ||
| * Stream-level :ref:`per-route gRPC max timeout | ||
| <envoy_api_field_route.RouteAction.max_grpc_timeout>`: this bounds the upstream timeout and allows | ||
| the timeout to be overriden via the *grpc-timeout* request header. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -369,6 +369,13 @@ ConnectionManagerImpl::ActiveStream::ActiveStream(ConnectionManagerImpl& connect | |
| // prevents surprises for logging code in edge cases. | ||
| request_info_.setDownstreamRemoteAddress( | ||
| connection_manager_.read_callbacks_->connection().remoteAddress()); | ||
|
|
||
| if (connection_manager_.config_.streamIdleTimeout().count()) { | ||
| idle_timeout_ms_ = connection_manager_.config_.streamIdleTimeout(); | ||
| idle_timer_ = connection_manager_.read_callbacks_->connection().dispatcher().createTimer( | ||
| [this]() -> void { onIdleTimeout(); }); | ||
| resetIdleTimer(); | ||
| } | ||
| } | ||
|
|
||
| ConnectionManagerImpl::ActiveStream::~ActiveStream() { | ||
|
|
@@ -605,9 +612,18 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, | |
| const Router::RouteEntry* route_entry = cached_route_.value()->routeEntry(); | ||
| if (route_entry != nullptr && route_entry->idleTimeout()) { | ||
| idle_timeout_ms_ = route_entry->idleTimeout().value(); | ||
| idle_timer_ = connection_manager_.read_callbacks_->connection().dispatcher().createTimer( | ||
| [this]() -> void { onIdleTimeout(); }); | ||
| resetIdleTimer(); | ||
| if (idle_timeout_ms_.count()) { | ||
| // If we have a route-level idle timeout but no global stream idle timeout, create a timer. | ||
| if (idle_timer_ == nullptr) { | ||
| idle_timer_ = connection_manager_.read_callbacks_->connection().dispatcher().createTimer( | ||
| [this]() -> void { onIdleTimeout(); }); | ||
| } | ||
| } else if (idle_timer_ != nullptr) { | ||
|
Member
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. Do you mind adding a few comments here since this logic block is so important and might not be immediately obvious to the reader? (Basically describing in code that the route told us to disable the idle timeout so we are killing the connection level one if it exists). |
||
| // If we had a global stream idle timeout but the route-level idle timeout is set to zero | ||
| // (to override), we disable the idle timer. | ||
| idle_timer_->disableTimer(); | ||
| idle_timer_ = nullptr; | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -617,6 +633,9 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, | |
| } | ||
|
|
||
| decodeHeaders(nullptr, *request_headers_, end_stream); | ||
|
|
||
| // Reset it here for both global and overriden cases. | ||
| resetIdleTimer(); | ||
| } | ||
|
|
||
| void ConnectionManagerImpl::ActiveStream::traceRequest() { | ||
|
|
||
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.
One thing of note - I don't believe there's any encode/decode events between initial receipt of headers and completed headers. While I've not seen giant request headers in prod (due to restricted size limits), I've seen users send 100k+ response headers, so I assume there's folks out there who do large headers on both ends. If headers are broken into many frames and shipped over a small connection, I think the connection could still idle out, which is unfortunate since it is making forward progress. I think documenting this may cause more confusion than it's worth but at least calling it out here.
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.
I'll add something on the potential mismatch between wire traffic and observed events.
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.
Yeah good point @alyssawilk. I agree we probably don't need to worry about that now but good to keep in the back of our minds.