RFR: 8308033: The jcmd thread dump related tests should test virtual threads [v2]

[Chris Plummer]

On Thu, 2 May 2024 03:57:42 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

>> test/hotspot/jtreg/serviceability/tmtools/jstack/DaemonThreadTest.java line 77:
>> 
>>> 75:             t.setDaemon(false);
>>> 76:         }
>>> 77:         testThread(t, "");
>> 
>> I think the following is a bit clearer:
>> 
>> 
>> // Virtual threads are always daemon threads. Therefore if the current thread is a
>> // virtual thread, then NormalThread will be a daemon thread and we need to explicitly
>> // make it not a daemon thread.
>> 
>> 
>> You don't actually need the isVirtual() check, especially with the comment explaining things. However, you could also forgo all this logic and the comment by just sticking setDaemon(false) in the NormalThread constructor.
>
> Done - I've updated the PR to just call `setDaemon(false)`  from the constructor of `NormalThread` instead of this conditional logic.
> 
> When I initially added this logic, I was trying to avoid doing any changes that could impact the testing when running under platform threads. But thinking about this test's context, it merely wants to verify that the jstack output correctly presents daemon/non-daemon status, so explicitly setting a "normal" thread to non-daemon shouldn't impact what this test was originally verifying.

Yes, I also briefly wondered if somehow this would be subverting the testing of whether the thread was not daemon by default, but that's not what is being tested here.

>> test/jdk/sun/tools/jstack/BasicJStackTest.java line 53:
>> 
>>> 51:             // print the stacktraces of virtual threads.
>>> 52:             throw new SkippedException("skipping test since current thread is a virtual thread");
>>> 53:         }
>> 
>> I wonder if we can rely on the virtual thread always be mounted. If so, we could revisit this test after [JDK-8330846](https://bugs.openjdk.org/browse/JDK-8330846) is implemented.
>
> Hello Chris, if I understand that JBS issue correctly, then by default jstack (and other tools) will start including stacktraces of virtual threads that currently are mounted on a carrier thread (and thus haven't been parked) in the output.
> 
> If that's the case, then I think when that JBS issue is implemented we can remove this conditional check and the skipping of the test since the virtual thread would end up appearing in the thread dump because that's the thread which would be currently launching the `jstack` process and waiting (through a `ReentrantLock`'s `Condition` object) for the jstack process to complete.
> 
> To revisit this test when JDK-8330846 is implemented, do you want me to file an issue? Or did you mean we should wait doing the changes in this PR until JDK-8330846 is implemented?

I think just noting this in [JDK-8330846](https://bugs.openjdk.org/browse/JDK-8330846) should be good enough.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/19016#discussion_r1588463768
PR Review Comment: https://git.openjdk.org/jdk/pull/19016#discussion_r1588462528