response decoder: add new interfaces for life time tracking of response decoder by danzh2010 · Pull Request #41048 · envoyproxy/envoy

danzh2010 · 2025-09-11T14:39:01Z

Commit Message: introduce ResponseDecoderHandle interface via which ResponseDecoder object can be obtained with guarantee that the object is still alive. Also added a new interface getResponseDecoderHandle() to ResponseDecoder and a new base implementation of ResponseDecoder ResponseDecoderImplBase which implements these new interfaces. Existing subclasses of ResponseDecoder are changed to inherit from ResponseDecoderImplBase.

Additional Description: also make EnvoyQuicClientStream and ResponseDecoderWrapper to use the handle instead of directly accessing the passed-in response decoder, guarded by envoy.reloadable_features.use_response_decoder_handle. And when the decoder is dead, log error or abort based on envoy.reloadable_features.abort_when_accessing_dead_decoder.

We should gradually transit from using raw pointer or reference of ResponseDecoder to using the handle interface everywhere else. This PR makes the transition in the 2 classes which we are observing life time issues today.

Risk Level: low
Testing: existing tests passed
Docs Changes: N/A
Release Notes: Y
Platform Specific Features: N/A
Runtime guard: envoy.reloadable_features.abort_when_accessing_dead_decoder(default false) and envoy.reloadable_features.use_response_decoder_handle
Issue: #41060

Signed-off-by: Dan Zhang <danzh@google.com>

repokitteh-read-only · 2025-09-11T14:39:10Z

CC @envoyproxy/runtime-guard-changes: FYI only for changes made to (source/common/runtime/runtime_features.cc).

🐱

Caused by: #41048 was opened by danzh2010.

see: more, trace.

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 · 2025-09-11T15:54:05Z

/assign @RyanTheOptimist @abeyad

abeyad

Thanks so much for working on this Dan! I really like this fix.

abeyad · 2025-09-11T16:14:33Z

envoy/router/router.h

 * and/or router components
 */
