RFR: 8308341: JNI_GetCreatedJavaVMs returns a partially initialized JVM

[Johan Sjölen]

On Thu, 25 May 2023 05:02:19 GMT, David Holmes <dholmes at openjdk.org> wrote:

> We now track the in-progress and completed states of VM creation and only return a VM from JNI_GetCreatedJavaVMs when there is a fully initialized VM.
> 
> Testing: 
>  - new regression test
>  - tiers 1-3 (sanity)
> 
> Thanks

Hi, over all looks good, but I have some questions regarding the tests and I wonder whether you missed a case.

src/hotspot/share/prims/jni.cpp line 3475:

> 3473: 
> 3474: // Global invocation API vars
> 3475: enum VM_Creation_State {

Nit: Would `enum class` be preferred here?

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`?

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.

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

PR Review: https://git.openjdk.org/jdk/pull/14139#pullrequestreview-1443519793
PR Review Comment: https://git.openjdk.org/jdk/pull/14139#discussion_r1205276576
PR Review Comment: https://git.openjdk.org/jdk/pull/14139#discussion_r1205284282
PR Review Comment: https://git.openjdk.org/jdk/pull/14139#discussion_r1205305164