On Wed, 29 Jun 2022 23:24:28 GMT, Chris Plummer <cjplum...@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