-class UpstreamToDownstream : public Http::ResponseDecoder, public Http::StreamCallbacks {
+class UpstreamToDownstream : public Http::ResponseDecoderImplBase, public Http::StreamCallbacks {


why do we need to change the inheritance to ResponseDecoderImplBase?

I think this is so we get an implementation of the new getHandle method?

ResponseDecoderImplBase implements the new interface getResponseDecoderHandle()

sorry this was a comment i had left in before i read the full PR but forgot to remove :/ sorry about that!

abeyad · 2025-09-11T17:11:43Z

source/common/http/codec_wrappers.h

  // ResponseDecoder
  void decode1xxHeaders(ResponseHeaderMapPtr&& headers) override {
-    inner_.decode1xxHeaders(std::move(headers));
+    if (Http::ResponseDecoder* inner = getInnerDecoder()) {


nit: I leave it up to you if you think it helps readability, but you could refactor this into a function:

void runWithInnerDecoder(std::function<>(Http::ResponseDecoder*) callback) { if (Http::ResponseDecoder* inner = getInnerDecoder()) { callback(inner); } else { onInnerDecoderDead(); } }

then all of these functions would look like:

runWithInnerDecoder([headers](Http::ResponseDecoder* inner) -> { inner->decode1xxHeaders(std::move(headers)); });

stack trace with lambda function is hard to read. I'd prefer inlining the if else block.

abeyad · 2025-09-11T17:18:16Z

envoy/http/codec.h

+   * @return A handle to the response decoder. Caller can check the response decoder's liveness via
+   * the handle.
+   */
+  virtual ResponseDecoderHandlePtr getResponseDecoderHandle() PURE;


I don't think this can be a pure virtual function, b/c there are several classes (including within Google, several classes) that inherit from ResponseDecoder, so unless we add an override in all those functions, we'd fail to compile, no?

Should this perhaps be named createResponseDecoderHandle() to make it clear that it is creating a new object (as suggested by the return type of std::unique_ptr)?

Ali: I think this will be a breaking API change, but we get those all the time in Envoy which makes import so much fun. When this gets imported, can we change those classes to extend the new base class?

The ResponseDecoderImplBase implements it. So all the subclasses of ResponseDecoder are now inheriting from ResponseDecoderImplBase

I just meant that there are subclasses of ResponseDecoder in Google specific code that will need to be changed. As Ryan said, I guess the import will just need to handle it.

renamed to createResponseDecoderHandle

abeyad · 2025-09-11T17:22:13Z

source/common/quic/envoy_quic_client_stream.cc

    // In case the status is invalid or missing, the response_decoder_.decodeHeaders() will fail the
    // request
-    response_decoder_->decodeHeaders(std::move(headers), fin);
+    if (Http::ResponseDecoder* decoder = getResponseDecoder()) {


Q: why aren't we using a ResponseDecoderWrapper here? It seems like logic is duplicated here and in ResponseDecoderWrapper

The wrapper is extra layer of indirection and it has other virtual interfaces which are not needed by quic client stream.

Signed-off-by: Dan Zhang <danzh@google.com>

RyanTheOptimist

I think it would be good to understand why we only need this mechanism in QUIC and not in other codecs. Perhaps we can update the CL description to flesh this out.

RyanTheOptimist · 2025-09-11T17:30:21Z

envoy/http/codec.h


+class ResponseDecoder;
+
+class ResponseDecoderHandle {


Please add a brief comment explaining the purpose of this class.

RyanTheOptimist · 2025-09-11T17:32:54Z

envoy/http/codec.h

+   * @return A handle to the response decoder. Caller can check the response decoder's liveness via
+   * the handle.
+   */
+  virtual ResponseDecoderHandlePtr getResponseDecoderHandle() PURE;


Should this perhaps be named createResponseDecoderHandle() to make it clear that it is creating a new object (as suggested by the return type of std::unique_ptr)?

Ali: I think this will be a breaking API change, but we get those all the time in Envoy which makes import so much fun. When this gets imported, can we change those classes to extend the new base class?

RyanTheOptimist · 2025-09-11T17:33:41Z

envoy/router/router.h

 * and/or router components
 */
-class UpstreamToDownstream : public Http::ResponseDecoder, public Http::StreamCallbacks {
+class UpstreamToDownstream : public Http::ResponseDecoderImplBase, public Http::StreamCallbacks {


I think this is so we get an implementation of the new getHandle method?

RyanTheOptimist · 2025-09-11T17:43:24Z

source/common/http/response_decoder_impl_base.h

+  }
+
+protected:
+  std::shared_ptr<bool> live_trackable_;


I see. So effectively, this Handle is basically a weak pointer to the decoder, but manually implemented?

Looking at the CL in more detail, I wonder if we could do something a bit simpler. What if we added a new capability to the response decoder, which is a callback to run when the decoder is destroyed. We could then implement this decoder something like:

class ResponseDecoderHandleImpl : public ResponseDecoderHandle { public: ResponseDecoderHandleImpl(explicit ResponseDecoder& decoder) : ldecoder_(&decoder) { decoder_.SetDestructionCallback( [] { OnDecoderDestroyed();} ); } void OnDecoderDestroyed() { decoder_ = nullptr; } OptRef<ResponseDecoder> get() override { if (decoder_ != nullpter) { return *decoder_; } return {}; } private: ResponseDecoder* decoder_; };

Then we don't need explicit ref-counting in the base class, and we don't need the Decoder to create Handles (though maybe we would still want to?). WDYT?

ResponseDecoderHandleImpl also needs to notify the decoder_ about its own lifetime then. There would be mutual references and detaching upon destruction between the handle and the decoder. It's possible, but I feel shared_ptr is less error-prone. Happy to chat more offline about the approach.

abeyad · 2025-09-11T17:53:25Z

I think it would be good to understand why we only need this mechanism in QUIC and not in other codecs. Perhaps we can update the CL description to flesh this out.

This bug effects HTTP/1 too from what we saw, it's not protocol specific. So the fix will need to handle all protocols. I think that's what ResponseDecoderWrapper does anyway, so I'm not sure why we need the specific checks in EnvoyQuicClientStream

danzh2010 · 2025-09-11T18:03:35Z

I think it would be good to understand why we only need this mechanism in QUIC and not in other codecs. Perhaps we can update the CL description to flesh this out.

This bug effects HTTP/1 too from what we saw, it's not protocol specific. So the fix will need to handle all protocols. I think that's what ResponseDecoderWrapper does anyway, so I'm not sure why we need the specific checks in EnvoyQuicClientStream

It's upto each codec whether to use the handle interface or not. I change the quic codec to use the handle as an example.

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 · 2025-09-15T20:42:07Z

/retest

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 · 2025-09-16T01:26:06Z

The PR notifier seems unrelated. This PR is ready for review!

danzh2010 · 2025-09-16T14:14:33Z

/retest

abeyad

Looks great, thanks Dan!

abeyad · 2025-09-16T17:40:00Z

Looks like the PR notifier job is broken at tip of tree (unrelated to this PR)

RyanTheOptimist

LGTM!

RyanTheOptimist · 2025-09-16T19:54:54Z

source/common/quic/envoy_quic_client_stream.h

  void useCapsuleProtocol();
 #endif

+  Http::ResponseDecoder* getResponseDecoder();


nit: Please add a comment which explains when/why it can be null.

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 · 2025-09-17T00:27:14Z

/retest

danzh2010 · 2025-09-17T00:46:50Z

/retest

…f response decoder (envoyproxy#41048)" This reverts commit 566b192. Signed-off-by: Dan Zhang <danzh@google.com>

#41141) …f response decoder (#41048)" This reverts commit 566b192. It's causing crash at ResponseDecoderWrapper Signed-off-by: Dan Zhang <danzh@google.com> Co-authored-by: Dan Zhang <danzh@google.com>

…se decoder (envoyproxy#41048) Commit Message: introduce ResponseDecoderHandle interface via which ResponseDecoder object can be obtained with guarantee that the object is still alive. Also added a new interface getResponseDecoderHandle() to ResponseDecoder and a new base implementation of ResponseDecoder ResponseDecoderImplBase which implements these new interfaces. Existing subclasses of ResponseDecoder are changed to inherit from ResponseDecoderImplBase. Additional Description: also make EnvoyQuicClientStream and ResponseDecoderWrapper to use the handle instead of directly accessing the passed-in response decoder, guarded by `envoy.reloadable_features.use_response_decoder_handle`. And when the decoder is dead, log error or abort based on `envoy.reloadable_features.abort_when_accessing_dead_decoder`. We should gradually transit from using raw pointer or reference of ResponseDecoder to using the handle interface everywhere else. This PR makes the transition in the 2 classes which we are observing life time issues today. Risk Level: low Testing: existing tests passed Docs Changes: N/A Release Notes: Y Platform Specific Features: N/A Runtime guard: envoy.reloadable_features.abort_when_accessing_dead_decoder(default false) and envoy.reloadable_features.use_response_decoder_handle Issue: envoyproxy#41060 --------- Signed-off-by: Dan Zhang <danzh@google.com> Co-authored-by: Dan Zhang <danzh@google.com> Signed-off-by: Misha Badov <mbadov@google.com>

envoyproxy#41141) …f response decoder (envoyproxy#41048)" This reverts commit 566b192. It's causing crash at ResponseDecoderWrapper Signed-off-by: Dan Zhang <danzh@google.com> Co-authored-by: Dan Zhang <danzh@google.com> Signed-off-by: Misha Badov <mbadov@google.com>

…of response decoder (envoyproxy#41048)" This reverts commit bfd5b10. Signed-off-by: Dan Zhang <danzh@google.com>

envoyproxy#41141) …f response decoder (envoyproxy#41048)" This reverts commit 566b192. It's causing crash at ResponseDecoderWrapper Signed-off-by: Dan Zhang <danzh@google.com> Co-authored-by: Dan Zhang <danzh@google.com>

Commit Message: Reapply "response decoder: add new interfaces for life time tracking of response decoder (#41048)" Additional Description: In addition to changes in #41048, this PR added a new constructor to ResponseDecoderWrapper to take a ResponseDecoderHandle to avoid accessing the decode object at anytime without checking its liveliness. It also changes ConnectivityGrid and HttpConnPoolImplBase to retain an ResponseDecoderHandle rather than directly referencing to ResponseDecoder during the async connection establishment. Added a regression test for the crash observed with the previous change. Risk Level: medium Testing: new integration tests Docs Changes: N/A Release Notes: Y Platform Specific Features: N/A Runtime guard: envoy.reloadable_features.use_response_decoder_handle default true and envoy.reloadable_features.abort_when_accessing_dead_decoder default false --------- Signed-off-by: Dan Zhang <danzh@google.com> Co-authored-by: Dan Zhang <danzh@google.com>

danzh1989 added 9 commits September 9, 2025 19:53

Add response decoder handle

ca29616

Signed-off-by: Dan Zhang <danzh@google.com>

Add response decoder handle

742ce65

Signed-off-by: Dan Zhang <danzh@google.com>

remove weird file

7a6605b

Signed-off-by: Dan Zhang <danzh@google.com>

fix subclasses

a57534a

Signed-off-by: Dan Zhang <danzh@google.com>

Fix duplicated lines in envoy_quic_client_stream.cc

7d6c5e9

Signed-off-by: Dan Zhang <danzh@google.com>

Fix duplicate declaration in envoy_quic_client_stream.h

5a704e8

Signed-off-by: Dan Zhang <danzh@google.com>

Update response_decoder_ usage in envoy_quic_client_stream.cc

918841f

Signed-off-by: Dan Zhang <danzh@google.com>

fixing build

563f3e7

Signed-off-by: Dan Zhang <danzh@google.com>

more refinning

fdd17cf

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 requested a review from mattklein123 as a code owner September 11, 2025 14:39

danzh2010 changed the title ~~response decoder: add new interfaces for life time assurance~~ response decoder: add new interfaces for life time tracking of response decoder Sep 11, 2025

danzh1989 added 2 commits September 11, 2025 15:38

handle error message

417e117

Signed-off-by: Dan Zhang <danzh@google.com>

release note

ba8f0d3

Signed-off-by: Dan Zhang <danzh@google.com>

repokitteh-read-only bot assigned abeyad and RyanTheOptimist Sep 11, 2025

abeyad reviewed Sep 11, 2025

View reviewed changes

Merge branch 'main' into decoderhandle

97ef957

Signed-off-by: Dan Zhang <danzh@google.com>

RyanTheOptimist reviewed Sep 11, 2025

View reviewed changes

danzh1989 added 2 commits September 11, 2025 21:20

fix build

5f4575a

Signed-off-by: Dan Zhang <danzh@google.com>

rename

50708eb

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 requested review from agrawroh, botengyao and yanavlasov as code owners September 11, 2025 21:59

fix more build

74680c2

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 requested a review from zuercher as a code owner September 12, 2025 14:39

fix http1 ActiveClient

be6d7cb

Signed-off-by: Dan Zhang <danzh@google.com>

danzh1989 added 2 commits September 14, 2025 20:04

fix test

850c93e

Signed-off-by: Dan Zhang <danzh@google.com>

test coverage

1a7b13b

Signed-off-by: Dan Zhang <danzh@google.com>

Merge branch 'main' into decoderhandle

242b503

Signed-off-by: Dan Zhang <danzh@google.com>

abeyad previously approved these changes Sep 16, 2025

View reviewed changes

RyanTheOptimist previously approved these changes Sep 16, 2025

View reviewed changes

danzh1989 added 2 commits September 16, 2025 21:14

comment

6575c1b

Signed-off-by: Dan Zhang <danzh@google.com>

Merge branch 'main' into decoderhandle

4627f86

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 dismissed stale reviews from RyanTheOptimist and abeyad via 4627f86 September 16, 2025 21:15

abeyad approved these changes Sep 16, 2025

View reviewed changes

danzh2010 enabled auto-merge (squash) September 16, 2025 21:22

RyanTheOptimist approved these changes Sep 16, 2025

View reviewed changes

danzh2010 merged commit 566b192 into envoyproxy:main Sep 17, 2025
25 checks passed

danzh2010 pushed a commit to danzh2010/envoy that referenced this pull request Sep 19, 2025

Revert "response decoder: add new interfaces for life time tracking o…

bfd5b10

…f response decoder (envoyproxy#41048)" This reverts commit 566b192. Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 mentioned this pull request Sep 19, 2025

Revert "response decoder: add new interfaces for life time tracking o… #41141

Merged

danzh2010 pushed a commit to danzh2010/envoy that referenced this pull request Sep 23, 2025

Reapply "response decoder: add new interfaces for life time tracking …

422b405

…of response decoder (envoyproxy#41048)" This reverts commit bfd5b10. Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 pushed a commit to danzh2010/envoy that referenced this pull request Sep 23, 2025

Reapply "response decoder: add new interfaces for life time tracking …

9fc6812

…of response decoder (envoyproxy#41048)" This reverts commit bfd5b10. Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 mentioned this pull request Sep 23, 2025

response decoder: Roll forward ResponseDecoderHandle #41193

Merged


		class ResponseDecoder;

		class ResponseDecoderHandle {

Conversation

danzh2010 commented Sep 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Sep 11, 2025

Uh oh!

danzh2010 commented Sep 11, 2025

Uh oh!

abeyad 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!

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!

RyanTheOptimist 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!

abeyad commented Sep 11, 2025

Uh oh!

danzh2010 commented Sep 11, 2025

Uh oh!

danzh2010 commented Sep 15, 2025

Uh oh!

danzh2010 commented Sep 16, 2025

Uh oh!

danzh2010 commented Sep 16, 2025

Uh oh!

abeyad left a comment

Choose a reason for hiding this comment

Uh oh!

abeyad commented Sep 16, 2025

Uh oh!

RyanTheOptimist 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!

danzh2010 commented Sep 17, 2025

Uh oh!

danzh2010 commented Sep 17, 2025

Uh oh!

Uh oh!

Reviewers

danzh2010 commented Sep 11, 2025 •

edited

Loading