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

[Alan Bateman]

On Wed, 29 Jun 2022 23:24:28 GMT, Chris Plummer <cjplummer at openjdk.org> wrote:

>> 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.

II'm just pointing out that initialising breakpointReached to false is unnecessary as it's the default value.

>> 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 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.

Whatever you are comfortable with but I think we should add the raw type and avoid the cast.

>> 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.

The test uses a platform thread by default so it's not testing the issue in JDK-8287847. The suggestion is to have it re-run with the system property set, like this:

@run main/othervm SuspendAfterDeath 
@run main/othervm -Dmain.wrapper=Virtual SuspendAfterDeath 

I accept this may be different to the existing com/sun/jdi tests but they pre-date virtual threads.

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

PR: https://git.openjdk.org/jdk19/pull/88