search

qos-ch / reload4j

reload4j is a drop-in replacement for log4j 1.2.17
Apache License 2.0
148 stars 22 forks source link

No appender with 1.2.24, 1.2.23 works fine #62

Closed davidmoten closed 1 year ago

davidmoten commented 1 year ago

We use reload4j in the lib directory of a glassfish 4 installation (old!) with Java 8, webapps have reload4j at provided scope. 1.2.23 works fine for us but no appender was found when we used reload4j 1.2.24. Being the logger, no information turns up in the logs even after setting system logging to FINE level. I checked the MANIFEST.MF files between the two versions and they look fine, checking further of course I am hamstrung by an enormous amount of source code change associated with reformatting. Any suggestions as to what has been affected?

davidmoten commented 1 year ago

Perhaps this commit: https://github.com/qos-ch/reload4j/commit/3f35d074614e3e2099201629c7dfc816de11d097?

I'll try reverting that commit and test with it.

davidmoten commented 1 year ago

Yes that was it, that commit changed class loading behaviour. When I checked out tag v1.2.24, reverted commit https://github.com/qos-ch/reload4j/commit/3f35d074614e3e2099201629c7dfc816de11d097, rebuilt the jar and deployed, it all worked fine. Can you resolve this with a release soon please?

davidmoten commented 1 year ago

I've had a bit more of a look. We have reload4j.jar in glassfish/lib and log4j.properties in the webapp WAR (a reasonable strategy given each WAR might want to customize its logging configuration). reload4j 1.2.24 no longer uses the Thread Context Class Loader for finding the log4j.properties resource and doesn't have visibility of the webapp classes so we end up with no logging (No appender found message).

davidmoten commented 1 year ago

I believe reload4j should continue using the Thread Context Class Loader if present. Can this be sorted out please?

zhiyli commented 1 year ago

We are hitting the same issue. Any updates and when can we expect this to be fixed? Thanks a lot.

davidmoten commented 1 year ago

@ceki can we get an update on this please?

ceki commented 1 year ago

Commit 3f35d074614 fixes a pre-existing bug. The fact that it enabled an undesired behavior needs to be analyzed and corrected appropriately. However, I am quite swamped at the moment. If you need an issue fixed as soon as possible, then consider championing a release.

davidmoten commented 1 year ago

Unfortunately I can't make the dollars happen for you but that commit was a major breaking change for users of Java 8 in my books. I'd like to see that commit fixed or rolled back in a release and fixed properly later. The Thread Context Class Loader use is just as valid for Java 9+ (and if disabled there we'd be buggered promoting our stuff to Java 9+). The java1() method stuff in the old code seems to be there just because Thread.getContextClassLoader only appeared in Java 1.2.

ceki commented 1 year ago

When run in JDK 9 and later, the code in reload4j 1.2.23 never used the TCCL. Version 1.2.24 preserves the same behavior.

However, in JDK 8 and earlier, the code in reload4j 1.2.23 used the TCCL by default. While in your environment it may be appropriate to use TCCL in JDK 9+ as well, other users might want to preserve existing behavior.

I don't think there is a perfect solution fixing this issue. The best I can think of would be to default to TCCL, unless the "log4j.ignoreTCL" flag was set to true. If getting the resource using TCCL was unsuccessful then try with the class loader that loaded "Loader.class". BTW, this is the behavior you seem to ask for.

davidmoten commented 1 year ago

Thanks @ceki, I think defaulting to TCCL and using a system property to skip it is great. Thanks for reviewing.

davidmoten commented 1 year ago

Thanks @ceki! I've reviewed the commit 7d7327869ab28023d1eed0234d813e93d9e1311d. I can see some comment and javadoc typos, would be easier to provide review comments if you made a PR, even if you merge it before others get to it. Works well for changes where you might get some discussion. Anyway, thanks again for making the time for this.

ceki commented 1 year ago

@davidmoten Thank you for the suggestion. I've looked at commit 7d7327869 but could not see any typos. You are most welcome to make comments on said commit directly.

davidmoten commented 1 year ago

Righto, added comment about one trivial typo in the commit.

I suspect this javadoc needs updating:

https://github.com/qos-ch/reload4j/blob/47b6d713e229e5af242b9756bdda6fcc22fc7c08/src/main/java/org/apache/log4j/helpers/Loader.java#L150-L153

Also noticed other typos in Loader.java, trivial also (don't care if never get fixed):

is is https://github.com/qos-ch/reload4j/blob/47b6d713e229e5af242b9756bdda6fcc22fc7c08/src/main/java/org/apache/log4j/helpers/Loader.java#L65

jaadoc https://github.com/qos-ch/reload4j/blob/47b6d713e229e5af242b9756bdda6fcc22fc7c08/src/main/java/org/apache/log4j/helpers/Loader.java#L69

dound https://github.com/qos-ch/reload4j/blob/47b6d713e229e5af242b9756bdda6fcc22fc7c08/src/main/java/org/apache/log4j/helpers/Loader.java#L87

Extentsion https://github.com/qos-ch/reload4j/blob/47b6d713e229e5af242b9756bdda6fcc22fc7c08/src/main/java/org/apache/log4j/helpers/Loader.java#L103