RFR: 8260366: ExtendedSocketOptions <clinit> can deadlock in some circumstances [v3]

[Vyom Mani Tewari]

On Thu, 18 Feb 2021 05:01:41 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

>>> > Changes looks OK to me, any specific reason for removing "final" specifier from "getInstance" & "register" methods ?.
>>> 
>>> Hello Vyom, thank you for the review. Since those two methods are `static`, the `final` was redundant on them and since this patch was already changing those 2 methods, I decided to remove it while I was at it. However, if you and others feel that this patch shouldn't change it, I will introduce it back.
>> 
>> I think it's OK for me. What about improving the test little bit,  your test wants to  load both classes at the same time. Please have a look on modified test.
>> 
>> 
>> /*
>>  * Copyright (c) 2021, Oracle and/or its affiliates. All rights reserved.
>>  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
>>  *
>>  * This code is free software; you can redistribute it and/or modify it
>>  * under the terms of the GNU General Public License version 2 only, as
>>  * published by the Free Software Foundation.
>>  *
>>  * This code is distributed in the hope that it will be useful, but WITHOUT
>>  * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>>  * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
>>  * version 2 for more details (a copy is included in the LICENSE file that
>>  * accompanied this code).
>>  *
>>  * You should have received a copy of the GNU General Public License version
>>  * 2 along with this work; if not, write to the Free Software Foundation,
>>  * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
>>  *
>>  * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
>>  * or visit www.oracle.com if you need additional information or have any
>>  * questions.
>>  */
>> import org.testng.Assert;
>> import org.testng.annotations.Test;
>> 
>> import java.util.concurrent.Callable;
>> import java.util.concurrent.CountDownLatch;
>> import java.util.concurrent.ExecutorService;
>> import java.util.concurrent.Executors;
>> import java.util.concurrent.Future;
>> 
>> /**
>>  * @test 
>>  * @bug 8260366
>>  * @summary Verify that concurrent classloading of
>>  * sun.net.ext.ExtendedSocketOptions and jdk.net.ExtendedSocketOptions doesn't
>>  * lead to a deadlock
>>  * @modules java.base/sun.net.ext:open
>>  * @run testng/othervm ExtendedSocketOptionsTest
>>  * @run testng/othervm ExtendedSocketOptionsTest
>>  * @run testng/othervm ExtendedSocketOptionsTest
>>  * @run testng/othervm ExtendedSocketOptionsTest
>>  * @run testng/othervm ExtendedSocketOptionsTest
>>  */
>> public class ExtendedSocketOptionsTest {
>> 
>>     /**
>>      * Loads {@code jdk.net.ExtendedSocketOptions} and
>>      * {@code sun.net.ext.ExtendedSocketOptions} concurrently in a thread of
>>      * their own and expects the classloading of both those classes to
>>      * succeed.Additionally, after the classloading is successfully done, calls
>>      * the sun.net.ext.ExtendedSocketOptions#getInstance() and expects it to
>>      * return a registered ExtendedSocketOptions instance.
>>      *
>>      * @throws java.lang.Exception
>>      */
>>     @Test
>>     public void testConcurrentClassLoad() throws Exception {
>>         CountDownLatch latch = new CountDownLatch(1);
>>         final Callable<Class<?>> task1 = new Task("jdk.net.ExtendedSocketOptions", latch);
>>         final Callable<Class<?>> task2 = new Task("sun.net.ext.ExtendedSocketOptions", latch);
>>         final ExecutorService executor = Executors.newFixedThreadPool(2);
>>         try {
>>             final Future<Class<?>>[] results = new Future[2];
>> 
>>             // submit
>>             results[0] = executor.submit(task1);
>>             results[1] = executor.submit(task2);
>>             latch.countDown();
>> 
>>             // wait for completion
>>             for (Future<Class<?>> result: results) {
>>                 final Class<?> k = result.get();
>>                 System.out.println("Completed loading " + k.getName());
>>             }
>>         } finally {
>>             executor.shutdownNow();
>>         }
>>         // check that the sun.net.ext.ExtendedSocketOptions#getInstance() does indeed return
>>         // the registered instance
>>         final Class<?> k = Class.forName("sun.net.ext.ExtendedSocketOptions");
>>         final Object extSocketOptions = k.getDeclaredMethod("getInstance").invoke(null);
>>         Assert.assertNotNull(extSocketOptions, "sun.net.ext.ExtendedSocketOptions#getInstance()"
>>                 + " unexpectedly returned null");
>>     }
>> 
>>     private static class Task implements Callable<Class<?>> {
>> 
>>         private final String className;
>>         private final CountDownLatch latch;
>> 
>>         private Task(final String className, CountDownLatch latch) {
>>             this.className = className;
>>             this.latch = latch;
>>         }
>> 
>>         @Override
>>         public Class<?> call() {
>>             System.out.println(Thread.currentThread().getName() + " loading " + this.className);
>>             try {
>>                 latch.await();
>>                 return Class.forName(this.className);
>>             } catch (ClassNotFoundException | InterruptedException e) {
>>                 System.err.println("Failed to load " + this.className);
>>                 throw new RuntimeException(e);
>>             }
>>         }
>>     }
>> }
>
>> What about improving the test little bit, your test wants to load both classes at the same time. Please have a look on modified test.
> 
> Hello Vyom, I think that's a good suggestion to use a latch for deciding when to trigger the classloading. I've taken your input and have made some relatively minor change to the way that latch gets used and updated my PR with that change. The latch now waits for both the tasks to reach the point where they are going to do a `Class.forName` on their respectively class names. This should make the test trigger that classloading in separate threads as simultaneously as possible.

I think below change will address Andrey's concern 
public static ExtendedSocketOptions getInstance() {
        ExtendedSocketOptions temp = instance;
        if (temp == null) {
            synchronized (ExtendedSocketOptions.class) {
                try {
                    // If the class is present, it will be initialized which
                    // triggers registration of the extended socket options.
                    Class<?> c = Class.forName("jdk.net.ExtendedSocketOptions");
                } catch (ClassNotFoundException e) {
                    // the jdk.net module is not present => no extended socket options
                    instance = new NoExtendedSocketOptions();
                }
                temp = instance;
            }
        }
        return temp;
    }

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

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