RFR: 8308341: JNI_GetCreatedJavaVMs returns a partially initialized JVM

[Johan Sjölen]

On Fri, 26 May 2023 01:06:21 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> src/hotspot/share/prims/jni.cpp line 3943:
>> 
>>> 3941:   // initialization, so only bail-out if something seems very wrong.
>>> 3942:   // Though how would we get here in that case?
>>> 3943:   if (vm_created == NOT_CREATED) {
>> 
>> Shouldn't we also handle `IN_PROGRESS`?
>
> I guess my comment was not clear enough. :) It is normal to get here while IN_PROGRESS due to calls from the JDK native code during initialization. So we only consider it an error to call then when NOT_CREATED.

Sloppy reading from me! That makes sense, and the code doing the work (L3960-L3965) seems to take all precautions necessary to ensure that nothing goes wrong if we're `IN_PROGRESS`.

Just a minor suggestion: "during VM initialization" could be "while VM initialization is in progress" to mirror the language in the enum, but that's up to you.

>> test/hotspot/jtreg/runtime/jni/getCreatedJavaVMs/exeGetCreatedJavaVMs.c line 1:
>> 
>>> 1: /*
>> 
>> Can we make this test behave even worse than it already does and have you caused a crash with this test before applying your changes? If it has caused a crash, then I think we're OK with the current form of the test.
>> 
>> Two ideas to make it more likely to crash:
>> 
>> 1. Have more threads racing
>> 2. Let's say you spawn more threads, is it possible for `printf` to prevent data races as it's MT-Safe? More threads would increase likelihood of contention on the implicit lock, leading to some threads having been significantly slowed down by having to perform a more expensive locking procedure, is my thought process.
>
> The test crashes an unpatched VM. We only need the two threads. One will initiate the creation of the VM and the other then tries to attach to it. We want the second thread to call GetCreatedJavaVMs very quickly to make it more likely the VM is not initialized, but not so quickly that GetCreatedJavaVMs reports zero VMs (regardless of the patch). The time it takes the main thread to create the second thread seems to handle that nicely. Of course it depends on number of CPUs etc but on my local machine the test, and the original version from the JBS issue, reliably crashes every time before the fix.

Alright, then I'm very happy with this.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/14139#discussion_r1206430443
PR Review Comment: https://git.openjdk.org/jdk/pull/14139#discussion_r1206432196