On Wed, 29 Jun 2022 08:55:22 GMT, Alan Bateman <al...@openjdk.org> wrote:
>> Chris Plummer has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> fix indentation
>
> src/jdk.jdwp.agent/share/native/libjdwp/threadControl.c line 418:
>
>> 416: // Thread not alive so put on otherThreads list instead
>> of runningVThreads.
>> 417: // It might not have started yet or might have
>> terminated. Either way,
>> 418: // otherThreads is the place for it.
>
> If a terminated thread is added to otherThreads then when will it be removed?
Yes, I had the same question. otherThreads is swept every time any thread is
resumed. This code really isn't much different than what is done for platform
threads, except that the detection that it should go on otherThreads is done
earlier on commonSuspend(). The code differences arise from the differences in
how platform threads and virtual threads are tracked. I hope to unify them
someday, but for now don't want virtual thread support to risk destabilizing
platform thread support.
> src/jdk.jdwp.agent/share/native/libjdwp/threadControl.c line 419:
>
>> 417: // It might not have started yet or might have
>> terminated. Either way,
>> 418: // otherThreads is the place for it.
>> 419: list = &otherThreads;
>
> Minor nit but the native code here uses 4-space indentation.
fixed
> test/jdk/com/sun/jdi/SuspendAfterDeath.java line 62:
>
>> 60: public class SuspendAfterDeath extends TestScaffold {
>> 61: private volatile ThreadReference thread;
>> 62: private volatile boolean breakpointReached = false;
>
> The volatile-write to initialise this to false is not needed here.
Actually I was a bit confused as to why thread was declared volatile, so I just
followed the pattern. Maybe you can explain since you wrote it.
> test/jdk/com/sun/jdi/SuspendAfterDeath.java line 99:
>
>> 97: List argList = new ArrayList(Arrays.asList(args));
>> 98: argList.add("Virtual");
>> 99: args = (String[]) argList.toArray(args);
>
> The raw type and casting caught my addition here. Here's something more
> succulent if you'd like:
>
> args = Stream.concat(Stream.of(args), Stream.of("Virtual"))
> .toArray(String[]::new);
That came from TestScaffold.startUp():
protected void startUp(String targetName) {
List argList = new ArrayList(Arrays.asList(args));
argList.add(targetName);
println("run args: " + argList);
connect((String[]) argList.toArray(args));
waitForVMStart();
}
TBH I simply find myself staring at Streams code like that for way too long
before I figure out what it is trying to do. If you use/read code like that a
lot, it's probably not an issue, but I don't, and prefer something more
straight forward, even if it is not nearly as elegant. Someone should add
`String[] Arrays.appendToArray(String[] arr, String s)`, or even `String[]
Arrays.concat(String[] s1, String[] s2)` would do.
> test/jdk/com/sun/jdi/SuspendAfterDeath.java line 135:
>
>> 133: }
>> 134: }
>> 135: }
>
> I wonder if we need to add `@run main/othervm -Dmain.wrapper=Virtual
> SuspendAfterDeath` to the test description so that running the jdk_jdi test
> group will exercise the issue. As it stands, I think it would require a test
> run with -Dmain.wrapper=Virtual to exercise this code.
Yes, it requires `-Dmain.wrapper=Virtual`, and that was intentional. My
thinking was that we don't have any com/sun/jdi tests that do any virtual
thread testing unless run with `-Dmain.wrapper=Virtual`, so why should this
test be any different? If I made it do virtual thread testing without
`-Dmain.wrapper=Virtual`, then the `-Dmain.wrapper=Virtual` testing becomes
redundant.
-------------
PR: https://git.openjdk.org/jdk19/pull/88
