On Thu, 2 May 2024 03:57:42 GMT, Jaikiran Pai <j...@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
