On Thu, 18 Feb 2021 05:01:41 GMT, Jaikiran Pai <j...@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
