Only call ares_library_init()/cleanup() routines in process_wide() if ares extension used in DNS resolving. by yanjunxiang-google · Pull Request #21884 · envoyproxy/envoy

yanjunxiang-google · 2022-06-24T19:56:43Z

Only call ares_library_init()/cleanup() routines in process_wide() if ares extension is used in DNS resolving.

Signed-off-by: Yanjun Xiang yanjunxiang@google.com

Commit Message:
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

… ares extension is linked in. Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

yanjunxiang-google · 2022-06-24T19:58:17Z

/assign @jmarantz @yanavlasov @mattklein123

jmarantz · 2022-06-24T20:07:24Z

per IM I am not seeing how this can work.

yanjunxiang-google · 2022-06-24T22:20:06Z

per IM I am not seeing how this can work.

With c-ares library is an extension now, this change helps fixing a bug that if someone build Envoy with c-ares is not linked in here, :

envoy/source/extensions/extensions_build_config.bzl

Line 340 in 326b205

    
           "envoy.network.dns_resolver.cares":                "//source/extensions/network/dns_resolver/cares:config",

, the ares_library_init()/ares_library_cleanup() should not be called.

This is not an issue before since ares library is in core. It is a minor issue now since ares extension is by default included.

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

yanjunxiang-google · 2022-06-26T17:11:31Z

per IM I am not seeing how this can work.

Thanks for the comments. Please take a look the latest fix.

The latest fix will only call ares_library_init() when the createDnsResolver() is called for c-ares. And it will only call ares_library_cleanup() during ~process_wide() when c-ares createDnsResolver() is ever called.

Overall, this PR can help that if c-ares extension is not linked in, or even is linked in but never used due to configuration, ares_library_init()/ares_library_cleanup() will not be called.

jmarantz

Thanks I think this is vectoring toward the solution we discussed.

source/extensions/network/dns_resolver/cares/dns_impl.cc

jmarantz · 2022-06-26T18:25:04Z

source/extensions/network/dns_resolver/cares/dns_impl.cc

