Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

http1: remove exceptions from H/1 codec #11778

Merged
merged 21 commits into from Aug 10, 2020

Conversation

asraa
Copy link
Contributor

@asraa asraa commented Jun 26, 2020

Commit Message:
Remove all throw statements from H/1 codec

Additional Description:
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.

One of these was significant, that is the work done in sendProtocolError. This may run onMessageBeginBase(), which may throw, so all calls to sendProtocolError are wrapped in RETURN_IF_ERROR. I was dubious about the sendLocalReply inside this, but that calls ResponseEncoder::encodeHeaders(), which is safe.

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)
Testing: Unit Tests
Docs Changes: N/A
Release Notes: N/A
Part of: #10878

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

Signed-off-by: Asra Ali <asraa@google.com>
@asraa
Copy link
Contributor Author

asraa commented Jun 26, 2020

/cc @yanavlasov

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

@antoniovicente antoniovicente left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like exception removal really is around the corner. That's really exciting.

What additional changes are needed to move this PR out of draft form?

source/common/http/http1/codec_impl.h Outdated Show resolved Hide resolved
source/common/http/http1/codec_impl.cc Show resolved Hide resolved
source/common/http/http1/codec_impl.cc Show resolved Hide resolved
source/common/http/http1/codec_impl.cc Outdated Show resolved Hide resolved
source/common/http/http1/codec_impl.cc Show resolved Hide resolved
source/common/http/http1/codec_impl.cc Show resolved Hide resolved
Signed-off-by: Asra Ali <asraa@google.com>
@asraa
Copy link
Contributor Author

asraa commented Jun 30, 2020

I can move out of draft mode -- but this won't be ready to merge until we split the codecs to legacy versions. #10591

^ That needs to basically be sync-ed every day (every codec change). We probably just want a target merge date for that PR, because it is taking way too much time for me to merge every couple days.

Signed-off-by: Asra Ali <asraa@google.com>
@antoniovicente
Copy link
Contributor

I can move out of draft mode -- but this won't be ready to merge until we split the codecs to legacy versions. #10591

^ That needs to basically be sync-ed every day (every codec change). We probably just want a target merge date for that PR, because it is taking way too much time for me to merge every couple days.

Yeah... it would be good to merge that prerequisite PR. At this point we may want to wait until after the 1.16 branch split.

@stale
Copy link

stale bot commented Jul 11, 2020

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!

@stale stale bot added the stale stalebot believes this issue/PR has not been touched recently label Jul 11, 2020
@stale
Copy link

stale bot commented Jul 19, 2020

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!

@stale stale bot closed this Jul 19, 2020
@asraa asraa added no stalebot Disables stalebot from closing an issue and removed stale stalebot believes this issue/PR has not been touched recently labels Jul 20, 2020
@asraa asraa reopened this Jul 20, 2020
@asraa asraa marked this pull request as ready for review July 20, 2020 20:02
@asraa
Copy link
Contributor Author

asraa commented Jul 21, 2020

This is ready for review (and actual merge, since the split is ready)

Signed-off-by: Asra Ali <asraa@google.com>
Signed-off-by: Asra Ali <asraa@google.com>
Signed-off-by: Asra Ali <asraa@google.com>
Signed-off-by: Asra Ali <asraa@google.com>
@asraa
Copy link
Contributor Author

asraa commented Aug 3, 2020

Friendly ping? PTAL

@mattklein123 mattklein123 self-assigned this Aug 3, 2020
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM to start testing with one small comment. Thanks for working on this!

/wait

Comment on lines 566 to 567
// Make sure that dispatching_ is set to false after dispatching, even when
// ConnectionImpl::dispatch throws an exception.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this comment be modified to talk about early return? I think this is why you still need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes! modified

Signed-off-by: Asra Ali <asraa@google.com>
mattklein123
mattklein123 previously approved these changes Aug 3, 2020
@asraa
Copy link
Contributor Author

asraa commented Aug 4, 2020

@antoniovicente @yanavlasov any other comments? otherwise will submit and submit a PR to lint for reintroducing throws in the new codecs.

antoniovicente
antoniovicente previously approved these changes Aug 4, 2020
test/common/http/http1/codec_impl_test.cc Outdated Show resolved Hide resolved
Signed-off-by: Asra Ali <asraa@google.com>
@asraa asraa dismissed stale reviews from antoniovicente and mattklein123 via 5a302f2 August 4, 2020 16:49
@asraa asraa merged commit 2187f10 into envoyproxy:master Aug 10, 2020
@asraa asraa deleted the remove-h1-exceptions branch August 10, 2020 12:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no stalebot Disables stalebot from closing an issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants