[http] (Proof of concept) Remove an exception from the H/1 codec by asraa · Pull Request #10484 · envoyproxy/envoy

asraa · 2020-03-23T16:52:42Z

This is a proof of concept to remove exceptions from the codec.

The flow control stays the same by returning non-zero error code from codec callbacks. Information about the error (for eg, header or trailer overflow) is stored in a local variable codec_exception_ and returned as an absl::Status. Note: this only happens while dispatching.
If an error status is detected after a call to dispatch in the HCM or codec, we close the connection in the same way we catch a codec exception.
If we are not dispatching, the codec error is treated as a RELEASE_ASSERT.

Testing: See H/1 codec (large diff) for many headers testing that returns an error status

Signed-off-by: Asra Ali <asraa@google.com>

htuch

Without diving into the weeds, the basic control flow here makes sense. Is the plan-of-record to have a forked codec with runtime switch?

mattklein123

At a high level this looks great to me. Flushing out a few high level comments. Thank you so much for working on this!

/wait

mattklein123 · 2020-03-23T18:13:01Z

source/common/http/codec_client.cc

  try {
-    codec_->dispatch(data);
+    auto status = codec_->dispatch(data);
+    if (!status.ok()) {


Should we differentiate between codec exception and premature response exception here to be clear and maybe assert that it's just the codec type for now if the other is not implemented yet?

Hmm. Yes. It looks like I'd need to differentiate between codec/premature, and also have some information on the HttpCode of the premature response.

We could wrap absl::Status to have that data (and have our defined HttpCodes), or we could have an accessor in to the codecs, for eg getCodecException()/getPrematureResponseException. wdyt? @yanavlasov

We could wrap absl::Status to have that data (and have our defined HttpCodes)

My preference would be for the return value to contain the needed info, similar to the exception it is replacing?

Wrapping Status would make it lose some of its value. Why do we need to distinguish between the two failure conditions? For protocol error stats?

Yeah we do different logic depending on the exception type currently. This needs to be modeled somehow.

Looking at this seems like absl::Status might not be what we want. Specifically its error space does not map to Envoy use cases well. Here we use the "Internal error", which is not strictly correct. absl::Status error space is mostly a reflection of internal Google practices centered around stubby and seems to not be well suited for us.
I think we should define our own type very similar to absl::Status but with our own error space, which would let us map exception types naturally. absl::Status is very small if you take various helpers out. We can use the same handle-body pattern to make it fit into a register and get rid of some absl::Status stuff we will not need and perhaps add stuff that we will need.
However, this will require us to make our own StatusOr<> as well.

+1 for doing what @yanavlasov suggests if that is the cleanest way of getting what we need. It doesn't seem that bad/hard to duplicate.

source/common/http/codec_client.cc

source/common/http/conn_manager_impl.cc

include/envoy/http/codec.h

mattklein123 · 2020-03-23T18:20:13Z

source/common/http/http1/codec_impl.cc

  if (data.length() > 0) {
    for (const Buffer::RawSlice& slice : data.getRawSlices()) {
-      total_parsed += dispatchSlice(static_cast<const char*>(slice.mem_), slice.len_);
+      dispatching_ = true;


You might consider using Cleanup (or similar) here, to avoid any strangeness around not setting disaptching_ back to false in the early return cases. Granted, it might not matter but probably good to be consistent. If this is done you can probably just set dispatching_ above to true with a guard in a single place?

Yes, I like this pattern as well.

There is similar code in the H2 codec that should be changed to use cleanup also.

Should this also be set during maybeDirectDispatch? I think it wouldn't hurt.

Setting dispatch_ at the top of ConnectionImpl::dispatch + cleanup to unset seems like the way to go.

Should this also be set during maybeDirectDispatch? I think it wouldn't hurt.

As in, redundantly set to dispatching_ to true inside the method?

test/common/http/codec_client_test.cc

asraa · 2020-03-23T18:26:01Z

the basic control flow here makes sense. Is the plan-of-record to have a forked codec with runtime switch?

It might be. Would love more comments. We could fork the codecs, but the change to dispatch's return value would still need to be changed. (Or we could have an inner wrapper to Connection::dispatch).

mattklein123 · 2020-03-23T18:26:49Z

Is the plan-of-record to have a forked codec with runtime switch?

We discussed this in our meeting last week. If we do this in small incremental changes I don't personally feel the need to make this runtime controllable. If we do want to do that I think yes we are going to need to copy the code. cc @yanavlasov @asraa @antoniovicente @alyssawilk

Signed-off-by: Asra Ali <asraa@google.com>

htuch · 2020-03-24T13:16:35Z

source/common/http/http1/codec_impl.cc

  if (data.length() > 0) {
    for (const Buffer::RawSlice& slice : data.getRawSlices()) {
-      total_parsed += dispatchSlice(static_cast<const char*>(slice.mem_), slice.len_);
+      dispatching_ = true;


ASSSERT(!dispatching_) would make sense here.

htuch · 2020-03-24T13:17:15Z

source/common/http/http1/codec_impl.cc

+      dispatching_ = false;
    }
  } else {
+    dispatching_ = true;


htuch · 2020-03-24T13:17:39Z

source/common/http/http1/codec_impl.cc

  if (data.length() > 0) {
    for (const Buffer::RawSlice& slice : data.getRawSlices()) {
-      total_parsed += dispatchSlice(static_cast<const char*>(slice.mem_), slice.len_);
+      dispatching_ = true;


Yes, I like this pattern as well.

htuch · 2020-03-24T13:20:39Z

source/common/http/http1/codec_impl.h


  static http_parser_settings settings_;

+  bool dispatching_ : 1;


Nit: prefer default initialize scalars with {} (this might be necessary to allow the suggested ASSERTs to work.

This one was initialized like this in the H/2 codec, however in a new connection it is initialized as false... it makes more sense for me to initialize false, so I did {false}.

source/common/http/http1/codec_impl.cc

antoniovicente · 2020-03-24T16:33:27Z

source/common/http/http1/codec_impl.h

  static http_parser_settings settings_;

+  bool dispatching_ : 1;
+  absl::Status codec_exception_;


Can you comment on the use of this new field?

We should also ASSERT(codec_exception_.ok()) in various places including at the top of dispatchSlice and places where we end up overwriting it.

I think with custom type we can push this logic into the type and either make it ASSERT when an error condition is being overwritten or make it idempotent.

Some methods shouldn't be entered when there's an error already set. I agree that we can have the custom type complain when an error value is overwritten.

source/common/http/http1/codec_impl.cc

Signed-off-by: Asra Ali <asraa@google.com>

alyssawilk

Looks good overall! some thoughts on my end

include/envoy/http/codec.h

source/common/http/http1/codec_impl.cc

alyssawilk · 2020-03-24T18:20:04Z

source/common/http/http1/codec_impl.cc

-    throw CodecProtocolException(absl::StrCat(header_type, " size exceeds limit"));
+    codec_exception_ = absl::InternalError(absl::StrCat(header_type, " size exceeds limit"));
+    // Crash if not dispatching_, otherwise exit early from the  parser.
+    RELEASE_ASSERT(dispatching_, "CodecProtocolException occurred outside of dispatching");


If we're going to do this I'd prefer we have ASSERTS under completeLastHeader() and all friends that they're below dispatching as well.
Basically let's have fuzzing give us defense in depth for functions which should only be called here.

I know that this will likely break something, and I'm a little confused by our model of throwing exceptions -- RequestEncoderImpl::encodeHeaders gets called outside the scope of dispatching_ pretty often, and throws when there's missing path/method headers. I'm guessing that when it's called outside of dispatching_, we already have the guarantee that these exist?

If so, maybe I should put ASSERT's in the branches where we throw

know that this will likely break something, and I'm a little confused by our model of throwing exceptions -- RequestEncoderImpl::encodeHeaders gets called outside the scope of dispatching_ pretty often, and throws when there's missing path/method headers.

TBH I don't recall the history here. That "exception" shouldn't be an exception, it should just be an ASSERT/crash/etc. because it should never happen, so feel free to fix that however you want.

You're right, it's only the encode pathways that may break something. I added ASSERTs!

Signed-off-by: Asra Ali <asraa@google.com>

asraa · 2020-03-25T19:11:53Z

Quick update: It looks like addressing quite a few comments here means we should have a custom Envoy::Status. Yan's working on that in parallel with me working on the plumbing to split the codec into a legacy and exception-removed one.

stale · 2020-04-01T22:24:30Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

Signed-off-by: Asra Ali <asraa@google.com>

stale · 2020-04-08T22:34:03Z

This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

Signed-off-by: Asra Ali <asraa@google.com>

asraa · 2020-04-13T17:48:42Z

Hey!! I updated this PR to address the comments, including adding Yan's StatusOr in. Some outstanding comments I responded to. WDYT?

Signed-off-by: Asra Ali <asraa@google.com>

mattklein123 · 2020-04-14T18:18:05Z

source/common/http/http1/codec_impl.cc

-      return 0;
+      Envoy::StatusOr<int> statusor =
+          static_cast<ConnectionImpl*>(parser->data)->onHeaderField(at, length);
+      return statusor.ok() ? statusor.value() : 1;


Can you add a comment on why we return 1 in the non-OK case? Same elsewhere. You might consider a human readable constant for the special return codes if that is helpful for readability.

Done. I'd prefer a human readable constant. For uniformity, I think it makes the most sense to use -1, and reserve 1 and 2 (which have special meanings for on_headers_complete)

mattklein123 · 2020-04-14T18:22:09Z

source/common/http/http1/codec_impl.cc

+    // Crash if not dispatching_, otherwise set error and exit early from the parser.
+    RELEASE_ASSERT(dispatching_, "CodecProtocolError occurred outside of dispatching");


We must be dispatching during header completion, right? Meaning, I think it's very easy to convince ourselves that this must be the case? If so I'm not sure it's worth this assert here. (vs. encode paths which are a lot more confusing).

Hmm yes, completeLastHeader has a very tractable call stack, all of which occur in callbacks.

mattklein123 · 2020-04-14T18:32:27Z

source/common/http/http1/codec_impl.cc

-    throw CodecProtocolException(absl::StrCat(header_type, " size exceeds limit"));
+    codec_exception_ = absl::InternalError(absl::StrCat(header_type, " size exceeds limit"));
+    // Crash if not dispatching_, otherwise exit early from the  parser.
+    RELEASE_ASSERT(dispatching_, "CodecProtocolException occurred outside of dispatching");


know that this will likely break something, and I'm a little confused by our model of throwing exceptions -- RequestEncoderImpl::encodeHeaders gets called outside the scope of dispatching_ pretty often, and throws when there's missing path/method headers.

TBH I don't recall the history here. That "exception" shouldn't be an exception, it should just be an ASSERT/crash/etc. because it should never happen, so feel free to fix that however you want.

mattklein123

In general this looks awesome. Let me know how you want to proceed with the full/final review staging.

Signed-off-by: Asra Ali <asraa@google.com>

asraa · 2020-04-15T13:11:37Z

@yanavlasov @antoniovicente What do you think of this POC? There are some minor nits about adding ASSERTs unresolved. I think I've gotten the general structure of the HTTP/1 change, and I could proceed replacing all throws for HTTP/1.

If we want to split off, we should decide what to do about the common changes -- dupe them in our branches, and resolve conflicts before merging? Or a separate PR with the common changes (dispatch return value, HCM, codec_client, and test changes)

Signed-off-by: Asra Ali <asraa@google.com>

(This is the merge-able portion of #10484. It does not include the behavior changes in H/1. Only the necessary changes are there for H/1 and H/2 exception removal to happen separately.) Description: This introduces a new return value, `Envoy::Http::Status` for `Connection::dispatch`. Currently, dispatch will always return an OK status. This facilitates the migration to remove exceptions in codecs and replace them with error statuses. The HCM and codec client can now handle statuses returned from dispatch, although they will continue to support exceptions (by translating them to the corresponding status) until legacy codecs are deprecated. Following this, PRs will stage exception removal in H/1 and H/2 codecs, codecs will be forked to have a legacy version, and a runtime override will allow users to continue to use legacy codecs during the migration. See issue #10878 for a full detailed plan and overview. Risk: Medium-high. A no-op, but touches sensitive code. Testing: All tests pass, this is a no-op Issue: #10878 Signed-off-by: Asra Ali <asraa@google.com>

asraa added 2 commits March 20, 2020 16:30

poc

0f6c111

Signed-off-by: Asra Ali <asraa@google.com>

change dispatch signature and remove a single exception from H/1 codec

b1c8111

Signed-off-by: Asra Ali <asraa@google.com>

asraa requested review from alyssawilk and mattklein123 as code owners March 23, 2020 16:52

mattklein123 assigned mattklein123, alyssawilk, antoniovicente and yanavlasov Mar 23, 2020

asraa added 2 commits March 23, 2020 13:06

Merge remote-tracking branch 'upstream/master' into poc

9380312

Signed-off-by: Asra Ali <asraa@google.com>

fix spelling

d688092

Signed-off-by: Asra Ali <asraa@google.com>

htuch reviewed Mar 23, 2020

View reviewed changes

mattklein123 requested changes Mar 23, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Mar 23, 2020

asraa added 2 commits March 24, 2020 08:45

change tests

62725d2

Signed-off-by: Asra Ali <asraa@google.com>

fix tests to Throw

1330a92

Signed-off-by: Asra Ali <asraa@google.com>

repokitteh-read-only bot removed the waiting label Mar 24, 2020

htuch reviewed Mar 24, 2020

View reviewed changes

mattklein123 added the waiting label Mar 24, 2020

antoniovicente reviewed Mar 24, 2020

View reviewed changes

source/common/http/http1/codec_impl.cc Show resolved Hide resolved

antoniovicente reviewed Mar 24, 2020

View reviewed changes

source/common/http/http1/codec_impl.cc Outdated Show resolved Hide resolved

add cleanup and asserts/comments

e4f899e

Signed-off-by: Asra Ali <asraa@google.com>

repokitteh-read-only bot removed the waiting label Mar 24, 2020

alyssawilk reviewed Mar 24, 2020

View reviewed changes

mattklein123 added the waiting label Mar 24, 2020

fixup some comments

1aced66

Signed-off-by: Asra Ali <asraa@google.com>

repokitteh-read-only bot removed the waiting label Mar 25, 2020

mattklein123 added the waiting label Mar 25, 2020

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Apr 1, 2020

Merge remote-tracking branch 'upstream/master' into poc

5e91f71

Signed-off-by: Asra Ali <asraa@google.com>

stale bot closed this Apr 8, 2020

asraa added 4 commits April 13, 2020 09:38

Merge remote-tracking branch 'upstream/master' into poc

b04cce8

Signed-off-by: Asra Ali <asraa@google.com>

use statusor and address comments

e44f20a

Signed-off-by: Asra Ali <asraa@google.com>

fixup

a3de648

Signed-off-by: Asra Ali <asraa@google.com>

fix hcm

c506e18

Signed-off-by: Asra Ali <asraa@google.com>

asraa reopened this Apr 13, 2020

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Apr 13, 2020

repokitteh-read-only bot removed the waiting label Apr 13, 2020

use envoy namespace for status

32f9fe6

Signed-off-by: Asra Ali <asraa@google.com>

mattklein123 reviewed Apr 14, 2020

View reviewed changes

mattklein123 added the waiting label Apr 14, 2020

add comments

bd36821

Signed-off-by: Asra Ali <asraa@google.com>

repokitteh-read-only bot removed the waiting label Apr 15, 2020

Merge remote-tracking branch 'upstream/master' into poc

c2cc3ed

Signed-off-by: Asra Ali <asraa@google.com>

asraa added 2 commits April 16, 2020 10:15

fix clang tidy

0f9fa45

Signed-off-by: Asra Ali <asraa@google.com>

Merge remote-tracking branch 'upstream/master' into poc

7a7facf

Signed-off-by: Asra Ali <asraa@google.com>

asraa mentioned this pull request Apr 21, 2020

[HTTP] High level design: Remove exceptions in codecs #10878

Closed

9 tasks

asraa closed this Apr 21, 2020

asraa mentioned this pull request Apr 21, 2020

[http] Introduce error status return type for dispatch (no-op) #10879

Merged


		static http_parser_settings settings_;

		bool dispatching_ : 1;

		// Crash if not dispatching_, otherwise set error and exit early from the parser.
		RELEASE_ASSERT(dispatching_, "CodecProtocolError occurred outside of dispatching");

Conversation

asraa commented Mar 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

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!

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!

Uh oh!

Uh oh!

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!

Uh oh!

asraa commented Mar 23, 2020

Uh oh!

mattklein123 commented Mar 23, 2020

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!

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!

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

asraa commented Mar 25, 2020

Uh oh!

stale bot commented Apr 1, 2020

asraa commented Mar 23, 2020 •

edited

Loading