Skip to content

logging: drop read mutexes on log calls #41149

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 20 commits into
base: main
Choose a base branch
from
Open

logging: drop read mutexes on log calls #41149

wants to merge 20 commits into from

Conversation

kyessenov
Copy link
Contributor

@kyessenov kyessenov commented Sep 19, 2025 •
edited
Loading

Change-Id: Ia077d70fec5ec2a48d7c01ef2f0701dd58fd2211

Commit Message: Logging in production should be setup early before threading starts. That means the locks in the logger are superficial and only were needed in tests. Instead, change tests to setup a custom test log sink early via global environment.
Risk Level: low, locks are not meaningful in prod.
Testing: passed

@kyessenov
logging: drop read mutexes
38c1c78 
Change-Id: Ia077d70fec5ec2a48d7c01ef2f0701dd58fd2211
Signed-off-by: Kuat Yessenov <[email protected]>
@kyessenov kyessenov changed the title logging: drop read mutexes WIP logging: drop read mutexes Sep 19, 2025
@kyessenov
fix tests
21299f9 
Change-Id: I9e80ad4e33c4a1a35201417d8cc3746913019368
Signed-off-by: Kuat Yessenov <[email protected]>
@kyessenov kyessenov changed the title WIP logging: drop read mutexes logging: drop read mutexes on log calls Sep 22, 2025
@kyessenov
Copy link
Contributor Author

/assign-from @envoyproxy/senior-maintainers

@repokitteh-read-only RepoKitteh - Read Only
Copy link

@envoyproxy/senior-maintainers assignee is @wbpcode

🐱

Caused by: a #41149 (comment) was created by @kyessenov.

see: more, trace.

@kyessenov
fix mobile
2a83239 
Change-Id: Ieb1f2fc8a9eae719b61ae4b722fa552d286d4f8c
Signed-off-by: Kuat Yessenov <[email protected]>
@kyessenov
more macros
f20b955 
Change-Id: I0f706b80249619aebae21de47d33dbc32cb02251
Signed-off-by: Kuat Yessenov <[email protected]>
@kyessenov
quiche
836746a 
Change-Id: I1a39fd7783bf486897c8f98df5ecc416d457c110
Signed-off-by: Kuat Yessenov <[email protected]>
@kyessenov
fix quiche
df2d0b3 
Change-Id: Iec3d77f66491fad7124c5c3d4ff7328268239722
Signed-off-by: Kuat Yessenov <[email protected]>
@kyessenov
Copy link
Contributor Author

It's not pretty to add SETUP_LOG_RECORDER - but my thinking is that we should not make this test pattern convenient to use. Metrics or other observable signals should be used instead.

@kyessenov
more quiche
a2034e5 
Change-Id: Ia2150e941051e5ff94fdfcdae2ec7a904d70b8d7
Signed-off-by: Kuat Yessenov <[email protected]>
@kyessenov
restore formatter
745eb7f 
Change-Id: I305d4bb37364a83235091844a8ba1b84b9fc29a7
Signed-off-by: Kuat Yessenov <[email protected]>
@kyessenov
try fixing mobile
c7a733f 
Change-Id: I3186d2d6143baa83e6ae9ae3d1ea2c85ee59ca1d
Signed-off-by: Kuat Yessenov <[email protected]>
@kyessenov
fix ci
413dbf1 
Change-Id: I6cbec0cb8f8af6f249a4518a51bfebed3d703092
Signed-off-by: Kuat Yessenov <[email protected]>
@kyessenov
fix mobile
97369cd 
Change-Id: I891d2cc07ae9aeb03cea85ecac963d683881c61a
Signed-off-by: Kuat Yessenov <[email protected]>
@kyessenov
Revert "fix mobile"
1faefd9 
This reverts commit 97369cd.

Revert "fix ci"

This reverts commit 413dbf1.

Revert "try fixing mobile"

This reverts commit c7a733f.

