Skip to content

Fix data race on LoggerContext.size (#1038) - #1055

Open
seonwooj0810 wants to merge 1 commit into
qos-ch:masterfrom
seonwooj0810:fix/issue-1038-loggercontext-size-datarace
Open

Fix data race on LoggerContext.size (#1038)#1055
seonwooj0810 wants to merge 1 commit into
qos-ch:masterfrom
seonwooj0810:fix/issue-1038-loggercontext-size-datarace

Conversation

@seonwooj0810

Copy link
Copy Markdown
Contributor

Fixes #1038

Root cause

LoggerContext.getLogger(String) creates each new logger under the monitor of its parent node (synchronized (logger)). Concurrent callers creating loggers below different parents therefore hold different locks, yet they all update the same counter via incSize(), which performed an unsynchronized size++. That read-modify-write races, so increments are lost under contention and size() reports fewer loggers than actually exist. This is exactly the ThreadSanitizer data race reported in the issue.

Change

Make the counter an AtomicInteger and use incrementAndGet() / get(), so the count is maintained atomically regardless of which parent monitor a thread happens to hold. No locking is added to the hot path.

Test evidence

Added LoggerContextTest#concurrentGetLoggerKeepsSizeConsistent: 16 threads each create 500 loggers under their own distinct parent (so their incSize() calls hold distinct monitors, maximising contention), then the test asserts size() matches the real logger count.

  • Before the fix the test fails, e.g. expected: <8017> but was: <7970> (lost increments).
  • After the fix it passes; the full LoggerContextTest (17 tests) is green.

Verification done: built and ran mvn -pl logback-classic -am -Dtest=LoggerContextTest test on the branch (all pass), and confirmed the new test fails on unpatched master.

Loggers are created in getLogger(String) under the monitor of their
parent node, so concurrent creations below distinct parents run under
different locks. incSize() then performed an unsynchronized size++ on
the shared counter, so increments were lost under contention and
size() could report fewer loggers than actually exist (reported via
ThreadSanitizer).

Make the counter an AtomicInteger so it is updated atomically
regardless of which parent monitor is held.

Signed-off-by: seonwoo_jung <79202163+seonwooj0810@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Data race on LoggerContext.size

1 participant