Skip to content

Conversation

@jasontedor
Copy link
Contributor

@jasontedor jasontedor commented Sep 1, 2017

This commit fixes a bug in StackLocator#calcLocation on JDK 9. The particular issue here is that on JDK 9, all stack trace locations report as line 71 of StackLocatorUtil. This is due to a bug in the JDK 9 implementation of StackLocator. The bug is that instead of dropping the top frames of the stack until the first frame that matches the fully-qualified class name of the logger, the implementation would drop all frames from the top that match the fully-qualified class name of the logger. Of course, at this point in the stack trace, there would be none. The fix is to reverse the condition, that we drop all frames until we reach a frame matching the fully-qualified class name of the logger, and then drop all frames matching the fully-qualified class name of the logger. This commit also adds a test that was broken before this change and now passes.

@jasontedor
Copy link
Contributor Author

@rgoers
Copy link
Member

rgoers commented Sep 1, 2017

Finding the frame prior to the first frame that matches the class name of the logger is likely to return a frame that still points at the logger class because multiple methods in the logger may be called.
What the original code is trying to do is find the first stack frame that matches the fully-qualified class name of the logger and then skip all the stack frames for that logger. If it isn't doing that correctly then it obviously needs to be fixed.

@jasontedor
Copy link
Contributor Author

@rgoers Thanks for the feedback, that makes sense! I will do this:

  • make a change that will make that work
  • add a comment to the code that explains why the code is doing what it's going to be doing
  • update the test to account for that situation

@rgoers
Copy link
Member

rgoers commented Sep 1, 2017

@jasontedor Thanks for contributing to Log4j - we appreciate all the help we can get!

@jasontedor
Copy link
Contributor Author

You're welcome @rgoers, it is my pleasure. I pushed a commit as we discussed.

This commit fixes a bug in StackLocator#calcLocation on JDK 9. The
particular issue here is that on JDK 9, all stack trace locations report
as line 71 of StackLocatorUtil. This is due to a bug in the JDK 9
implementation of StackLocator. The bug is that instead of dropping the
top frames of the stack until the first frame that matches the
fully-qualified class name of the logger, the implementation would drop
all frames from the top that match the fully-qualified class name of the
logger. Of course, at this point in the stack trace, there would be
none. The fix is to reverse the condition, that we drop all frames until
we reach a frame matching the fully-qualified class name of the logger,
and then drop all frames matching the fully-qualified class name of the
logger. This commit also adds a test that was broken before this change
and now passes.
@jasontedor jasontedor force-pushed the java-9-stack-locator-calc-location branch from 2d9d81b to 02eee31 Compare September 2, 2017 14:43
@MartyIX
Copy link

MartyIX commented Sep 4, 2017

@jasontedor

This is due to a bug in the JDK 9 implementation of StackLocator.

Do you happen to have a bug report link?

Thank you :)

Copy link
Member

@rgoers rgoers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically I forgot the ! when walking the elements back to the logger.

@asfgit asfgit merged commit 02eee31 into apache:master Sep 4, 2017
@jasontedor jasontedor deleted the java-9-stack-locator-calc-location branch September 4, 2017 13:40
@jasontedor
Copy link
Contributor Author

@MartyIX The bug report is linked above. The bug manifests as that the source line on any logging event will appear to come from stack locator itself, which is clearly a mistake. This also impacts the use of the %l pattern. This bug only manifests on JDK 9.

@jasontedor
Copy link
Contributor Author

Thanks @rgoers.

@rgoers
Copy link
Member

rgoers commented Sep 22, 2017

I just tried running one of my webapps on tomcat under Java 9. Although I didn't have much success due to Spring (that is a different story) I do see the location information correctly.

@jasontedor
Copy link
Contributor Author

I can confirm that as well, both the unit tests we have that rely on this are passing now, and the application is logging the correct location when %l is used as a pattern.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants