RFR: 8264760: JVM crashes when two threads encounter the same resolution error [v3]

[David Holmes]

On Thu, 8 Apr 2021 04:38:37 GMT, Wang Huang <whuang at openjdk.org> wrote:

>> As shown in JDK-8264760, I changed notes with @dholmes-ora and only fixed this issue by deleting the assert. The other whole bigger big will be fixed in the other issue.
>
> Wang Huang has updated the pull request incrementally with one additional commit since the last revision:
> 
>   add throw exception

Hi Wang,

Thanks for reporting this and offering to fix it. I have a comment on the actual fix and a few issues with the testcase - though thanks for adding one. With a race condition like this it is hard to write a test that will fail reliably without the fix.

Thanks,
David

src/hotspot/share/classfile/systemDictionary.cpp line 1939:

> 1937:     ResolutionErrorEntry* entry = resolution_errors()->find_entry(index, hash, pool, which);
> 1938:     if (entry != NULL && entry->nest_host_error() == NULL) {
> 1939:       entry->set_nest_host_error(message);

You should never find an entry where the nest_host_error() is NULL. If there were such an entry then it implies a regular resolution error occurred, but that means we cannot have reached this code as `klass_at` in our caller would have thrown the exception. So this should simply be:
if (entry != NULL) {
  assert(entry->nest_host_error() != NULL, "cannot have a NULL error at this point");
}
You also need to delete part of the method comment:
//... If an entry already exists it will
// be updated with the nest host error message.
as we don't do that.

test/hotspot/jtreg/runtime/Nestmates/membership/HostNoNestMember.jcod line 1:

> 1: class HostNoNestMember {

This new file needs a copyright notice and GPL header.

Also please add a comment stating what this jcod file describes - see other jcod files in the directory for example.

test/hotspot/jtreg/runtime/Nestmates/membership/TestNestHostErrorWithMultiThread.java line 38:

> 36: import java.util.concurrent.CountDownLatch;
> 37: 
> 38: class HostNoNestMember {

Please add a comment describing how this class is replaced with the jcode version, and the difference in that version.

test/hotspot/jtreg/runtime/Nestmates/membership/TestNestHostErrorWithMultiThread.java line 43:

> 41:   }
> 42: 
> 43:   public int foo() {

Please call this "test".

test/hotspot/jtreg/runtime/Nestmates/membership/TestNestHostErrorWithMultiThread.java line 53:

> 51:   public static void main(String args[]) throws Throwable {
> 52:     TestNestHostErrorWithMultiThread t = new TestNestHostErrorWithMultiThread();
> 53:     t.test();

There is no need to create an instance  - just use a static test method.

The test code could be placed directly in main in this case.

test/hotspot/jtreg/runtime/Nestmates/membership/TestNestHostErrorWithMultiThread.java line 58:

> 56:   public void test() throws Throwable {
> 57: 
> 58:     CountDownLatch latch = new CountDownLatch(1);

The use of CDL doesn't guarantee that both threads have reached the await() before the main thread does the countDown(). If you use a CyclicBarrier instead it will trip when the second thread arrives.

test/hotspot/jtreg/runtime/Nestmates/membership/TestNestHostErrorWithMultiThread.java line 62:

> 60:     new Thread(() -> {
> 61:       try {
> 62:         latch.await();

Please add a comment:

// Try to have all threads trigger the nesthost check at the same time

test/hotspot/jtreg/runtime/Nestmates/membership/TestNestHostErrorWithMultiThread.java line 64:

> 62:         latch.await();
> 63:         HostNoNestMember h = new HostNoNestMember();
> 64:         h.foo();

Please follow this by:
`throw new RuntimeException("Did not get expected IllegalAccessError");`

test/hotspot/jtreg/runtime/Nestmates/membership/TestNestHostErrorWithMultiThread.java line 68:

> 66:         System.out.println("OK - got expected exception: " + expected);
> 67:       } catch (InterruptedException e) {}
> 68:     }).start();

Please extract the logic to a Runnable so that both threads can use that one runnable rather than duplicating the code.

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

Changes requested by dholmes (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3392