http: global connection manager per-stream idle timeouts. by htuch · Pull Request #3879 · envoyproxy/envoy

htuch · 2018-07-17T21:52:55Z

This is a followup to #3841, where we introduce HCM-wide stream idle timeouts. This has two effects:

We can now timeout immediately after stream creation, potentially before receiving request
headers and routing.
A default timeout can be configured across all routes. This is overridable on a per-route basis.

The default and overriding semantics are explained in the docs. Also added as a bonus some docs
about how timeouts interact more generally in Envoy.

Fixes #3853.

Risk Level: Low. While there is some change to the per-route vs. HCM wide semantics for stream idle
timeouts, it's not anticipated this feature is in common use yet (it's only a couple of days since
landing), and the caveats in #3841 with the new 5 minute default timeout should already apply.
Testing: Unit/integration tests added.

Signed-off-by: Harvey Tuch htuch@google.com

This is a followup to envoyproxy#3841, where we introduce HCM-wide stream idle timeouts. This has two effects: 1. We can now timeout immediately after stream creation, potentially before receiving request headers and routing. 2. A default timeout can be configured across all routes. This is overridable on a per-route basis. The default and overriding semantics are explained in the docs. Also added as a bonus some docs about how timeouts interact more generally in Envoy. Fixes envoyproxy#3853. Risk Level: Low. While there is some change to the per-route vs. HCM wide semantics for stream idle timeouts, it's not anticipated this feature is in common use yet (it's only a couple of days since landing), and the caveats in envoyproxy#3841 with the new 5 minute default timeout should already apply. Testing: Unit/integration tests added. Signed-off-by: Harvey Tuch <htuch@google.com>

alyssawilk

Thanks for this one - it's great to see our HTTP idle timeouts getting better and better!

alyssawilk · 2018-07-18T13:10:44Z

api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto

  google.protobuf.Duration idle_timeout = 11 [(gogoproto.stdduration) = true];

+  // The stream idle timeout for connections managed by the connection manager.
+  // If not specified, this defaults to 5 minutes. The default value was select


-> selected

alyssawilk · 2018-07-18T13:10:55Z

api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto

+  // If not specified, this defaults to 5 minutes. The default value was select
+  // so as not to interfere with any smaller configured timeouts that may have
+  // existed in configurations prior to the introduction of this feature, while
+  // introducing robustness to TCP connections that terminate without FIN.


alyssawilk · 2018-07-18T13:13:59Z

api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto

+  // headers, the :ref:`stream_idle_timeout
+  // <envoy_api_field_config.filter.network.http_connection_manager.v2.HttpConnectionManager.stream_idle_timeout>`
+  // applies. Each time an encode/decode event for headers or data is processed
+  // for the stream, the timer will be reset. If the timeout fires, the stream


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.

I'll add something on the potential mismatch between wire traffic and observed events.

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.

alyssawilk · 2018-07-18T13:14:20Z

docs/root/intro/arch_overview/http_connection_management.rst

+Timeouts
+--------
+
+Various configurable timeouts apply to an HTTP connection and its constituent streams:


Thank you for this!

alyssawilk · 2018-07-18T13:20:22Z

source/common/http/conn_manager_impl.cc

+      if (idle_timer_ == nullptr) {
+        idle_timer_ = connection_manager_.read_callbacks_->connection().dispatcher().createTimer(
+            [this]() -> void { onIdleTimeout(); });
+      }


Do we have a test for setting a general timeout and overriding with 0? It looks to me like we'd set idle_timeout_ms_ = 0, then resetIdleTimer and arm it for 'immediately'. Maybe resetIdleTimer should do the "no idle timeout set" check?

Yeah, good catch, we also need to change the documented per-route override semantics for this to make it possible to disable at the route level.

Signed-off-by: Harvey Tuch <htuch@google.com>

alyssawilk

LGTM though if @mattklein123 has time to do a pass today that'd be great.

mattklein123 · 2018-07-18T22:41:02Z

I'm traveling today, will look tonight or tomorrow morning.

…

On Wed, Jul 18, 2018, 11:13 AM alyssawilk ***@***.***> wrote: ***@***.**** approved this pull request. LGTM though if @mattklein123 <https://github.com/mattklein123> has time to do a pass today that'd be great. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3879 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGA17Aas7IfHsk1RQbInru6vxqCfmdioks5uH3rGgaJpZM4VTnz8> .

mattklein123

LGTM, one small comment request, also needs a master merge. Thank you for adding this!

mattklein123 · 2018-07-19T14:40:52Z

api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto

+  // headers, the :ref:`stream_idle_timeout
+  // <envoy_api_field_config.filter.network.http_connection_manager.v2.HttpConnectionManager.stream_idle_timeout>`
+  // applies. Each time an encode/decode event for headers or data is processed
+  // for the stream, the timer will be reset. If the timeout fires, the stream


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.

mattklein123 · 2018-07-19T14:41:53Z

docs/root/intro/arch_overview/http_connection_management.rst

+Timeouts
+--------
+
+Various configurable timeouts apply to an HTTP connection and its constituent streams:


mattklein123 · 2018-07-19T14:46:10Z

source/common/http/conn_manager_impl.cc

+          idle_timer_ = connection_manager_.read_callbacks_->connection().dispatcher().createTimer(
+              [this]() -> void { onIdleTimeout(); });
+        }
+      } else if (idle_timer_ != nullptr) {


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).

Signed-off-by: Harvey Tuch <htuch@google.com>

htuch assigned mattklein123 and alyssawilk Jul 17, 2018

htuch requested review from alyssawilk and mattklein123 July 17, 2018 21:53

alyssawilk reviewed Jul 18, 2018

View reviewed changes

htuch added 4 commits July 18, 2018 12:28

Merge remote-tracking branch 'upstream/master' into global-idle-timeout

78dc7fe

Review feedback.

a308189

Signed-off-by: Harvey Tuch <htuch@google.com>

Fix TSAN race with set_allow_unexpected_disconnects.

1407f36

Signed-off-by: Harvey Tuch <htuch@google.com>

More test fixes.

f0736fc

Signed-off-by: Harvey Tuch <htuch@google.com>

alyssawilk previously approved these changes Jul 18, 2018

View reviewed changes

mattklein123 reviewed Jul 19, 2018

View reviewed changes

htuch added 2 commits July 19, 2018 14:11

Merge remote-tracking branch 'upstream/master' into global-idle-timeout

efdaff1

Additional comments.

3f039a5

Signed-off-by: Harvey Tuch <htuch@google.com>

htuch dismissed alyssawilk’s stale review via 3f039a5 July 19, 2018 18:24

Typo.

528b32b

Signed-off-by: Harvey Tuch <htuch@google.com>

mattklein123 approved these changes Jul 20, 2018

View reviewed changes

htuch merged commit 672083c into envoyproxy:master Jul 23, 2018

htuch deleted the global-idle-timeout branch July 23, 2018 17:27

htuch mentioned this pull request Sep 4, 2018

http2.tx_reset increments even when stream is not disconnected with stream_idle_timeout #4316

Closed

dankirkpatrick mentioned this pull request Feb 20, 2019

503 upstream connect error or disconnect/reset before headers istio/istio#9906

Closed

Conversation

htuch commented Jul 17, 2018

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch Jul 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 commented Jul 18, 2018 via email

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

htuch Jul 18, 2018 •

edited

Loading