Add utility to check whether the execution is in main thread. #14457

chaoqin-li1123 · 2020-12-16T23:34:09Z

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

Commit Message: Use a singleton to store the main thread id and use it for assertion that some function should be executed in main thread. Prepare for [#issue 14320] (#14320) to eliminate data plane exceptions.
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]

Signed-off-by: chaoqin-li1123 <chaoqin@uchicago.edu>

…_macros Signed-off-by: chaoqin-li1123 <chaoqin@uchicago.edu>

Signed-off-by: chaoqin-li1123 <chaoqin@uchicago.edu>

alyssawilk

Thanks for adding this utility! One thought from me regarding testing/corner cases.

alyssawilk · 2021-01-05T17:08:41Z

source/common/common/thread.h

@@ -168,5 +169,19 @@ class AtomicPtr : private AtomicPtrArray<T, 1, alloc_mode> {
  T* get(const MakeObject& make_object) { return BaseClass::get(0, make_object); }
 };

+struct MainThread {
+  using MainThreadSingleton = InjectableSingleton<MainThread>;
+  bool inMainThread() const { return main_thread_id_ == std::this_thread::get_id(); }


What happens if inMainThread is called before Init? should we check for that?
I'd be great to have unit tests, for corner cases like this.

RELEASE_ASSERT(loader_ != nullptr, "InjectableSingleton used prior to initialization"); The assert in get() method in InjectableSingleton will fail in this case.

I will add some unit test to verify this condition.

I am not sure how to implement the unit test. Can we expect a RELEASE_ASSERT in gtest?

I think EXPECT_DEATH should work there?

Thanks, it work!

Signed-off-by: chaoqin-li1123 <chaoqin@uchicago.edu>

…_macros Signed-off-by: chaoqin-li1123 <chaoqin@uchicago.edu>

alyssawilk

Looks great! In future do you mind filling out the template when you submit PRs? All of the
Additional Description:
sections.

addressed

chaoqin-li1123 · 2021-01-07T15:27:43Z

Thanks! Will fill out all the description next time.

mattklein123 · 2021-01-07T17:26:33Z

source/common/common/thread.h

+  bool inMainThread() const { return main_thread_id_ == std::this_thread::get_id(); }
+  static void init() { MainThreadSingleton::initialize(new MainThread()); }
+  static void clear() {
+    free(MainThreadSingleton::getExisting());


Post review drive by: I think you have a mismatched new/free here? Should this be delete? cc @chaoqin-li1123

Sorry, it is my mistake. i will fix it in a new PR.

I guess free doesn't leak memory here because the struct doesn't have a destructor. In the fixing PR should be changed to new/delete.

* master: (48 commits) Resolve 14506, avoid libidn2 for our curl dependency (envoyproxy#14601) fix new/free mismatch in Mainthread utility (envoyproxy#14596) opencensus: deprecate Zipkin configuration. (envoyproxy#14576) upstream: clean up code location (envoyproxy#14580) configuration impl: add cast for ios compilation (envoyproxy#14590) buffer impl: add cast for android compilation (envoyproxy#14589) ratelimit: add dynamic metadata to ratelimit response (envoyproxy#14508) tcp_proxy: wait for CONNECT response before start streaming data (envoyproxy#14317) stream info: cleanup address handling (envoyproxy#14432) [deps] update upb to latest commit (envoyproxy#14582) Add utility to check whether the execution is in main thread. (envoyproxy#14457) listener: undeprecate bind_to_port (envoyproxy#14480) Fix data race in overload integration test (envoyproxy#14586) deps: update PGV (envoyproxy#14571) dependencies: update cve_scan.py for some libcurl 7.74.0 false positives. (envoyproxy#14572) Network::Connection: Add L4 crash dumping support (envoyproxy#14509) ssl: remember stat names for configured ciphers. (envoyproxy#14534) formatter: add custom date formatting to downstream cert start and end dates (envoyproxy#14502) feat(lua): allow setting response body when the upstream response body is empty (envoyproxy#14486) Generalize the gRPC access logger base classes (envoyproxy#14469) ... Signed-off-by: Michael Puncel <mpuncel@squareup.com>

chaoqin-li1123 added 25 commits December 12, 2020 20:21

macros and helper function

d77d349

Signed-off-by: chaoqin-li1123 <chaoqin@uchicago.edu>

expose main thread verification interface

3e9a5be

Signed-off-by: chaoqin-li1123 <chaoqin@uchicago.edu>

fix format

51f5549

Signed-off-by: chaoqin-li1123 <chaoqin@uchicago.edu>

Merge branch 'master' of https://github.com/envoyproxy/envoy into try…

948b973

…_macros Signed-off-by: chaoqin-li1123 <chaoqin@uchicago.edu>

use singleton

eac8710

Signed-off-by: chaoqin-li1123 <chaoqin@uchicago.edu>

assert

a40cf1f

Signed-off-by: chaoqin-li1123 <chaoqin@uchicago.edu>

fix dependency

f7abf76

Signed-off-by: chaoqin-li1123 <chaoqin@uchicago.edu>

fix dependency

2b9db0d

Signed-off-by: chaoqin-li1123 <chaoqin@uchicago.edu>

fix format

702dec0

Signed-off-by: chaoqin-li1123 <chaoqin@uchicago.edu>

use in server.cc

360087a

Signed-off-by: chaoqin-li1123 <chaoqin@uchicago.edu>

clean up

48abc00

Signed-off-by: chaoqin-li1123 <chaoqin@uchicago.edu>

move main thread utility

230ad21

Signed-off-by: chaoqin-li1123 <chaoqin@uchicago.edu>

move main thread utility

402662d

Signed-off-by: chaoqin-li1123 <chaoqin@uchicago.edu>

move main thread utility

4624a04

Signed-off-by: chaoqin-li1123 <chaoqin@uchicago.edu>

move main thread utility

079f10d

Signed-off-by: chaoqin-li1123 <chaoqin@uchicago.edu>

move main thread utility

bacbc44

Signed-off-by: chaoqin-li1123 <chaoqin@uchicago.edu>

move main thread utility

3bc0be0

Signed-off-by: chaoqin-li1123 <chaoqin@uchicago.edu>

change interface

dc708f7

Signed-off-by: chaoqin-li1123 <chaoqin@uchicago.edu>

change interface

545b891

Signed-off-by: chaoqin-li1123 <chaoqin@uchicago.edu>

change interface

41f8ccc

Signed-off-by: chaoqin-li1123 <chaoqin@uchicago.edu>

format

fc353f8

Signed-off-by: chaoqin-li1123 <chaoqin@uchicago.edu>

interface

67e7b94

Signed-off-by: chaoqin-li1123 <chaoqin@uchicago.edu>

format

d92cd8f

Signed-off-by: chaoqin-li1123 <chaoqin@uchicago.edu>

fix test

be7c2ad

Signed-off-by: chaoqin-li1123 <chaoqin@uchicago.edu>

fix test

f4d44ec

Signed-off-by: chaoqin-li1123 <chaoqin@uchicago.edu>

chaoqin-li1123 marked this pull request as draft December 17, 2020 03:56

chaoqin-li1123 changed the title ~~Add utility to check whether the execution is in main thread.~~ [WIP]: Add utility to check whether the execution is in main thread. Dec 17, 2020

chaoqin-li1123 added 3 commits December 16, 2020 22:21

fix test

c28f634

Signed-off-by: chaoqin-li1123 <chaoqin@uchicago.edu>

fix test

c62f434

Signed-off-by: chaoqin-li1123 <chaoqin@uchicago.edu>

fix test

7ad4971

Signed-off-by: chaoqin-li1123 <chaoqin@uchicago.edu>

chaoqin-li1123 added 4 commits December 23, 2020 12:19

move main thread utility to thread.h

86c21c9

Signed-off-by: chaoqin-li1123 <chaoqin@uchicago.edu>

move main thread utility to thread.h

fff8dc9

Signed-off-by: chaoqin-li1123 <chaoqin@uchicago.edu>

sync

72efaa4

Signed-off-by: chaoqin-li1123 <chaoqin@uchicago.edu>

format

8ca3633

Signed-off-by: chaoqin-li1123 <chaoqin@uchicago.edu>

chaoqin-li1123 requested review from yanavlasov and htuch December 24, 2020 01:57

yanavlasov previously approved these changes Jan 5, 2021

View reviewed changes

alyssawilk reviewed Jan 5, 2021

View reviewed changes

alyssawilk self-assigned this Jan 5, 2021

chaoqin-li1123 added 2 commits January 5, 2021 16:24

add unit test

82ce44f

Signed-off-by: chaoqin-li1123 <chaoqin@uchicago.edu>

format

c05f2cd

Signed-off-by: chaoqin-li1123 <chaoqin@uchicago.edu>

chaoqin-li1123 dismissed yanavlasov’s stale review via c05f2cd January 5, 2021 22:30

chaoqin-li1123 added 9 commits January 5, 2021 16:54

fix typo

f9b3b1b

Signed-off-by: chaoqin-li1123 <chaoqin@uchicago.edu>

fix test

0fa4c90

Signed-off-by: chaoqin-li1123 <chaoqin@uchicago.edu>

fix test

68584ce

Signed-off-by: chaoqin-li1123 <chaoqin@uchicago.edu>

fix test

6fc21a6

Signed-off-by: chaoqin-li1123 <chaoqin@uchicago.edu>

add comment

022efb5

Signed-off-by: chaoqin-li1123 <chaoqin@uchicago.edu>

add comment

dbcd0d6

Signed-off-by: chaoqin-li1123 <chaoqin@uchicago.edu>

add comment

b4918e0

Signed-off-by: chaoqin-li1123 <chaoqin@uchicago.edu>

fix spelling

48997e1

Signed-off-by: chaoqin-li1123 <chaoqin@uchicago.edu>

Merge branch 'master' of https://github.com/envoyproxy/envoy into try…

080f38e

…_macros Signed-off-by: chaoqin-li1123 <chaoqin@uchicago.edu>

alyssawilk approved these changes Jan 7, 2021

View reviewed changes

alyssawilk merged commit 40c44e5 into envoyproxy:master Jan 7, 2021

mattklein123 reviewed Jan 7, 2021

View reviewed changes

chaoqin-li1123 mentioned this pull request Jan 7, 2021

fix new/free mismatch in Mainthread utility #14596

Merged

chaoqin-li1123 mentioned this pull request Jan 12, 2021

[utility]: Change behavior of main thread verification utility #14660

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add utility to check whether the execution is in main thread. #14457

Add utility to check whether the execution is in main thread. #14457

chaoqin-li1123 commented Dec 16, 2020

alyssawilk left a comment

alyssawilk Jan 5, 2021

chaoqin-li1123 Jan 5, 2021

chaoqin-li1123 Jan 5, 2021

chaoqin-li1123 Jan 5, 2021

alyssawilk Jan 5, 2021

chaoqin-li1123 Jan 6, 2021

alyssawilk left a comment

chaoqin-li1123 commented Jan 7, 2021

mattklein123 Jan 7, 2021

chaoqin-li1123 Jan 7, 2021

chaoqin-li1123 Jan 7, 2021

Add utility to check whether the execution is in main thread. #14457

Add utility to check whether the execution is in main thread. #14457

Conversation

chaoqin-li1123 commented Dec 16, 2020

alyssawilk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alyssawilk left a comment

Choose a reason for hiding this comment

chaoqin-li1123 commented Jan 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment