[WIP]: Split dns resolver implementation for main thread and filter by chaoqin-li1123 · Pull Request #15952 · envoyproxy/envoy

chaoqin-li1123 · 2021-04-13T15:41:32Z

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Commit Message: This is part of the effort to remove C++ exception from data plane by adding assertion that the code is executed in main thread when an exception is caught.(#14320) Currently we can't add main thread assertion because dns resolver is used in both main thread and dns filter. This pr split the implementation for main thread and filter so that we can add main thread assertion to the exception catching code in core codebase.
Additional Description:
Risk Level: low
Testing: modify dns filter test to reflect the new change
Docs Changes:none
Release Notes:none
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

htuch · 2021-04-13T22:14:31Z

/assign-from @envoyproxy/first-pass-reviewers

repokitteh-read-only · 2021-04-13T22:14:35Z

@envoyproxy/first-pass-reviewers assignee is @antoniovicente

🐱

Caused by: a #15952 (comment) was created by @htuch.

see: more, trace.

antoniovicente

Thanks for helping our understanding of how Envoy uses exceptions. I'm a bit worried about the code duplication introduced by this PR, wondering what we can do to reduce the amount of duplicated code.

antoniovicente · 2021-04-13T22:36:47Z

source/common/network/dns_impl.cc

      //  and executed in both main thread and worker thread. Maybe split the code for filter and
      //  main thread.
-      TRY_NEEDS_AUDIT { callback_(resolution_status, std::move(address_list)); }
+      TRY_ASSERT_MAIN_THREAD { callback_(resolution_status, std::move(address_list)); }


Having 2 copies of the code that handles DNS resolutions just because of this try/catch issue doesn't seem good.

The use of dispatcher_ post to rethrow the exception in a different thread seems even stranger. Who would catch the posted exception?

Can we require that the callback passed in to dns_impl never throw?

+1 I think it's a non-starter to copy this code. Either we need to share all of it, or we should just change the semantics of exception handling within the callback per @antoniovicente (though this may be difficult).

rethrow the exception in a different thread seems even stranger.

It's posting to the same thread. It's an artifact of not throwing through the C code. I agree, it's not good.

Thanks, this is only a draft, I am not satisfied with the code duplication myself. l agree it is better to handle it without copying the code.

Do we know which callback throws?

The reason why we post to the same thread is because of #4307, c_ares as a c library has exception unsafe code, so exception throwing need to be executed in another call stack in the same thread.

The callbeck can be in logical_dns_cluster or strict_dns_cluster
Network::Utility::getAddressWithPort may throw. I haven't found any other exception throwing code yet.

dmitri-d · 2021-04-13T22:53:24Z

source/extensions/filters/udp/dns_filter/dns_impl.cc

@@ -0,0 +1,311 @@
+#include "extensions/filters/udp/dns_filter/dns_impl.h"


This is largely the same as implementation in source/common/network/dns_impl.cc (looks like the main differences are an addition of DnsResolverImpl::createDnsResolver() and removal of a main thread assert in onAresGetAddrInfoCallback). Couldn't you most of the implementation in a base class and use subclasses in common/network package and here?

yeah, that makes sense, onAresGetAddrInfoCallback is not a virtual method to override though. Let me see if I can change it to use error propagation without exception.

dmitri-d · 2021-04-13T22:54:06Z

test/extensions/filters/udp/dns_filter/dns_filter_test.cc


    resolver_ = std::make_shared<Network::MockDnsResolver>();
-    ON_CALL(dispatcher_, createDnsResolver(_, _)).WillByDefault(Return(resolver_));
+    // ON_CALL(dispatcher_, createDnsResolver(_, _)).WillByDefault(Return(resolver_));


Did you forget to remove that line?

jmarantz · 2021-04-13T23:03:02Z

I think @chaoqin-li1123 meant for this to be a draft to share with me.

yanavlasov · 2021-04-15T14:43:02Z

/wait-any

chaoqin-li1123 · 2021-04-16T18:55:27Z

I plan to change the error propagation and not to use C++ exception here, int the code path I only find two places that might throw, the getAddressWithPort() function, we can change this function to return nullptr upon failure. WDYT? @mattklein123

mattklein123 · 2021-04-16T19:12:40Z

I plan to change the error propagation and not to use C++ exception here, int the code path I only find two places that might throw, the getAddressWithPort() function, we can change this function to return nullptr upon failure. WDYT? @mattklein123

I'm not sure. I think this may be much more complicated as I think callbacks can kick off more configuration loads, etc. I'm not sure if these things can throw or not. We can give it a shot, as it certainly would be optimal to remove any throwing from the callbacks.

chaoqin-li1123 · 2021-04-16T20:37:16Z

I plan to change the error propagation and not to use C++ exception here, int the code path I only find two places that might throw, the getAddressWithPort() function, we can change this function to return nullptr upon failure. WDYT? @mattklein123

I'm not sure. I think this may be much more complicated as I think callbacks can kick off more configuration loads, etc. I'm not sure if these things can throw or not. We can give it a shot, as it certainly would be optimal to remove any throwing from the callbacks.

let's give it a shot.

chaoqin-li1123 · 2021-04-22T15:29:16Z

Closed this pull request as the try catch can be removed by refactoring the constructor of Ipv4Instance and Ipv6instance to be not throw.(#16122)

chaoqin-li1123 added 2 commits April 13, 2021 15:07

fix test

9215219

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

fix format

f5813ad

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

chaoqin-li1123 requested a review from mattklein123 as a code owner April 13, 2021 15:41

chaoqin-li1123 added 3 commits April 13, 2021 18:07

fix format

0127c7e

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

fix format

e8aad9e

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

change target visibility

c2fb9d6

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

htuch assigned mattklein123 Apr 13, 2021

repokitteh-read-only bot assigned antoniovicente Apr 13, 2021

antoniovicente assigned yanavlasov Apr 13, 2021

antoniovicente reviewed Apr 13, 2021

View reviewed changes

dmitri-d reviewed Apr 13, 2021

View reviewed changes

mattklein123 added the waiting label Apr 13, 2021

antoniovicente assigned jmarantz Apr 13, 2021

chaoqin-li1123 changed the title ~~Split dns resolver implementation for main thread and filter~~ [WIP]: Split dns resolver implementation for main thread and filter Apr 14, 2021

repokitteh-read-only bot added waiting:any and removed waiting waiting:any labels Apr 15, 2021

mattklein123 removed their assignment Apr 16, 2021

antoniovicente removed their assignment Apr 22, 2021

antoniovicente added the waiting label Apr 22, 2021

chaoqin-li1123 closed this Apr 22, 2021

chaoqin-li1123 deleted the split_dns_impl branch June 8, 2021 14:16

		@@ -0,0 +1,311 @@
		#include "extensions/filters/udp/dns_filter/dns_impl.h"

Conversation

chaoqin-li1123 commented Apr 13, 2021

Uh oh!

htuch commented Apr 13, 2021

Uh oh!

repokitteh-read-only bot commented Apr 13, 2021

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmarantz commented Apr 13, 2021

Uh oh!

yanavlasov commented Apr 15, 2021

Uh oh!

chaoqin-li1123 commented Apr 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattklein123 commented Apr 16, 2021

Uh oh!

chaoqin-li1123 commented Apr 16, 2021

Uh oh!

chaoqin-li1123 commented Apr 22, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

chaoqin-li1123 commented Apr 16, 2021 •

edited

Loading