Change-Id: Ic3f9eecae1728a75afca478bf98b1686b8ad8059
Signed-off-by: Kuat Yessenov <[email protected]>
@kyessenov
Merge remote-tracking branch 'upstream/main' into drop_sync_mutex
d93edd6 
Change-Id: I80a5ac2d58d0f1133e4c577b5d05fae86e02cde1
@kyessenov
common logger
47a4ed0 
Change-Id: I03462e425bd0dace79c8b38f41ce254a5245eaaf
Signed-off-by: Kuat Yessenov <[email protected]>
@kyessenov
Merge remote-tracking branch 'upstream/main' into drop_sync_mutex
861cb7e
@kyessenov
fix tsan on should escape
a54ee9b 
Change-Id: Ic4cd4af5609746e9fc9fdee3596bcb7f3bcb300b
Signed-off-by: Kuat Yessenov <[email protected]>
@kyessenov
Copy link
Contributor Author

Need to update mobile LOG_CONTAINS tests - the rest should be ready for review.

@kyessenov
merge conflict
e10952e 
Change-Id: I56bcfa766dc1a978255f140dd186fb06ff6d0712
Signed-off-by: Kuat Yessenov <[email protected]>
@kyessenov
fix mobile test
8f2093b 
Change-Id: Ib3fa1fa8ed8cdba3204bd7a2dfa4f1f2d36e10ba
Signed-off-by: Kuat Yessenov <[email protected]>
@kyessenov
fix format
c6da6c7 
Change-Id: I77ff49849039ca33dfa8d813d02c609cfab8572a
Signed-off-by: Kuat Yessenov <[email protected]>
@kyessenov
Copy link
Contributor Author

Mobile/ASAN error seems to be some problem in CI. I can't reproduce it locally on LLVM 19:

bazel test --config=clang-asan --runs_per_test=100  test/common/integration:client_integration_test
PASSED

@RyanTheOptimist have you seen it before? Looks like LeakSanitizer is detecting new InternalEngine as some kind of a leak, and not sure how to stop it.

@ravenblackx
Copy link
Contributor

@RyanTheOptimist have you seen it before? Looks like LeakSanitizer is detecting new InternalEngine as some kind of a leak, and not sure how to stop it.

Looks like LeakSanitizer is correct, in that that bare pointer ends up in the member engine_ of Engine, and is never deleted.

I would say ideally engine_ should be a unique_ptr, and Engine's constructor should also receive a moved unique_ptr to populate it. I don't see any reason for there to be so many persistent bare pointers in this code.

@kyessenov
Copy link
Contributor Author

Sigh, yeah, it's leaky by design, but I don't understand why ASAN started barking all of a sudden and why it only happens on CI. (Ryan is out) @abeyad can we wrap new InternalEngine into unique_ptr or does that cause some other issues?

@kyessenov
try fixing mobile asan
07cb324 
Change-Id: I76c05c0d4fbf0de0e8983e21f4f82975ecefc652
Signed-off-by: Kuat Yessenov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattklein123 mattklein123 Awaiting requested review from mattklein123 mattklein123 is a code owner

@RyanTheOptimist RyanTheOptimist Awaiting requested review from RyanTheOptimist RyanTheOptimist is a code owner

@abeyad abeyad Awaiting requested review from abeyad abeyad is a code owner

@fredyw fredyw Awaiting requested review from fredyw fredyw is a code owner

@wbpcode wbpcode Awaiting requested review from wbpcode wbpcode is a code owner

@ggreenway ggreenway Awaiting requested review from ggreenway ggreenway is a code owner

@tonya11en tonya11en Awaiting requested review from tonya11en tonya11en is a code owner

@nezdolik nezdolik Awaiting requested review from nezdolik nezdolik is a code owner

@tyxia tyxia Awaiting requested review from tyxia tyxia is a code owner

@yanavlasov yanavlasov Awaiting requested review from yanavlasov yanavlasov is a code owner

@danzh2010 danzh2010 Awaiting requested review from danzh2010 danzh2010 is a code owner

@botengyao botengyao Awaiting requested review from botengyao botengyao is a code owner

@ravenblackx ravenblackx Awaiting requested review from ravenblackx ravenblackx is a code owner

@zuercher zuercher Awaiting requested review from zuercher zuercher is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

@wbpcode wbpcode

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kyessenov @ravenblackx @wbpcode