[http] Introduce error status return type for dispatch (no-op) by asraa · Pull Request #10879 · envoyproxy/envoy

asraa · 2020-04-21T17:11:52Z

(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

@yanavlasov

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

yanavlasov · 2020-04-22T16:01:20Z

include/envoy/http/codec.h

+   * callbacks.
   */
-  virtual void dispatch(Buffer::Instance& data) PURE;
+  virtual Envoy::Http::Status dispatch(Buffer::Instance& data) PURE;


You do not need to fully qualify Status namespace here. I think just Status should work.

Done -- I changed the rest of the namespaces as well, opting for Http::Status, since Http::Connection is also used

yanavlasov · 2020-04-22T16:03:25Z

include/envoy/http/codec.h

   * Dispatch incoming connection data.
   * @param data supplies the data to dispatch. The codec will drain as many bytes as it processes.
+   * @return Envoy::Http::Status indicating the status of the codec. Holds any errors found in the
+   * callbacks.


Maybe something like this: "Holds any errors encountered during processing of incoming data" ?

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

asraa · 2020-04-27T15:37:37Z

ping?

asraa · 2020-04-29T15:29:00Z

@mattklein123 Hey Matt! Could you please take a look at this? It is code pulled from #10484 that doesn't include the local H/1 changes, but just no-op return value change that would handle (currently all OK) error statuses from codecs.

mattklein123 · 2020-04-29T18:22:20Z

Will take a look, but can @yanavlasov please review first and approve?

jmarantz

I haven't gone over the tests in detail yet but here's a first pass.

source/common/http/codec_client.cc

jmarantz · 2020-04-29T18:43:20Z

source/common/http/conn_manager_impl.cc

    try {
-      codec_->dispatch(data);
+      status = codec_->dispatch(data);
+      // TODO(#10878): Remove this when exception removal is complete. It is currently in migration.
+      // Soon we won't need to catch these exceptions, as they'll be propagated through the error
+      // statuses callbacks and returned from dispatch.
    } catch (const FrameFloodException& e) {
-      handleCodecException(e.what());
-      return Network::FilterStatus::StopIteration;
+      status = bufferFloodError(e.what());
    } catch (const CodecProtocolException& e) {
+      status = codecProtocolError(e.what());
+    }


can we factor out this stanza from codec_client.c? It looks identical.

The stanza below is different of course but this looks like it would make a good helper function callable in both places.

qq before I make this change (as it's pretty big) about where to factor this out to:

utility function that takes codec and data. pros: will be simple to remove when exception removal.

I could factor this out so it's baked into ConnectionImpl::dispatch, i.e. dispatch will always return a status now (by calling a helper with the try/catch block around an innerDispatch and then doing the exception to status conversion). This has the advantage of minimizing the diff between the new and legacy codec_impl_tests. calls to dispatch will always return a status, not a status sometimes and a throw for legacy. I am leaning to this option.

Yeah +1 for (2) that sounds great.

done, I wanted to at least try that suggestion out. A uniform return value from dispatch is clean and causes less confusion.

There was one big pain point, see the diff in http2/codec_impl_test.cc. This test

envoy/test/common/http/http2/codec_impl_test.cc

Line 414 in c86c679

EXPECT_THROW_WITH_MESSAGE(response_encoder_->encodeHeaders(early_hint_headers, false),

will fail unless I directly call the innerDispatch in the mock connection in the test fixture. Why? Because encodeHeaders throws, and if I directly call dispatch, I swallow the throw because write(_,_) does not return a status. The stack trace for the failure when that happens goes through encodeHeaders, encodeHeadersBase, sendPendingFrames(), and then throws. This throw does not occur in a dispatching context! see

envoy/source/common/http/http2/codec_impl.cc

Line 893 in c86c679

if (dispatching_ || connection_.state() == Network::Connection::State::Closed) {

Hence, this solution is kinda ugly because in H/2 test I had to call in inner dispatch, because we test a non-dispatching throw.

jmarantz · 2020-04-29T18:45:18Z

source/common/http/http2/codec_impl.cc

  ENVOY_CONN_LOG(trace, "dispatching {} bytes", connection_, data.length());
+  // Make sure that dispatching_ is set to false after dispatching, even when
+  // ConnectionImpl::dispatch throws an exception.
+  Cleanup cleanup([this]() { dispatching_ = false; });


TODO to make this not throw and thus remove this hack? It seems unexpected to have a method that returns a status and can also throw.

I think you are still going to need this given early returns?

Even with early returns, we would still be executing a dispatching_ = false line at the end of the method if we are guaranteed not to throw, right? Or early returns like possible RELEASE_ASSERTs? If so I'll add a clarification

I just meant early returns in this function, unless you structure it in such a way to only have a single return at the end. It's not a big deal either way, we can sort it in future PRs?

Does this function still call others that throw? Or are the throws only self-inflicted in the loop below? Maybe we can just clean this up right here?

Gotcha -- yeah, it'll depend on the next PR. I clarified, left the issue # in there just for remembering in future PRs when it comes up!

Does this function still call others that throw?

Yeah -- nghttp2_session_mem_recv may throw in a callback, so I don't think it's possible to clean up right now.

jmarantz · 2020-04-29T18:48:05Z

test/integration/fake_upstream.h

+      Http::Status status;
      try {
-        parent_.codec_->dispatch(data);
+        status = parent_.codec_->dispatch(data);
+        // TODO(#10878): Remove this when exception removal is complete. It is currently in
+        // migration. Soon we won't need to catch these exceptions, as they'll be propagated through
+        // the error statuses callbacks and returned from dispatch.
      } catch (const Http::CodecProtocolException& e) {
-        ENVOY_LOG(debug, "FakeUpstream dispatch error: {}", e.what());
+        status = Http::codecProtocolError(e.what());
+      }


This looks similar to the factorizable stanza above, though there's one less 'catch' in it. Re-use the suggested factorered helper-method out here anyway?

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

asraa · 2020-04-30T21:02:34Z

test/common/http/http2/codec_impl_test.cc

        while (buffer_.length() > 0) {
          dispatching_ = true;
-          connection.dispatch(buffer_);
+          status = connection.innerDispatch(buffer_);


I felt bad about writing this line. FYI.

Can you maybe add a comment/TODO to reference exception removal for someone that comes across this?

mattklein123

You definitely win the epic slog/refactor award. 👏 :)

re: the exception that you had to work around with calling innerDispatch, this is another example of a case in which that just shouldn't ever happen since no one should ever call it that way (similar to H1 trying to make a client call without host, method, etc.). I'm not sure how we want to handle this, but I would probably advocate just moving those cases to RELEASE_ASSERT and making them death tests, but I don't feel strongly about it. If you want to do that in a follow up I would just put in relevant TODO/comment as I mentioned in the review. Thank you!

/wait

mattklein123 · 2020-04-30T21:46:58Z

source/common/http/codec_client.cc

-    codec_->dispatch(data);
-  } catch (CodecProtocolException& e) {
-    ENVOY_CONN_LOG(debug, "protocol error: {}", *connection_, e.what());
+  Status status = codec_->dispatch(data);


nit: const, same in other places if applicable.

mattklein123 · 2020-04-30T21:51:04Z

source/common/http/http2/codec_impl.cc

  ENVOY_CONN_LOG(trace, "dispatching {} bytes", connection_, data.length());
+  // Make sure that dispatching_ is set to false after dispatching, even when
+  // ConnectionImpl::dispatch throws an exception.
+  Cleanup cleanup([this]() { dispatching_ = false; });


I think you are still going to need this given early returns?

mattklein123 · 2020-04-30T21:54:12Z

test/common/http/http2/codec_impl_test.cc

        while (buffer_.length() > 0) {
          dispatching_ = true;
-          connection.dispatch(buffer_);
+          status = connection.innerDispatch(buffer_);


Can you maybe add a comment/TODO to reference exception removal for someone that comes across this?

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

jmarantz · 2020-05-01T17:18:31Z

source/common/http/http2/codec_impl.cc

  ENVOY_CONN_LOG(trace, "dispatching {} bytes", connection_, data.length());
+  // Make sure that dispatching_ is set to false after dispatching, even when
+  // ConnectionImpl::dispatch throws an exception.
+  Cleanup cleanup([this]() { dispatching_ = false; });


Does this function still call others that throw? Or are the throws only self-inflicted in the loop below? Maybe we can just clean this up right here?

jmarantz · 2020-05-01T17:20:10Z

source/common/http/utility.cc

+  } catch (CodecProtocolException& e) {
+    status = codecProtocolError(e.what());
+  } catch (PrematureResponseException& e) {
+    status = prematureResponseError(e.what(), e.responseCode());


should we also catch EnvoyException to make sure there are no others that leak through?

or maybe even std::exception?

I would rather not catch any more exceptions than we do today and otherwise let the process crash? I'm not sure why we would expand the catch targets as part of this cleanup?

sg. if we don't have a higher level catch then expanding the scope here isn't needed.

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

mattklein123 · 2020-05-01T22:36:36Z

LGTM but might need a tidy fix. Nice work!

/wait

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

mattklein123 · 2020-05-04T20:10:09Z

/azp run envoy-presubmit

azure-pipelines · 2020-05-04T20:10:18Z

Azure Pipelines successfully started running 1 pipeline(s).

yanavlasov

LGTM modulo minor nit. Thanks for getting this done.

yanavlasov · 2020-05-05T16:08:10Z

source/common/http/conn_manager_impl.cc

 }

-void ConnectionManagerImpl::handleCodecException(const char* error) {
+void ConnectionManagerImpl::handleCodecException(absl::string_view error) {


Nit: make it handleCodecError ? Since it is not an exception anymore.

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

asraa added 2 commits April 21, 2020 10:48

change dispatch return value

b3b5559

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

add issue tracking

4a3bc53

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

asraa requested review from alyssawilk and mattklein123 as code owners April 21, 2020 17:11

unify error handling in test class

460a156

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

yanavlasov requested changes Apr 22, 2020

View reviewed changes

asraa added 4 commits April 22, 2020 12:21

address comment

0e6d9fc

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

simply qualifyin names

1ac68e4

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

use Http:: namespace

10b3650

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

remaining namspaces

2b41385

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

asraa mentioned this pull request Apr 22, 2020

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

Closed

9 tasks

fix

b13b62c

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

mattklein123 assigned yanavlasov Apr 22, 2020

asraa added 3 commits April 23, 2020 09:17

Merge remote-tracking branch 'upstream/master' into common-codec-change

c75cec0

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

Merge remote-tracking branch 'upstream/master' into common-codec-change

2ed3b5b

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

update test after merge

ff3a842

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

jmarantz self-assigned this Apr 29, 2020

mattklein123 self-assigned this Apr 29, 2020

jmarantz reviewed Apr 29, 2020

View reviewed changes

mattklein123 added the waiting label Apr 29, 2020

make dispatch always return status

9b311d8

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

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

fix fuzz tests

8465105

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

asraa commented Apr 30, 2020

View reviewed changes

mattklein123 requested changes Apr 30, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Apr 30, 2020

address comments

732bd62

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

repokitteh-read-only bot removed the waiting label May 1, 2020

jmarantz reviewed May 1, 2020

View reviewed changes

just add tracking number for the cleanup

0597d3b

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

repokitteh-read-only bot added the waiting label May 1, 2020

asraa added 2 commits May 4, 2020 12:04

fix clang tidy

4bd4f25

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

Merge remote-tracking branch 'upstream/master' into common-codec-change

7b8ee31

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

repokitteh-read-only bot removed the waiting label May 4, 2020

mattklein123 previously approved these changes May 4, 2020

View reviewed changes

yanavlasov requested changes May 5, 2020

View reviewed changes

rename

7f125d9

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

asraa dismissed mattklein123’s stale review via 7f125d9 May 5, 2020 16:29

yanavlasov approved these changes May 5, 2020

View reviewed changes

mattklein123 approved these changes May 5, 2020

View reviewed changes

mattklein123 merged commit 23185de into envoyproxy:master May 5, 2020

Conversation

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

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!

asraa commented Apr 27, 2020

Uh oh!

asraa commented Apr 29, 2020

Uh oh!

mattklein123 commented Apr 29, 2020

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asraa Apr 29, 2020 • 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!

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!

asraa May 1, 2020 • 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!

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!

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!

mattklein123 commented May 1, 2020

Uh oh!

mattklein123 commented May 4, 2020

asraa commented Apr 21, 2020 •

edited

Loading

asraa Apr 29, 2020 •

edited

Loading

asraa May 1, 2020 •

edited

Loading