[jdk19] RFR: 8287847: Fatal Error when suspending virtual thread after it has terminated [v2]

[Chris Plummer]

On Wed, 29 Jun 2022 08:55:22 GMT, Alan Bateman <alanb at 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