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

Replacing log4j with reload4j can cause NoSuchFieldError #41

Open
scott-kirk opened this issue Feb 17, 2022 · 6 comments
Open

Replacing log4j with reload4j can cause NoSuchFieldError #41

scott-kirk opened this issue Feb 17, 2022 · 6 comments
Assignees
Labels
bug Something isn't working documentation Improvements or additions to documentation
Milestone

Comments

@scott-kirk
Copy link

This appears to be an interaction between certain versions of the slf4j-log4j12 library and reload4j. If reload4j is used instead of log4j, then using MDC will cause the following stack trace:

Exception in thread "main" java.lang.NoSuchFieldError: tlm
	at org.apache.log4j.MDCFriend.fixForJava9(MDCFriend.java:11)
	at org.slf4j.impl.Log4jMDCAdapter.<clinit>(Log4jMDCAdapter.java:38)
	at Main.main(Main.java:5)

This was discovered by trying to replace log4j with reload4j in the apache Kafka repository, https://github.com/apache/kafka/blob/6eed7743ff6c0e73d65c09bac2e2ad9586cc56ce/gradle/dependencies.gradle#L174

It only occurs in Java versions above 8. The minimal reproduction can be found here: https://github.com/scott-kirk/reload4j-exception
I know it's slf4j code that's directly causing this, and it is fixed by upgrading slf4j to the latest version, I was just caught off guard by this issue and think it'd be useful to either explicitly say to use the latest version of slf4j or to somehow apply a workaround to avoid needing to also upgrade slf4j.

@ceki
Copy link
Member

ceki commented Feb 17, 2022

Thank you for this detailed report.

The following analysis is inaccurate and should not be heeded. It is preserved here for the historical record.

org.apache.log4j.MDCFriend shipping with slf4j-log4j12 tries to repair log4j1.x MDC on the fly. This reparation fails in
modularized applications and in JDK versions 11(?) and later. The same identical exception is thrown if slf4j-log4j and log4j
1.2.17 are on the classpath.

Thus, the issue is not caused by relaod4j but by slf4j-log4j12 trying to fix log4j's MDC and failing to do so due to new JDK reflection restrictions.

@scott-kirk
Copy link
Author

Thanks for the response and the explanation. The link you provided is useful, but could you elaborate on The same identical exception is thrown if slf4j-log4j and log4j 1.2.17 are on the classpath? In my repo with the reproduction if I edit the gradle file and comment out reload4j and uncomment log4j there's no error thrown so I don't seem to have the behavior described. It's an easy enough fix to just upgrade slf4j but I suppose I'm just wondering why the docs say it has more to do with the jdk version when in my repo there's no error when using log4j.

This could be just an interaction with the specific versions I'm using, but I just want to make sure we're on the same page

@ceki
Copy link
Member

ceki commented Feb 18, 2022

Thanks for the response and the explanation. The link you provided is useful, but could you elaborate on The same identical exception is thrown if slf4j-log4j and log4j 1.2.17 are on the classpath? In my repo with the reproduction if I edit the gradle file and comment out reload4j and uncomment log4j there's no error thrown so I don't seem to have the behavior described. It's an easy enough fix to just upgrade slf4j but I suppose I'm just wondering why the docs say it has more to do with the jdk version when in my repo there's no error when using log4j.

You are right. Thank you for following up on this.

When I create the classpath manually, the observe behaviors are indeed different when run with log4j.jar and reload4j.jar. Now that I think of it, this actually makes sense, since the type of MDC package private field tlm was changed from Object (in log4j) to ThreadLocal (in reload4j).

This could be just an interaction with the specific versions I'm using, but I just want to make sure we're on the same page

No, mystery solved as explained above. I'll update the docs again (tomorrow).

@ceki
Copy link
Member

ceki commented Feb 18, 2022

SLF4J documentation updated to explain the problem.

See https://www.slf4j.org/codes.html#no_tlm

@ceki ceki self-assigned this Apr 4, 2022
@ceki ceki added the documentation Improvements or additions to documentation label Apr 4, 2022
@ceki ceki closed this as completed Apr 21, 2022
ijuma added a commit to apache/kafka that referenced this issue May 11, 2022
* Replace `log4j` with `reload4j` in `copyDependantLibs`. Since we have
  some projects that have an explicit `reload4j` dependency, it
  was included in the final release release tar - i.e. it was effectively
  a workaround for this bug.
