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

Broken logger initialization in 2.24.1 #3143

Closed
kelunik opened this issue Oct 30, 2024 · 8 comments · Fixed by #3199
Closed

Broken logger initialization in 2.24.1 #3143

kelunik opened this issue Oct 30, 2024 · 8 comments · Fixed by #3199
Labels
api Affects the public API bug Incorrect, unexpected, or unintended behavior of existing code

Comments

@kelunik
Copy link

kelunik commented Oct 30, 2024

Description

LoggerContext / LoggerRegistry don't play well together in using weak references, which can result in a null logger if the GC kicks in at a bad timing.

Configuration

Version: 2.24.1

Operating system: Linux / macOS

JDK: 21.0.4

Logs

java.lang.NullPointerException

Cannot invoke "org.apache.logging.log4j.spi.ExtendedLogger.logIfEnabled(String, org.apache.logging.log4j.Level, org.apache.logging.log4j.Marker, String)" because "this.logger" is null

at org.apache.logging.slf4j.Log4jLogger.warn(Log4jLogger.java:233)
at <internal company code>

Reproduction

Optimizing transformations of a program can be designed that reduce the number of objects that are reachable to be less than those which would naively be considered reachable. For example, a Java compiler or code generator may choose to set a variable or parameter that will no longer be used to null to cause the storage for such an object to be potentially reclaimable sooner.

https://docs.oracle.com/javase/specs/jls/se21/html/jls-12.html#jls-12.6.1

Fix

#2936 should fix this, but is currently unreleased.

Reference.reachabilityFence would also fix this, but is JDK 9+ only.

See also: https://shipilev.net/jvm/anatomy-quarks/8-local-var-reachability/

@kelunik
Copy link
Author

kelunik commented Oct 30, 2024

@ppkarwasz
Copy link
Contributor

@kelunik,

Thank you for the report.

The LoggerRegistry implementation was changed in 2.25.0-SNAPSHOT. Can you check if that fixes your issue?
Our snapshots are located in the https://repository.apache.org/snapshots/ Maven repository (see also our download page).

@ppkarwasz ppkarwasz added waiting-for-user More information is needed from the user and removed waiting-for-maintainer labels Nov 4, 2024
@centic9
Copy link
Member

centic9 commented Nov 10, 2024

We hit a similar issues at Apache POI, there it happens reproducibly if log4j-core 2.24.0 and log4j-api 2.24.1 are used.

Reproducer: Run "./gradlew runWriteFile -PpoiVersion=5.4.0" in a checkout of https://github.com/centic9/poi-reproduce

This version-mismatch happens if a library like Apache POI upgrades "api" to the latest version, but projects using it do not upgrade log4j-core along the way. This is likely a common situation.

Is this also fixed in 2.25.0-SNAPSHOT or is this something else which we should report as a separate issue?

Edit: I verified with current 2.25.0-SNAPSHOT and this problem still reproduces, so I reported #3196 to discuss this as a separate issue.

@anuragagarwal561994
Copy link
Contributor

@ppkarwasz when the version 2.25 will be released? And is 2.23 devoid of this bug?

@centic9
Copy link
Member

centic9 commented Nov 10, 2024

As far as I couuld reproduce, it will not be fixed by current 2.25.0-SNAPSHOT, therefore I created bug #3196 for the issue with mixed "api" and "core" versions.

@vy vy added bug Incorrect, unexpected, or unintended behavior of existing code api Affects the public API labels Nov 11, 2024
ppkarwasz added a commit that referenced this issue Nov 11, 2024
Removes weak references to `Logger`s in `LoggerRepository`.
The usage of weak references in `LoggerRepository` might cause `null` to be returned by `LogManager.getLogger()` of all Log4j Core versions up to `2.24.1`.
Versions of Log4j API up to `2.24.0` did hold **hard** references to all the registered loggers, so the change will not alter the previous behavior.

This PR also inverts the order of the `String` and `MessageFactory` keys to the `LoggerRepository` multi-map to limit the number of internal maps. The external map is a `WeakHashMap` to allow logger-specific message factories to be GC-ed.

Closes #3143.
@ppkarwasz
Copy link
Contributor

@anuragagarwal561994,

The bug was introduced in #2961 and it only affects Log4j API 2.24.1 and all the implementations that use LoggerRegistry.

Regarding the 2.25.0, we have a backlog of releases to make, so it might not happen before December. @vy, what do you think about releasing 2.24.2 with a fix to this problem?

@ppkarwasz ppkarwasz removed the waiting-for-user More information is needed from the user label Nov 15, 2024
ppkarwasz added a commit that referenced this issue Nov 15, 2024
Removes weak references to `Logger`s in `LoggerRepository`.
The usage of weak references in `LoggerRepository` might cause `null` to be returned by `LogManager.getLogger()` of all Log4j Core versions up to `2.24.1`.
Versions of Log4j API up to `2.24.0` did hold **hard** references to all the registered loggers, so the change will not alter the previous behavior.

This PR also inverts the order of the `String` and `MessageFactory` keys to the `LoggerRepository` multi-map to limit the number of internal maps. The external map is a `WeakHashMap` to allow logger-specific message factories to be GC-ed.

Closes #3143.
@anuragagarwal561994
Copy link
Contributor

Thanks for the change @ppkarwasz when can we expect the release for the same.

@ppkarwasz
Copy link
Contributor

@anuragagarwal561994,

Thanks for the change @ppkarwasz when can we expect the release for the same.

I am planning a 2.24.2 release for Monday morning, provided that #3209 and #3210 will get reviews by then. See also this dev@logging thread.

As always, you are invited to review PRs, run release candidates in your tests and vote on releases. A motivated -1 on a release candidate can prevent faulty software from being released to the public.

hulkoba pushed a commit to neighbourhoodie/logging-log4j2 that referenced this issue Nov 18, 2024
Removes weak references to `Logger`s in `LoggerRepository`.
The usage of weak references in `LoggerRepository` might cause `null` to be returned by `LogManager.getLogger()` of all Log4j Core versions up to `2.24.1`.
Versions of Log4j API up to `2.24.0` did hold **hard** references to all the registered loggers, so the change will not alter the previous behavior.

This PR also inverts the order of the `String` and `MessageFactory` keys to the `LoggerRepository` multi-map to limit the number of internal maps. The external map is a `WeakHashMap` to allow logger-specific message factories to be GC-ed.

Closes apache#3143.
ppkarwasz added a commit that referenced this issue Nov 18, 2024
)

This is a port of #3199 to the 2.24.x branch.

Removes weak references to `Logger`s in `LoggerRepository`.
The usage of weak references in `LoggerRepository` might cause `null` to be returned by `LogManager.getLogger()` of all Log4j Core versions up to `2.24.1`.
Versions of Log4j API up to `2.24.0` did hold **hard** references to all the registered loggers, so the change will not alter the previous behavior.

This PR also inverts the order of the `String` and `MessageFactory` keys to the `LoggerRepository` multi-map to limit the number of internal maps. The external map is a `WeakHashMap` to allow logger-specific message factories to be GC-ed.

Closes #3143.

Co-authored-by: Volkan Yazıcı <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Affects the public API bug Incorrect, unexpected, or unintended behavior of existing code
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants