Repository navigation
Conversation
When the ring buffer is full, `AsyncLogger` may log an event in the current thread. That path dropped the location computed or supplied by the caller, so `LoggerConfig` computed it again, and a location supplied with `LogBuilder.withLocation()` was replaced. - Pass the location to `logMessageInCurrentThread()` and log through the location-aware reliability strategy. - Resolve a missing location once, in the two entry points of the logger, and merge the translator methods with and without a location. - Add benchmarks for log builders with and without a precomputed location. Assisted-By: Claude Opus 5.5 <noreply@anthropic.com>
Assisted-By: Claude Opus 5.5 <noreply@anthropic.com>
ppkarwasz
added a commit
that referenced
this pull request
Nov 9, 2026
| @Override | ||
| public void append(final LogEvent event) { | ||
| if (Thread.currentThread() == testThread) { | ||
| locations.put(event.getMessage().getFormattedMessage(), event.getSource()); |
Contributor
There was a problem hiding this comment.
The recursion branch in both handleRingBufferFull methods passes the location too, but no test reaches it. A nested call from this appender while the buffer is full goes through that branch:
locations.put(event.getMessage().getFormattedMessage(), event.getSource());
if ("explicit".equals(event.getMessage().getFormattedMessage())) {
context.getLogger(AsyncLoggerQueueFullLocationTest.class)
.atInfo()
.withLocation(NESTED_LOCATION)
.log("nested");
}plus a NESTED_LOCATION constant and "nested" in containsOnlyKeys with its own assertion.
| public void logMessage( | ||
| final String fqcn, final Level level, final Marker marker, final Message message, final Throwable thrown) { | ||
| getTranslatorType().log(fqcn, level, marker, message, thrown); | ||
| getTranslatorType().log(fqcn, calcLocationIfRequested(fqcn), level, marker, message, thrown); |
Contributor
There was a problem hiding this comment.
ExtendedLoggerWrapper reaches this entry, because its requiresLocation() is false. The async location tests all go through log(…, location, …), so this line has no test. Can AsyncLoggerNullLocationTest get a case that logs through an ExtendedLoggerWrapper and checks the source on the enqueue route?
| type="fixed"> | ||
| <issue id="4470" link="https://github.com/apache/logging-log4j2/pull/4470"/> | ||
| <description format="asciidoc"> | ||
| Keep the caller location of events that asynchronous loggers log synchronously, because the ring buffer is full. |
Contributor
There was a problem hiding this comment.
Shorter line for the release notes:
Suggested change
| Keep the caller location of events that asynchronous loggers log synchronously, because the ring buffer is full. | |
| Keep the async logger location when the ring buffer is full. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #4340.
When the ring buffer is full,
AsyncLoggermay log an event in the current thread (theSYNCHRONOUSroute of the queue-full policy, or a recursive logging call).logMessageInCurrentThread()had no location parameter, so:AsyncLoggerwas dropped and computed again byLoggerConfig,LogBuilder.withLocation()was replaced by the computed one.Changes
logMessageInCurrentThread()receives the location and logs throughLogger.log(…, location, …), which uses the location-aware reliability strategy.log(…, location, …)andlogMessage(fqcn, …). This is the same stack depth as Compute AsyncLogger location when the caller passes null #4340.TranslatorType.log,logWithThreadLocalTranslator,initTranslator,logWithVarargTranslator) are merged: the methods without a location only differed by computing it.logWithThreadLocalTranslatorwith 34 bytes).Tests
AsyncLoggerQueueFullLocationTestfills the ring buffer, uses a queue-full policy that always returnsSYNCHRONOUSand checks the location of events logged with an explicit location, with a log builder and with a classic logging call, for both the thread-local and the vararg translator. It fails on2.x, where the explicit location is replaced.Benchmarks
AsyncLoggersLocationBenchmarkgets two new benchmarks: a log builder without a location (throughputLogBuilder) and with a precomputed location (throughputPrecomputedLocation).Comparing this PR with
2.x(16 threads, thread-local and vararg translator), the differences in the benchmarks that compute a location stay within the run-to-run noise of my machine (about ±5 %): their sign flips if the order of the runs is swapped. The benchmarks without a location and with a precomputed location are limited by the consumer thread at 16 threads and show no difference.AsyncLoggerUncachedThreadNameTestfailed in some of mytest-disruptor-4runs with this PR (it passed in the single run I did on2.x). It passes when run alone, and it sharestarget/AsyncLoggerTest.logwith other test classes that run in parallel forks (forkCount=1C), which is the likely cause.🤖 Generated with Claude Code