* Exclude `log4j` and `slf4j-log4j12` transitive dependencies for
  `streams:upgrade-system-tests`. Versions 0100 and 0101
  had a transitive dependency to `log4j` and `slf4j-log4j12` via
  `zkclient` and `zookeeper`. This avoids classpath conflicts that lead
  to [NoSuchFieldError](qos-ch/reload4j#41) in
  system tests.

Reviewers: Jason Gustafson <jason@confluent.io>
ijuma added a commit to apache/kafka that referenced this issue May 11, 2022
* Replace `log4j` with `reload4j` in `copyDependantLibs`. Since we have
  some projects that have an explicit `reload4j` dependency, it
  was included in the final release release tar - i.e. it was effectively
  a workaround for this bug.
* Exclude `log4j` and `slf4j-log4j12` transitive dependencies for
  `streams:upgrade-system-tests`. Versions 0100 and 0101
  had a transitive dependency to `log4j` and `slf4j-log4j12` via
  `zkclient` and `zookeeper`. This avoids classpath conflicts that lead
  to [NoSuchFieldError](qos-ch/reload4j#41) in
  system tests.

Reviewers: Jason Gustafson <jason@confluent.io>
ijuma added a commit to apache/kafka that referenced this issue May 11, 2022
* Replace `log4j` with `reload4j` in `copyDependantLibs`. Since we have
  some projects that have an explicit `reload4j` dependency, it
  was included in the final release release tar - i.e. it was effectively
  a workaround for this bug.
* Exclude `log4j` and `slf4j-log4j12` transitive dependencies for
  `streams:upgrade-system-tests`. Versions 0100 and 0101
  had a transitive dependency to `log4j` and `slf4j-log4j12` via
  `zkclient` and `zookeeper`. This avoids classpath conflicts that lead
  to [NoSuchFieldError](qos-ch/reload4j#41) in
  system tests.

Reviewers: Jason Gustafson <jason@confluent.io>
a0x8o added a commit to a0x8o/kafka that referenced this issue May 11, 2022
* Replace `log4j` with `reload4j` in `copyDependantLibs`. Since we have
  some projects that have an explicit `reload4j` dependency, it
  was included in the final release release tar - i.e. it was effectively
  a workaround for this bug.
* Exclude `log4j` and `slf4j-log4j12` transitive dependencies for
  `streams:upgrade-system-tests`. Versions 0100 and 0101
  had a transitive dependency to `log4j` and `slf4j-log4j12` via
  `zkclient` and `zookeeper`. This avoids classpath conflicts that lead
  to [NoSuchFieldError](qos-ch/reload4j#41) in
  system tests.

Reviewers: Jason Gustafson <jason@confluent.io>
@ceki ceki added this to the 1.2.21 milestone May 13, 2022
@ceki ceki reopened this May 13, 2022
@ceki ceki added the bug Something isn't working label May 17, 2022
@ceki
Copy link
Member

ceki commented May 17, 2022

This backward compatibility problem was fixed in commits 0cbed42 and dc3cc1f shipping with reload4j version 1.2.21.

Reload4j version slf4j-reload4j version slf4j-log4j12 version result
1.2.20 17.33 absent OK
1.2.20 absent 1.7.30 exception (see below)
1.2.21 17.33 absent OK
1.2.21 absent 1.7.30 OK
Exception in thread "main" java.lang.NoSuchFieldError: tlm
        at org.apache.log4j.MDCFriend.fixForJava9(MDCFriend.java:11)
        at org.slf4j.impl.Log4jMDCAdapter.<clinit>(Log4jMDCAdapter.java:38)
        at org.slf4j.impl.StaticMDCBinder.getMDCA(StaticMDCBinder.java:59)
        at org.slf4j.MDC.bwCompatibleGetMDCAdapterFromBinder(MDC.java:99)
        at org.slf4j.MDC.<clinit>(MDC.java:108)

@OlivierJaquemet
Copy link

@ceki As you indicated in your last message, this backward compatibility problem was fixed in commits 0cbed42 and dc3cc1f .
Version 1.2.21 including those commit has been released.

Therefore, I think this issue can be closed. Correct ?

xabolcs added a commit to openseedbox/openseedbox that referenced this issue Jun 23, 2022
reload4j is a drop-in replacement for log4j 1.2.17

The binary compatibility issue [0] between earlier versions of reload4j
and slf4j-log4j12 has been fixed.
Although it is recommended that you use slf4j-reload4j as the preferred
adapter for the slf4j/reload4j combination, with reload4j version 1.2.21
and later you can freely mix any version of slf4j-log4j12, if you have to.

From Docker:
SLF4J: Class path contains multiple SLF4J bindings.
SLF4J: Found binding in [jar:file:/src/openseedbox/lib/slf4j-reload4j-1.7.36.jar!/org/slf4j/impl/StaticLoggerBinder.class]
SLF4J: Found binding in [jar:file:/play/framework/lib/slf4j-log4j12-1.7.22.jar!/org/slf4j/impl/StaticLoggerBinder.class]
SLF4J: See http://www.slf4j.org/codes.html#multiple_bindings for an explanation.
SLF4J: Actual binding is of type [org.slf4j.impl.Reload4jLoggerFactory]

[0]: qos-ch/reload4j#41
xabolcs added a commit to openseedbox/openseedbox-server that referenced this issue Jul 7, 2022
reload4j is a drop-in replacement for log4j 1.2.17

The binary compatibility issue [0] between earlier versions of reload4j
and slf4j-log4j12 has been fixed.
Although it is recommended that you use slf4j-reload4j as the preferred
adapter for the slf4j/reload4j combination, with reload4j version 1.2.21
and later you can freely mix any version of slf4j-log4j12, if you have to.

From Docker:
SLF4J: Class path contains multiple SLF4J bindings.
SLF4J: Found binding in [jar:file:/src/openseedbox-server/lib/slf4j-reload4j-1.7.36.jar!/org/slf4j/impl/StaticLoggerBinder.class]
SLF4J: Found binding in [jar:file:/play/framework/lib/slf4j-log4j12-1.7.22.jar!/org/slf4j/impl/StaticLoggerBinder.class]
SLF4J: See http://www.slf4j.org/codes.html#multiple_bindings for an explanation.
SLF4J: Actual binding is of type [org.slf4j.impl.Reload4jLoggerFactory]

[0]: qos-ch/reload4j#41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working documentation Improvements or additions to documentation
Projects
None yet
Development

No branches or pull requests

4 participants
@ceki @OlivierJaquemet @scott-kirk and others