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

StackOverflow in LogHandler with classloading due to logging in WebAppClassLoader #1563

Open
oleg-nenashev opened this Issue May 20, 2017 · 7 comments

Comments

Projects
None yet
3 participants
@oleg-nenashev

oleg-nenashev commented May 20, 2017

Hello,

In Jenkins project we have recently upgraded from Jetty 9.2 to 9.4. In some conditions we hit StackOverflow in Jetty WebAppClassLoader when Jetty Debug logging is enabled. Issue - JENKINS-44330. It happens, because one of our LogAppenders does classloading:

at org.eclipse.jetty.util.log.JavaUtilLog.log(JavaUtilLog.java:167)
at org.eclipse.jetty.util.log.JavaUtilLog.debug(JavaUtilLog.java:227)
at org.eclipse.jetty.webapp.WebAppContext.isSystemResource(WebAppContext.java:857)
at org.eclipse.jetty.webapp.WebAppClassLoader.loadClass(WebAppClassLoader.java:548)
at java.lang.ClassLoader.loadClass(ClassLoader.java:357)
at hudson.logging.LogRecorder.orderedTargets(LogRecorder.java:82)
at hudson.logging.LogRecorder$2.publish(LogRecorder.java:108)
at hudson.logging.WeakLogHandler.publish(WeakLogHandler.java:53)
at java.util.logging.Logger.log(Logger.java:738)
at org.eclipse.jetty.util.log.JavaUtilLog.log(JavaUtilLog.java:167)
at org.eclipse.jetty.util.log.JavaUtilLog.debug(JavaUtilLog.java:227)
at org.eclipse.jetty.webapp.WebAppContext.isSystemResource(WebAppContext.java:857)
at org.eclipse.jetty.webapp.WebAppClassLoader.loadClass(WebAppClassLoader.java:548)
at java.lang.ClassLoader.loadClass(ClassLoader.java:357)
at hudson.logging.LogRecorder.orderedTargets(LogRecorder.java:82)
at hudson.logging.LogRecorder$2.publish(LogRecorder.java:108)
at hudson.logging.WeakLogHandler.publish(WeakLogHandler.java:53)

I think we should fix this regression on our side. OTOH I would like to record this issue here, because it may hit other projects. Jetty project contributors may want to apply a patch as well.

CC @olamy @damphyr

@oleg-nenashev oleg-nenashev changed the title from StackOverflow in Jetty due to the logging in classloader to StackOverflow in LogHandler with classloading due to logging in WebAppClassLoader May 20, 2017

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev May 20, 2017

So it will happen if and only if the Debug logging for Jetty is enabled: https://github.com/eclipse/jetty.project/blob/jetty-9.4.x/jetty-webapp/src/main/java/org/eclipse/jetty/webapp/WebAppContext.java#L857

It has been added by this commit: afb7505 .

oleg-nenashev commented May 20, 2017

So it will happen if and only if the Debug logging for Jetty is enabled: https://github.com/eclipse/jetty.project/blob/jetty-9.4.x/jetty-webapp/src/main/java/org/eclipse/jetty/webapp/WebAppContext.java#L857

It has been added by this commit: afb7505 .

oleg-nenashev added a commit to oleg-nenashev/jenkins that referenced this issue May 20, 2017

[FIXED JENKINS-44330] - Prevent classloading of Target comparator in …
…LogRecorder#orderedTargets()

It is just a hotfix, there may be other LogRecorders affected. Ideally we need a response from Jetty maintainers to eclipse/jetty.project#1563. No tests since I see no way to trigger such classloading + no actual need in it.
@gregw

This comment has been minimized.

Show comment
Hide comment
@gregw

gregw May 22, 2017

Contributor
Contributor

gregw commented May 22, 2017

@joakime

This comment has been minimized.

Show comment
Hide comment
@joakime

joakime May 22, 2017

Member

@gregw perhaps we should have a recursion check, this isn't the first StackOverflow from this new classpath pattern logic. See Issue #1448

Member

joakime commented May 22, 2017

@gregw perhaps we should have a recursion check, this isn't the first StackOverflow from this new classpath pattern logic. See Issue #1448

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev May 22, 2017

We fixed the issue on our side, but recursion prevention would be definitely useful. In the case of Debug logging it should also prevent issues with performance though #1448 won't be addressed then

We fixed the issue on our side, but recursion prevention would be definitely useful. In the case of Debug logging it should also prevent issues with performance though #1448 won't be addressed then

@gregw

This comment has been minimized.

Show comment
Hide comment
@gregw

gregw May 22, 2017

Contributor

So if we detect recursion in classloading, then what do we do? Guess it is because of logging and don't log during the load? Seams a bit contrived?

It is much harder for us, perhaps impossible to put recursion detection into logging, because we don't implement all the log handlers and it is a big tax to pay on log handlers that can never be recursive.

Still not convinced we can do anything reasonable.

Contributor

gregw commented May 22, 2017

So if we detect recursion in classloading, then what do we do? Guess it is because of logging and don't log during the load? Seams a bit contrived?

It is much harder for us, perhaps impossible to put recursion detection into logging, because we don't implement all the log handlers and it is a big tax to pay on log handlers that can never be recursive.

Still not convinced we can do anything reasonable.

@joakime

This comment has been minimized.

Show comment
Hide comment
@joakime

joakime May 22, 2017

Member

Would it be possible to put the recursion check in WebAppClassLoader.loadClass()?

Member

joakime commented May 22, 2017

Would it be possible to put the recursion check in WebAppClassLoader.loadClass()?

@gregw

This comment has been minimized.

Show comment
Hide comment
@gregw

gregw May 22, 2017

Contributor

Not really:

  • WACL is as asked to load a class
  • it does some logging
  • the log handler needs to load a class
  • recursion is detected

Then what? return null? throw exception? if so who catches it? How is logging ever instantiated because it can also happen:

  • something does some logging
  • the log handler needs to load a class
  • WACL is as asked to load the class
  • it does some logging
  • the log handler needs to load a class
  • recursion is detected

Basically logging can't be implemented in terms of anything that actually does logging!

Perhaps that means the ClassLoader should not do any logging?

Contributor

gregw commented May 22, 2017

Not really:

  • WACL is as asked to load a class
  • it does some logging
  • the log handler needs to load a class
  • recursion is detected

Then what? return null? throw exception? if so who catches it? How is logging ever instantiated because it can also happen:

  • something does some logging
  • the log handler needs to load a class
  • WACL is as asked to load the class
  • it does some logging
  • the log handler needs to load a class
  • recursion is detected

Basically logging can't be implemented in terms of anything that actually does logging!

Perhaps that means the ClassLoader should not do any logging?

Ykus added a commit to Ykus/jenkins that referenced this issue May 27, 2017

[FIXED JENKINS-44330] - Prevent classloading of Target comparator in …
…LogRecorder#orderedTargets()

It is just a hotfix, there may be other LogRecorders affected. Ideally we need a response from Jetty maintainers to eclipse/jetty.project#1563. No tests since I see no way to trigger such classloading + no actual need in it.

Ykus added a commit to Ykus/jenkins that referenced this issue May 27, 2017

[FIXED JENKINS-44330] - Prevent classloading of Target comparator in …
…LogRecorder#orderedTargets()

It is just a hotfix, there may be other LogRecorders affected. Ideally we need a response from Jetty maintainers to eclipse/jetty.project#1563. No tests since I see no way to trigger such classloading + no actual need in it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment