[http] Remove exceptions from HTTP/1 codec callbacks by asraa · Pull Request #11101 · envoyproxy/envoy

asraa · 2020-05-07T17:04:21Z

Commit Message: Remove exceptions from HTTP/1 codec callbacks. Replaces with http_parser exit codes that indicate failure. codec_status_ propagates the error.

Additional Description:

I know the diff is slightly messy but the principles I abided by were: Replace throw with setting codec_status_, immediately return and propagate return up to callback with an error exit code, always ASSERT(dispatching_) in the body of the method that throws, always ASSERT(codec_status_.ok()) before setting the codec status.
The remaining exception is in encodeHeaders, which I will need to replace with ENVOY_BUG
I audited for throws in the includes for this file and did not find anything used in the codec_impl, but I will need to do another pass.
This is just part 1 of my HTTP/1 PRs. Part 2 is exception to error handling for encodeHeaders and any other utility functions. This is just a PR to stage.

Testing: Tests pass, codec_impl_fuzz_test has been running for a few minutes.
Risk level: Medium, this should do nothing but is a codec behavior change.
Issues: #10878

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

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

source/common/http/http1/codec_impl.cc

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

mattklein123 · 2020-05-07T18:38:55Z

The remaining exception is in encodeHeaders, which I will need to replace with better error handling since the throw is not caught in a separate PR.

Just to make sure we are all on the same page, what is the plan for the "application error" exceptions? I don't really know what the caller would do with an error other than crash, and adding return code from encodeHeaders would be a larger change. Can we just PANIC/RELEASE_ASSERT in those cases for now?

asraa · 2020-05-07T18:55:32Z

Can we just PANIC/RELEASE_ASSERT in those cases for now?

Hmm yeah I'm happy to RELEASE_ASSERT, since that's preserving existing behavior, but I think I had some feedback to change this as I remove exceptions. Would you imagine some error status handling in sendLocalReply and other callers of encodeHeaders? @alyssawilk @antoniovicente What do you think?

mattklein123 · 2020-05-07T18:58:04Z

Would you imagine some error status handling in sendLocalReply and other callers of encodeHeaders?

I honestly don't know what that would be. If we insist in not crashing in these cases the only thing I can think of to do would be to ASSERT and then continue on to fill in some default values or something.

source/common/http/http1/codec_impl.h

jmarantz · 2020-05-08T12:59:14Z

source/common/http/http1/codec_impl.cc

          "envoy.reloadable_features.reject_unsupported_transfer_encodings")),
