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

Context data is lost if AsyncLogger and AsyncLoggerConfig are both used #1786

Closed
ppkarwasz opened this issue Sep 13, 2023 · 2 comments · Fixed by #1787
Closed

Context data is lost if AsyncLogger and AsyncLoggerConfig are both used #1786

ppkarwasz opened this issue Sep 13, 2023 · 2 comments · Fixed by #1787
Labels
bug Incorrect, unexpected, or unintended behavior of existing code

Comments

@ppkarwasz
Copy link
Contributor

Description

If the user uses both an AsyncLoggerContext (which makes all loggers to be of type AsyncLogger) and AsyncLoggerConfig, a race condition can occur, which causes context data to be lost.

Details

If both AsyncLogger and AsyncLoggerConfig are used together (which is no advised, but possible and there is a test for it):

  • AsyncLoggerConfig#log is called from the main disruptor thread with a RingBufferLogEvent argument,
  • AsyncLoggerConfig calls RingBufferLogEvent#createMemento() before sending the event to its own disruptor thread. However this method does not make a copy of the context data, nor does it mark the data as "frozen",
  • A RingBufferLogEvent#clear() on the main disruptor thread clears the context data. If the disruptor thread of AsyncLoggerConfig didn't log the message yet, it will log it without context data.
@ppkarwasz ppkarwasz added the bug Incorrect, unexpected, or unintended behavior of existing code label Sep 13, 2023
@ppkarwasz
Copy link
Contributor Author

This issues causes the "flakiness" AsyncLoggerThreadContextCopyOnWriteTest and related tests from #1568.

As it turns out, the tests were not "flaky", but pointed out a real problem.

@ppkarwasz ppkarwasz added the STF-Milestones Milestones funded by the Sovereign Tech Fund label Sep 14, 2023
ppkarwasz added a commit that referenced this issue Sep 18, 2023
This commit modernizes async thread context tests and copies the context
map `initializeBuilder()`, which solves #1786 for the 3.x codebase.
@jvz
Copy link
Member

jvz commented Sep 26, 2023

That's the fun thing about flaky tests; sometimes they flake because they don't assert the right thing, and sometimes they flake because they don't set things up such that what they're asserting is deterministic. Nice find!

@vy vy removed the STF-Milestones Milestones funded by the Sovereign Tech Fund label May 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Incorrect, unexpected, or unintended behavior of existing code
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants