Expunge stale entries in InternalLoggerRegistry #3681
Merged
Conversation
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
…so added InternalLoggerRegistryGCTest
Use ReferenceQueue to track and remove GC'd Logger WeakReferences from the registry map to prevent potential memory leaks. Includes improved unit tests.
Generated by gradle/develocity-actions |
ppkarwasz
requested changes
May 23, 2025
There was a problem hiding this comment.
@jhl221123, thank you for picking up this pull request!
The main code looks good to me overall. I do have a few suggestions regarding the unit tests:
...test/src/main/java/org/apache/logging/log4j/core/test/util/InternalLoggerRegistryGCTest.java
Outdated
Show resolved
Hide resolved
...test/src/main/java/org/apache/logging/log4j/core/test/util/InternalLoggerRegistryGCTest.java
Outdated
Show resolved
Hide resolved
...test/src/main/java/org/apache/logging/log4j/core/test/util/InternalLoggerRegistryGCTest.java
Outdated
Show resolved
Hide resolved
* Adjusted test class file name and its package location. * Used MockLogger in logger suppliers to avoid nested computeIfAbsent. * Employed a local SimpleMessageFactory instance in tests.
ppkarwasz
approved these changes
May 23, 2025
There was a problem hiding this comment.
The changes look solid—nice work! There's just one final task before we can wrap this up: could you please add a changelog entry under src/changelog/.2.x.x, as outlined in the changelog documentation?
Also, just a quick note on loggerRefByNameByMessageFactory: since it's a WeakHashMap, it doesn’t follow all the typical Map invariants. From what I can tell, you're using it safely, and I didn’t spot any issues that would lead to unexpected behavior. That said, given the sensitivity of this part of the codebase, I’d like a second pair of eyes on it.
@vy, could you take a moment to review this as well?
There was a problem hiding this comment.
Pull Request Overview
This PR implements a mechanism to expunge stale logger entries in InternalLoggerRegistry by tracking WeakReferences using a ReferenceQueue. Key changes include:
- Adding a ReferenceQueue field and an expungeStaleEntries() method.
- Updating logger retrieval and creation methods to trigger stale entry cleanup.
- Introducing unit tests to verify the expunging behavior under various GC and concurrent scenarios.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| log4j-core/src/main/java/org/apache/logging/log4j/core/util/internal/InternalLoggerRegistry.java | Added ReferenceQueue and expungeStaleEntries() method, plus calls to it in getLogger/hasLogger/computeIfAbsent |
| log4j-core-test/src/test/java/org/apache/logging/log4j/core/util/internal/InternalLoggerRegistryTest.java | Added unit tests to cover stale entry removal and concurrent access scenarios |
...j-core/src/main/java/org/apache/logging/log4j/core/util/internal/InternalLoggerRegistry.java
Show resolved
Hide resolved
|
I would like to and I will. Please allow me some time.
Op vr 23 mei 2025 om 21:27 schreef Piotr P. Karwasz <
***@***.***>
… ***@***.**** approved this pull request.
The changes look solid—nice work! There's just one final task before we
can wrap this up: could you please add a changelog entry under
src/changelog/.2.x.x, as outlined in the changelog documentation
<https://logging.apache.org/log4j/tools/log4j-changelog.html#changelog-entry-file>
?
Also, just a quick note on loggerRefByNameByMessageFactory: since it's a
WeakHashMap, it doesn’t follow all the typical Map invariants. From what
I can tell, you're using it safely, and I didn’t spot any issues that would
lead to unexpected behavior. That said, given the sensitivity of this part
of the codebase, I’d like a second pair of eyes on it.
@vy <https://github.com/vy>, could you take a moment to review this as
well?
—
Reply to this email directly, view it on GitHub
<#3681 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAARTSIM67PBK326RDUMPTD275ZDDAVCNFSM6AAAAAB5TRLZL6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDQNRVGQ4TQNJRGY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
vy
requested changes
May 28, 2025
...j-core/src/main/java/org/apache/logging/log4j/core/util/internal/InternalLoggerRegistry.java
Outdated
Show resolved
Hide resolved
...j-core/src/main/java/org/apache/logging/log4j/core/util/internal/InternalLoggerRegistry.java
Outdated
Show resolved
Hide resolved
...j-core/src/main/java/org/apache/logging/log4j/core/util/internal/InternalLoggerRegistry.java
Outdated
Show resolved
Hide resolved
...j-core/src/main/java/org/apache/logging/log4j/core/util/internal/InternalLoggerRegistry.java
Outdated
Show resolved
Hide resolved
...j-core/src/main/java/org/apache/logging/log4j/core/util/internal/InternalLoggerRegistry.java
Outdated
Show resolved
Hide resolved
...st/src/test/java/org/apache/logging/log4j/core/util/internal/InternalLoggerRegistryTest.java
Show resolved
Hide resolved
...st/src/test/java/org/apache/logging/log4j/core/util/internal/InternalLoggerRegistryTest.java
Outdated
Show resolved
Hide resolved
...st/src/test/java/org/apache/logging/log4j/core/util/internal/InternalLoggerRegistryTest.java
Outdated
Show resolved
Hide resolved
...st/src/test/java/org/apache/logging/log4j/core/util/internal/InternalLoggerRegistryTest.java
Outdated
Show resolved
Hide resolved
...st/src/test/java/org/apache/logging/log4j/core/util/internal/InternalLoggerRegistryTest.java
Outdated
Show resolved
Hide resolved
* Rename variables in InternalLoggerRegistry * Add final keyword in InternalLoggerRegistry and tests * Remove/clarify comments in InternalLoggerRegistry * Remove MockLogger class, using anonymous Logger instances in tests * Remove System.runFinalization() calls from tests * Improve assertion logic in testExpungeStaleWeakReferenceEntries * Remove testConcurrentAccess (lacked specific scenario)
vy
approved these changes
May 31, 2025
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.
This pull request addresses issue #3430 by implementing a mechanism to expunge stale entries in
InternalLoggerRegistry.It builds upon PR #3474 and incorporates feedback from @vy and @ppkarwasz.
Key changes include:
ReferenceQueue: When aLoggeris GC'd, itsWeakReferenceis enqueued. Subsequent calls to methods likegetLogger()orcomputeIfAbsent()triggerexpungeStaleEntries()to process it.expungeStaleEntrieslogic: If theReferenceQueueis not empty, a write lock is acquired, the queue is drained, and map entries are scanned once to removeWeakReferenceswith null referents and clean up any empty maps. No action or locking occurs if the queue is initially empty.@vy,
ReferenceQueuehas nopeek()/isEmpty(), so I couldn’t do a proper double-check and used an initialpoll()instead. Feedback is welcome if there's a better way or if I missed something. Thanks!Fixes #3430