-      output_buffer_([&]() -> void { this->onBelowLowWatermark(); },
-                     [&]() -> void { this->onAboveHighWatermark(); }),
+      dispatching_(false), output_buffer_([&]() -> void { this->onBelowLowWatermark(); },


I feel like the global capture could have unexpected costs as the code evolves. I think it's OK for tests, but WDYT of making this prod code explicitly capture what it needs?

I'm unfamiliar with this code, but I think the reason why it was written like that was because client/server would need to capture a disjoint set of things. Could pass both active request/pending response in, I can try to

oh, sorry. I misread the diff and thought you had added this. I'm not crazy about it but it's pre-existing so lgtm.

source/common/http/http1/codec_impl.cc

jmarantz · 2020-05-08T13:04:16Z

source/common/http/http1/codec_impl.cc

    sendProtocolError(Http1ResponseCodeDetails::get().InvalidUrl);
-    throw CodecProtocolException("http/1.1 protocol error: invalid url in request line");
+    ASSERT(codec_status_.ok());
+    ENVOY_LOG_MISC(info, "SETTING PROTOCOL ERROR");


did you plan to leave this in? If so, can you include the URL?

done -- but actually this was useful for debugging. What do you think about adding logging statements when the codec_status_ sets an error?

This ends up getting logged when onData calls dispatch, but that was too late for any codec level test debugging. At that point I might want to wrap the whole pattern of

ASSERT(codec_status_.ok()); codec_status_ = .... ; ENVOY_LOG_MISC(trace, codec_status_.message);

into one thing.

When debugging something in the heat of the moment, more logs are great, as long as the quantity of logs doesn't become a problem. I only suggest that if there's a URL to log, we include that.

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

alyssawilk · 2020-05-11T15:07:49Z

Sounds like a perfect use of ENVOY_BUG (assert in debug mode, log error with N^2 backoff in opt) with error handling - wasn't someone going to look into that this quarter?

…

On Thu, May 7, 2020 at 2:58 PM Matt Klein ***@***.***> wrote: Would you imagine some error status handling in sendLocalReply and other callers of encodeHeaders? I honestly don't know what that would be. If we insist in not crashing in these cases the only thing I can think of to do would be to ASSERT and then continue on to fill in some default values or something. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#11101 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AELALPNPOXAIKHJDKEXS5WDRQMAEXANCNFSM4M3P56KQ> .

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

mattklein123 · 2020-05-12T16:45:17Z

Sounds like a perfect use of ENVOY_BUG (assert in debug mode, log error
with N^2 backoff in opt) with error handling - wasn't someone going to look
into that this quarter?

Yes, agreed, if there is a sane default behavior we can agree on this is a fine use case of that paradigm IMO.

mattklein123

Thanks this LGTM at a high level so will defer to others for detailed review. Nice work!

source/common/http/http1/codec_impl.cc

jmarantz

basically looks great, modulo the [&] capture and some commenting around the significance of the specific status codes ints.

jmarantz · 2020-05-13T12:38:00Z

source/common/http/http1/codec_impl.h


  bool resetStreamCalled() { return reset_stream_called_; }

+  // The following define special return values for http_parser callbacks.


put in the comment here where these constants come from, and maybe that they need to not overlap with standard HTTP Status codes?

Thanks -- one clarification: you said "These codes do not overlap with standard HTTP Status codes". Is it also correct to strengthen that to "These codes must not overlap with standard HTTP Status codes"? Or are they unrelated?

Unrelated. They're return values for the user defined callbacks for the parser, so they don't do anything but signal execution flow for the parser.

Maybe "The use of these codes are distinct from standard HTTP Status codes"

jmarantz · 2020-05-13T12:45:24Z

/wait

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

jmarantz · 2020-05-13T22:45:48Z

/azp run

azure-pipelines · 2020-05-13T22:45:59Z

Azure Pipelines successfully started running 1 pipeline(s), but failed to run 2 pipeline(s).

This reverts commit 3550a7a.

…oxy#11101)" This reverts commit 3550a7a. Signed-off-by: Asra Ali <asraa@google.com> Signed-off-by: asraa <asraa@google.com>

…oxy#11101)" This reverts commit 3550a7a. Signed-off-by: Asra Ali <asraa@google.com>

#11194) This reverts commit 3550a7a. Signed-off-by: Asra Ali <asraa@google.com>

Commit Message: Remove all throw statements from H/1 codec This change removed all uses of C++ exceptions from H/1 codec. I modeled the flow after Yan's H/2 work (#11575). Codec status are set in uniform helper methods. This is the only change from the previous PR (#11101), besides merging newer exceptions. This change replaces all throw statements with a return of corresponding error Status and adds plumbing to return the status to codec callers. The dispatch() method returns the encountered error to the caller, which will be handled accordingly. The calls to the RequestEncoder::encodeHeaders() NOT called from dispatch() method will RELEASE_ASSERT if an error code is returned. This does not alter the existing behavior of abnormally terminating the process, just the method of termination: RELEASE_ASSERT vs uncaught exception. Risk Level: High (Codec changes) Signed-off-by: Asra Ali <asraa@google.com>

remove most h/1 throws

97d0fce

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

asraa commented May 7, 2020

View reviewed changes

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

add missing assert

ff70bb4

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

asraa requested a review from yanavlasov May 7, 2020 17:18

alyssawilk assigned yanavlasov May 7, 2020

mattklein123 self-assigned this May 7, 2020

jmarantz reviewed May 7, 2020

View reviewed changes

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

jmarantz reviewed May 8, 2020

View reviewed changes

asraa added 2 commits May 8, 2020 09:33

address comment

402b92d

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

use an enum

f162b35

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

move parser codes into the connectionimpl

0e6029e

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

mattklein123 previously approved these changes May 12, 2020

View reviewed changes

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

mattklein123 assigned jmarantz and unassigned mattklein123 May 12, 2020

jmarantz reviewed May 13, 2020

View reviewed changes

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

add comment

5677cf4

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

asraa dismissed mattklein123’s stale review via 5677cf4 May 13, 2020 14:02

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

jmarantz approved these changes May 13, 2020

View reviewed changes

jmarantz merged commit 3550a7a into envoyproxy:master May 14, 2020

asraa added a commit that referenced this pull request May 14, 2020

Revert "[http] Remove exceptions from HTTP/1 codec callbacks (#11101)"

b0497e1

This reverts commit 3550a7a.

asraa mentioned this pull request May 14, 2020

Revert "[http] Remove exceptions from HTTP/1 codec callbacks" #11193

Closed

asraa added a commit to asraa/envoy that referenced this pull request May 14, 2020

Revert "[http] Remove exceptions from HTTP/1 codec callbacks (envoypr…

b44da63

…oxy#11101)" This reverts commit 3550a7a. Signed-off-by: Asra Ali <asraa@google.com> Signed-off-by: asraa <asraa@google.com>

asraa added a commit to asraa/envoy that referenced this pull request May 14, 2020

Revert "[http] Remove exceptions from HTTP/1 codec callbacks (envoypr…

1674752

…oxy#11101)" This reverts commit 3550a7a. Signed-off-by: Asra Ali <asraa@google.com>

asraa mentioned this pull request May 14, 2020

Revert "[http] Remove exceptions from HTTP/1 codec callbacks (#11101)" #11194

Merged

asraa added a commit that referenced this pull request May 15, 2020

Revert "[http] Remove exceptions from HTTP/1 codec callbacks (#11101)" (

5083a9a

#11194) This reverts commit 3550a7a. Signed-off-by: Asra Ali <asraa@google.com>

This was referenced Jun 23, 2020

http:: sending http/1.1 errors via HCM #11714

Merged

http1: remove exceptions from H/1 codec #11778

Merged


		bool resetStreamCalled() { return reset_stream_called_; }

		// The following define special return values for http_parser callbacks.

Conversation

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

Uh oh!

Uh oh!

Uh oh!

mattklein123 commented May 7, 2020

Uh oh!

asraa commented May 7, 2020

Uh oh!

mattklein123 commented May 7, 2020

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!

Choose a reason for hiding this comment

Uh oh!

asraa May 8, 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!

alyssawilk commented May 11, 2020 via email

Uh oh!

mattklein123 commented May 12, 2020

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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

jmarantz commented May 13, 2020

Uh oh!

jmarantz commented May 13, 2020

Uh oh!

azure-pipelines bot commented May 13, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

asraa commented May 7, 2020 •

edited

Loading

asraa May 8, 2020 •

edited

Loading