+  void cleanup() override {
+    // Cleanup c-ares library if initialized.
+    if (ares_library_initialized_) {
+      ares_library_cleanup();


Can we move this cleanup to the ares factory destructor? That would feel a little cleaner.

Can't do it in the destructor since factories are statically initialized and will race with whatever static clean-up happens in c-ares.

Makes sense. Can you comment that the destructor is not called in deterministic order as the object is statically constructed.

source/extensions/network/dns_resolver/cares/dns_impl.cc

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

jmarantz · 2022-06-27T14:09:58Z

/wait

yanavlasov

/wait-any

source/common/network/dns_resolver/dns_factory_util.cc

yanavlasov · 2022-06-27T19:52:24Z

source/extensions/network/dns_resolver/cares/dns_impl.cc

+  void cleanup() override {
+    // Cleanup c-ares library if initialized.
+    if (ares_library_initialized_) {
+      ares_library_cleanup();


Can't do it in the destructor since factories are statically initialized and will race with whatever static clean-up happens in c-ares.

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

envoy/network/dns_resolver.h

source/exe/process_wide.cc

jmarantz · 2022-06-28T01:51:37Z

source/extensions/network/dns_resolver/cares/dns_impl.cc

+  void cleanup() override {
+    // Cleanup c-ares library if initialized.
+    if (ares_library_initialized_) {
+      ares_library_cleanup();


Makes sense. Can you comment that the destructor is not called in deterministic order as the object is statically constructed.

source/extensions/network/dns_resolver/cares/dns_impl.cc

yanavlasov · 2022-06-28T13:53:11Z

/wait

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

envoy/network/dns_resolver.h

jmarantz · 2022-06-30T12:06:30Z

/wait

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

jmarantz

Remember to look at the format error also.

source/common/config/utility.h

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

envoy/network/dns_resolver.h

jmarantz · 2022-07-01T13:58:22Z

/wait

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

jmarantz

Looks great modulo naming.

envoy/network/dns_resolver.h

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

yanjunxiang-google · 2022-07-01T20:59:31Z

/retest

repokitteh-read-only · 2022-07-01T20:59:35Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #21884 (comment) was created by @yanjunxiang-google.

see: more, trace.

yanjunxiang-google · 2022-07-01T22:10:11Z

Thanks jmarantz@ for the comments and approval!

…`. (#877) Also re-enable `text_gcc`. Fixes #872. **Details**: - `ServiceImpl` creates `Envoy::Logger::Context` which requires `Envoy::Thread::MutexBasicLockable`. - The `Envoy::Thread::MutexBasicLockable` was constructed after the `Envoy::Logger::Context`, so `Envoy::Logger::Context` became invalid during destruction of `ServiceImpl`. - this PR ensures that `Envoy::Thread::MutexBasicLockable` lives longer than `Envoy::Logger::Context`. - Fixing the same problem in the construction of `RequestSourceServiceImpl`. **Reason for `test_gcc` failure:** - envoyproxy/envoy#21884 triggered this bug, because it added a logging call initiated indirectly from the destructor of `Envoy::ProcessWide`. - This logging call happened during the destruction of `ServiceImpl` when the `Envoy::Thread::MutexBasicLockable` instance was already destroyed. - As a result the PURE virtual methods on its parent object `Envoy::Thread::BasicLockable` were called. - `test_gcc` reported it because it places a special function meant to detect this behavior in the `vtbl` of unimplemented pure virtual methods. Signed-off-by: Jakub Sobon <mumak@google.com>

Only call ares_library_init()/cleanup() routines in process_wide() if…

d20706b

… ares extension is linked in. Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

yanjunxiang-google requested review from mattklein123 and yanavlasov as code owners June 24, 2022 19:56

repokitteh-read-only bot assigned jmarantz, mattklein123 and yanavlasov Jun 24, 2022

yanjunxiang-google added 3 commits June 25, 2022 12:19

remove ares dependancy from process_wide

f9751b7

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

fix format error

c4aaa4b

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

addressing comments

95d2216

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

jmarantz reviewed Jun 26, 2022

View reviewed changes

yanjunxiang-google added 2 commits June 26, 2022 19:36

addressing comments

d425d91

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

fixing CI error

d22cb61

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

repokitteh-read-only bot added the waiting label Jun 27, 2022

yanjunxiang-google changed the title ~~Only call ares_library_init()/cleanup() routines in process_wide() if ares extension linked in~~ Only call ares_library_init()/cleanup() routines in process_wide() if ares extension used in DNS resolving. Jun 27, 2022

yanavlasov requested changes Jun 27, 2022

View reviewed changes

repokitteh-read-only bot added waiting:any and removed waiting labels Jun 27, 2022

adding mutex to protect ares_library_init() operation race

f37c515

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

repokitteh-read-only bot removed the waiting:any label Jun 27, 2022

jmarantz reviewed Jun 28, 2022

View reviewed changes

repokitteh-read-only bot added the waiting label Jun 28, 2022

addressing comments

838aa9c

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

repokitteh-read-only bot removed the waiting label Jun 28, 2022

jmarantz reviewed Jun 28, 2022

View reviewed changes

envoy/network/dns_resolver.h Outdated Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Jun 30, 2022

make the factory init/terminate generic

09f15d7

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

repokitteh-read-only bot removed the waiting label Jun 30, 2022

jmarantz reviewed Jun 30, 2022

View reviewed changes

source/common/config/utility.h Outdated Show resolved Hide resolved

addressing comments

6a26ee9

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

jmarantz reviewed Jul 1, 2022

View reviewed changes

envoy/network/dns_resolver.h Show resolved Hide resolved

envoy/network/dns_resolver.h Outdated Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Jul 1, 2022

addressing comments

76faa30

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

repokitteh-read-only bot removed the waiting label Jul 1, 2022

jmarantz reviewed Jul 1, 2022

View reviewed changes

envoy/network/dns_resolver.h Outdated Show resolved Hide resolved

mattklein123 added the waiting label Jul 1, 2022

addressing comments

8f2cb43

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

repokitteh-read-only bot removed the waiting label Jul 1, 2022

yanjunxiang-google added 2 commits July 1, 2022 16:43

fixing CI format error

10a71ed

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

addressing comments

629e580

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

jmarantz approved these changes Jul 1, 2022

View reviewed changes

mattklein123 approved these changes Jul 6, 2022

View reviewed changes

mattklein123 merged commit 9b424e6 into envoyproxy:main Jul 6, 2022

yanjunxiang-google deleted the ares-cleanup branch July 6, 2022 16:39

jiajunye mentioned this pull request Jul 15, 2022

Properly terminate DnsResolverFactory envoyproxy/nighthawk#876

Closed

mum4k mentioned this pull request Jul 16, 2022

Fix construction order of ServiceImpl and RequestSourceServiceImpl. envoyproxy/nighthawk#877

Merged

yanjunxiang-google mentioned this pull request Jul 21, 2022

Cleanup ares_library_init from Envoy ProcessWide::ProcessWide() #18713

Closed

Conversation

yanjunxiang-google commented Jun 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yanjunxiang-google commented Jun 24, 2022

Uh oh!

jmarantz commented Jun 24, 2022

Uh oh!

yanjunxiang-google commented Jun 24, 2022

Uh oh!

yanjunxiang-google commented Jun 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jmarantz Jun 26, 2022

Choose a reason for hiding this comment

Uh oh!

yanavlasov Jun 27, 2022

Choose a reason for hiding this comment

Uh oh!

jmarantz Jun 28, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jmarantz commented Jun 27, 2022

Uh oh!

yanavlasov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yanavlasov Jun 27, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jmarantz Jun 28, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

yanavlasov commented Jun 28, 2022

Uh oh!

Uh oh!

jmarantz commented Jun 30, 2022

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jmarantz commented Jul 1, 2022

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yanjunxiang-google commented Jul 1, 2022

Uh oh!

repokitteh-read-only bot commented Jul 1, 2022

Uh oh!

yanjunxiang-google commented Jul 1, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

yanjunxiang-google commented Jun 24, 2022 •

edited

Loading

yanjunxiang-google commented Jun 26, 2022 •

edited

Loading