8153133: Thread.dumpStack() can use StackWalker #6292
Conversation
|
👋 Welcome back jpai! A progress list of the required criteria for merging this PR into |
Webrevs
|
| // the StackWalker itself, which means that the StackWalker isn't usable | ||
| // at this point in time. So we fallback to creating a Exception instance | ||
| // and printing its stacktrace | ||
| new Exception(Thread.currentThread().name + " Stack trace").printStackTrace(); |
There was a problem hiding this comment.
The recursive initialisation issue will require discussion to see if we can avoid StackWalker.getInstance return null (which I assume is masking the issue).
printStackTrace interacts with locking of the streams to avoid garbled output when many threads are printing to standard output output/error at the same time. If we change dumpStack to use StackWalker then it will need to do the same.
There was a problem hiding this comment.
printStackTrace interacts with locking of the streams to avoid garbled output when many threads are printing to standard output output/error at the same time. If we change dumpStack to use StackWalker then it will need to do the same.
Indeed. I have updated the PR to use a lock while writing out to the System.err.
I had a look at the printStackTrace() implementation and it ends up locking the PrintStream (System.err) or PrintWriter for the duration of the entire stacktrace printing of each stacktrace element. The updated PR thus uses System.err as the lock to match that semantic.
There was a problem hiding this comment.
The recursive initialisation issue will require discussion to see if we can avoid StackWalker.getInstance return null (which I assume is masking the issue).
For a better context, here's the stacktrace of such a call to Thread.dumpStack during the class initialization of StackWalker:
Caused by: java.lang.NullPointerException: Cannot invoke "java.lang.StackWalker.forEach(java.util.function.Consumer)" because the return value of "java.lang.StackWalker.getInstance()" is null
at java.base/java.lang.Thread.dumpStack(Thread.java:1383)
at java.base/java.security.AccessController.checkPermission(AccessController.java:1054)
at java.base/java.lang.SecurityManager.checkPermission(SecurityManager.java:411)
at java.base/java.lang.reflect.AccessibleObject.checkPermission(AccessibleObject.java:91)
at java.base/java.lang.reflect.Method.setAccessible(Method.java:193)
at java.base/java.lang.Class$3.run(Class.java:3864)
at java.base/java.lang.Class$3.run(Class.java:3862)
at java.base/java.security.AccessController.doPrivileged(AccessController.java:318)
at java.base/java.lang.Class.getEnumConstantsShared(Class.java:3861)
at java.base/java.lang.System$2.getEnumConstantsShared(System.java:2295)
at java.base/java.util.EnumSet.getUniverse(EnumSet.java:408)
at java.base/java.util.EnumSet.noneOf(EnumSet.java:111)
at java.base/java.lang.StackWalker.<clinit>(StackWalker.java:291)
As you will notice, this call comes from the security (permission check) layer when StackWalker class is being clinited. The check for StackWalker.getInstance() being null, in the Thread.dumpStack() implementation is indeed almost a hackish way to identify this case where StackWalker's clinit is in progress (in the current thread). I can't think of a different way to handle this use case, so looking forward to any suggestions.
There was a problem hiding this comment.
The updated PR thus uses System.err as the lock to match that semantic.
The test case has also been updated to add a new test which invokes Thread.dumpStack() from multiple threads and verifies that the stacktrace written out isn't garbled. Running the test without the addition of the lock on System.err in the Thread.dumpStack implementation does indeed show the garbled output and causes the test failure (which is a good thing).
There was a problem hiding this comment.
I'm uncomfortable with this change, does the change have any benefit?
There was a problem hiding this comment.
I'm uncomfortable with this change, does the change have any benefit?
To me, the initial appeal of this change was to use a more natural API instead of creating an Exception instance and printing the stacktrace from it. Performance wise, I hadn't run any numbers. But having to fallback to using the Exception instance in this above case, that too by doing an almost hackish check, does indeed remove any kind of benefit to this change.
Like Mandy suggests I will go ahead and withdraw this PR and close the linked issue.
- Update test to verify that multiple threads invoking Thread.dumpStack() doesn't generate garbled output
|
@jaikiran Thanks for making this change and identifying the recursive initialization issue. I also concur with Alan that replacing Thread::dumpStack implementation with StackWalker does not buy us much. It has to fallback to |
Will do. |
Can I please get a review of this change which seeks to implement the enhancement noted in https://bugs.openjdk.java.net/browse/JDK-8153133?
The commit in this PR uses the
StackWalkerAPI to dump the stacktrace of the thread. A few things to note about this change:Exceptioninstance) which would statejava.lang.Exception: Stack traceand the rest of the lines would be the stacktrace. The change in this PR does a small (unrelated) enhacement which now includes the thread name in the message. So something likeMainThread Stack tracewhereMainThreadis the name of the thread whose stacktrace is being dumped. The linked JBS doesn't mention this change as a necessity but I decided to include it in this PR since I've always missed the thread name in that dumped stacktrace and since we are now using the StackWalker API, the mention ofjava.lang.Exception: ...line in the thread dump will no longer make sense (unless we wanted backward compatibility)Thread.dumpStack()to be called (for debugging purposes) from places where lambda usage might be too early.Exception.printStackTrace()to allow for calls toThread.dumpStack()whenStackWalkerclass itself is being initialized.The PR also includes a new jtreg test which verifies this change. The test has 3 test actions - one without security manager, one with security manager and one with
java.security.debug=access,stackto exercise the case where Thread.dumpStack() gets called during StackWalker class initialization.The linked JBS issue notes that it would be good to use the StackWalker API even in the
Thread.getStackTraceimplementation. This PR however doesn't include that change because I wanted to tackle that separately since I think that would need to run some performance benchmarks to evaluate any performance changes. The currentThread.dumpStackmethod clearly states that it is meant for debugging purposes so I haven't run any kind of performance benchmarks on this change.Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/6292/head:pull/6292$ git checkout pull/6292Update a local copy of the PR:
$ git checkout pull/6292$ git pull https://git.openjdk.java.net/jdk pull/6292/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 6292View PR using the GUI difftool:
$ git pr show -t 6292Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/6292.diff