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

support exception-free build #27412

Open
alyssawilk opened this issue May 15, 2023 · 7 comments
Open

support exception-free build #27412

alyssawilk opened this issue May 15, 2023 · 7 comments
Assignees
Labels
enhancement Feature requests. Not bugs or questions. no stalebot Disables stalebot from closing an issue

Comments

@alyssawilk
Copy link
Contributor

We've slowly whittled down exceptions from the Envoy data plane but even with the work we've done on #14320 there are still some exceptions in the data plane, and it's hard for new users of Envoy to know where to use exceptions and where to use other error handling.

I'd like to move towards Envoy being able to build without exceptions entirely, primarily for envoyproxy/envoy-mobile#176 but also due to continued concerns in-house with performance and safety of exceptions.

I have a proof-of-concept "no exceptions" build of Envoy Mobile, which currently replaces "throw" with "throw or panic" in about 60 files (down from ~120, and envoy mobile panics on config parse fail in any case), and straight removal of catch macros. I'd like to

  1. disallow new core code files from throwing exceptions (which we can add to iff necessary)
  2. replace existing catch calls with CATCH_EXCEPTION_MAIN_THREAD{} which can compile out in no-exceptions build
  3. slowly replace the throwEnvoyExceptionOrPanic call sites with statusOr returns boiling up to base server creation
  4. as each plug-in gets a "throw free" creation mode, disallow new extensions of that type from throwing on creation

we'll probably have to keep the TRY/CATCH macros in Envoy code perpetually, but it'd be really fantastic to get at least core code building without it, and maybe some of the folks who are eager to do clean up work can work on moving older extensions to statusor mode.

@alyssawilk alyssawilk added the enhancement Feature requests. Not bugs or questions. label May 15, 2023
@yanavlasov
Copy link
Contributor

This will solve a lot of internal problems, trying to mix exception enabled Envoy code with exception disabled internal code. However it is going to a massive refactor.

@htuch
Copy link
Member

htuch commented May 15, 2023

I agree on the positives. The main challenge in core code is that it is a very common pattern to use exceptions during config ingestion for validation. There is no reason this code could not eventually move to an exception free model, but it would require a significant effort to switch to a pattern like statusOr across the board. Hopefully someone has the cycles for this work.

Will we also ask extensions to move away from exceptions once core is exception free? One concern is that currently exceptions are an "invisible" (i.e. not observable at compile time) part of the extension API interface.

@alyssawilk
Copy link
Contributor Author

they're invisible by API but if you try to build fnoexceptions and you've got a lingering throw around the compiler will 100% catch it for you. As I've discovered a dozen times over the last month merging my no_exception branch =P

Should be pretty easy to switch the E-M build to no exceptions, then hopefully (but who knows) some day the core build. I don't think it's worth ever switching contrib, or asking folks downstream of Envoy to rework. it can be a permanent bazel option to compile out or in the TRY/CATCH macros

@adisuissa
Copy link
Contributor

It is a non-trivial amount of work, but if there are benefits, then it may be a worthy cause.

I suggest adding files (maybe even directories) to this list whenever that file is clean.

@alyssawilk
Copy link
Contributor Author

cc @paul-r-gall re 285574162 we can only dream....

alyssawilk added a commit that referenced this issue Jun 7, 2023
The new build option simply compiles out all try/catch code, while leaving in the exceptions. This can not yet be successfully used as it turns up fno-exceptions which chokes on throw statements. This is by design as if we compiled out throw as well, config failures would be fatal instead of gracefully handled.

Risk Level: low
Testing: manual testing
Docs Changes: n/a
Release Notes: n/a
Part of #27412

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
alyssawilk added a commit that referenced this issue Jun 15, 2023
#27987)

also fixing a bug in multi-catch in the main build (not yet used) and replacing two catch calls with CATCH macros while I'm in here

Risk Level: low
Testing: n/a
Docs Changes: n/a
Release Notes: n/a
Part of envoyproxy/envoy-mobile#176 #27412
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
asheryerm pushed a commit to asheryerm/envoy that referenced this issue Jul 5, 2023
…roxy#27811)

The new build option simply compiles out all try/catch code, while leaving in the exceptions. This can not yet be successfully used as it turns up fno-exceptions which chokes on throw statements. This is by design as if we compiled out throw as well, config failures would be fatal instead of gracefully handled.

Risk Level: low
Testing: manual testing
Docs Changes: n/a
Release Notes: n/a
Part of envoyproxy#27412

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: asheryer <asheryer@amazon.com>
asheryerm pushed a commit to asheryerm/envoy that referenced this issue Jul 5, 2023
envoyproxy#27987)

also fixing a bug in multi-catch in the main build (not yet used) and replacing two catch calls with CATCH macros while I'm in here

Risk Level: low
Testing: n/a
Docs Changes: n/a
Release Notes: n/a
Part of envoyproxy/envoy-mobile#176 envoyproxy#27412
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: asheryer <asheryer@amazon.com>
@github-actions
Copy link

github-actions bot commented Jul 5, 2023

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Jul 5, 2023
reskin89 pushed a commit to reskin89/envoy that referenced this issue Jul 11, 2023
…roxy#27811)

The new build option simply compiles out all try/catch code, while leaving in the exceptions. This can not yet be successfully used as it turns up fno-exceptions which chokes on throw statements. This is by design as if we compiled out throw as well, config failures would be fatal instead of gracefully handled.

Risk Level: low
Testing: manual testing
Docs Changes: n/a
Release Notes: n/a
Part of envoyproxy#27412

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Ryan Eskin <ryan.eskin89@protonmail.com>
reskin89 pushed a commit to reskin89/envoy that referenced this issue Jul 11, 2023
envoyproxy#27987)

also fixing a bug in multi-catch in the main build (not yet used) and replacing two catch calls with CATCH macros while I'm in here

Risk Level: low
Testing: n/a
Docs Changes: n/a
Release Notes: n/a
Part of envoyproxy/envoy-mobile#176 envoyproxy#27412
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Ryan Eskin <ryan.eskin89@protonmail.com>
@github-actions
Copy link

This issue has been automatically closed because it has not had activity in the last 37 days. If this issue is still valid, please ping a maintainer and ask them to label it as "help wanted" or "no stalebot". Thank you for your contributions.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Jul 12, 2023
@alyssawilk alyssawilk reopened this May 30, 2024
@alyssawilk alyssawilk self-assigned this May 30, 2024
@alyssawilk alyssawilk added no stalebot Disables stalebot from closing an issue and removed stale stalebot believes this issue/PR has not been touched recently labels May 30, 2024
alyssawilk added a commit that referenced this issue Sep 18, 2024
I reviewed a PR adding exceptions to a previously exception free path
without realizing it. Narrowing the checks so we're more likely to catch
this in CI.

Risk Level: n/a (tooling only)
Testing: ci
Docs Changes: n/a
Release Notes: n/a
part of #27412

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature requests. Not bugs or questions. no stalebot Disables stalebot from closing an issue
Projects
None yet
Development

No branches or pull requests

4 participants