No appender with 1.2.24, 1.2.23 works fine

[davidmoten]

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]

Perhaps this commit: 3f35d07?

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

[davidmoten]

Yes that was it, that commit changed class loading behaviour. When I checked out tag v1.2.24, reverted commit 3f35d07, rebuilt the jar and deployed, it all worked fine. Can you resolve this with a release soon please?

[davidmoten]

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]

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

[zhiyli]

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

[davidmoten]

@ceki can we get an update on this please?

[ceki]

Commit 3f35d07 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]

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]

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]

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

[davidmoten]

Thanks @ceki! I've reviewed the commit 7d73278. 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]

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

[davidmoten]

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

I suspect this javadoc needs updating:

reload4j/src/main/java/org/apache/log4j/helpers/Loader.java

Lines 150 to 153 in 47b6d71

	* <p>Nota bene: In versions of reload4j 1.2.23 and earlier, the documentation stated that
	* the thread context class loader was used to load the specified class but
	* when running under JDK 9 and later this was <b>not</b> actually the case. As of version 1.2.24,
	* the javadoc above matches the code as executed.

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

is is

reload4j/src/main/java/org/apache/log4j/helpers/Loader.java

Line 65 in 47b6d71

* <code>ClassLoader.getSystemResource(resource)</code>, that is is using the

jaadoc

reload4j/src/main/java/org/apache/log4j/helpers/Loader.java

Line 69 in 47b6d71

* <p>Nota bene: In versions of reload4j 1.2.23 and earlier, the jaadoc documentation stated that

dound

reload4j/src/main/java/org/apache/log4j/helpers/Loader.java

Line 87 in 47b6d71

// if we were instructed to ignore TCL or if no url was dound, try using the

Extentsion

reload4j/src/main/java/org/apache/log4j/helpers/Loader.java

Line 103 in 47b6d71

// may be the case that clazz was loaded by the Extentsion class