8156871: Possible concurrency issue with JVM_AddModuleExports
authorlfoltan
Thu, 16 Jun 2016 13:34:32 -0400
changeset 39290 0cc9f5028562
parent 39289 a17f754703e5
child 39291 a4efe145313d
8156871: Possible concurrency issue with JVM_AddModuleExports Summary: Need for single PackageEntry flag to determine a package's unqualifed export state. Reviewed-by: acorn, ctornqvi, dholmes, jiangli
hotspot/src/share/vm/classfile/moduleEntry.cpp
hotspot/src/share/vm/classfile/modules.cpp
hotspot/src/share/vm/classfile/packageEntry.cpp
hotspot/src/share/vm/classfile/packageEntry.hpp
hotspot/test/runtime/modules/CompilerUtils.java
hotspot/test/runtime/modules/ModuleStress/ExportModuleStressTest.java
hotspot/test/runtime/modules/ModuleStress/src/jdk.test/module-info.java
hotspot/test/runtime/modules/ModuleStress/src/jdk.test/test/Main.java
hotspot/test/runtime/modules/ModuleStress/src/jdk.translet/module-info.java
hotspot/test/runtime/modules/ModuleStress/src/jdk.translet/translet/Main.java
--- a/hotspot/src/share/vm/classfile/moduleEntry.cpp	Wed Jun 15 09:48:24 2016 -0400
+++ b/hotspot/src/share/vm/classfile/moduleEntry.cpp	Thu Jun 16 13:34:32 2016 -0400
@@ -201,7 +201,7 @@
 }
 
 void ModuleEntryTable::create_unnamed_module(ClassLoaderData* loader_data) {
-  assert_locked_or_safepoint(Module_lock);
+  assert(Module_lock->owned_by_self(), "should have the Module_lock");
 
   // Each ModuleEntryTable has exactly one unnamed module
   if (loader_data->is_the_null_class_loader_data()) {
@@ -227,7 +227,7 @@
 ModuleEntry* ModuleEntryTable::new_entry(unsigned int hash, Handle module_handle, Symbol* name,
                                          Symbol* version, Symbol* location,
                                          ClassLoaderData* loader_data) {
-  assert_locked_or_safepoint(Module_lock);
+  assert(Module_lock->owned_by_self(), "should have the Module_lock");
   ModuleEntry* entry = (ModuleEntry*) NEW_C_HEAP_ARRAY(char, entry_size(), mtModule);
 
   // Initialize everything BasicHashtable would
@@ -258,7 +258,7 @@
 }
 
 void ModuleEntryTable::add_entry(int index, ModuleEntry* new_entry) {
-  assert_locked_or_safepoint(Module_lock);
+  assert(Module_lock->owned_by_self(), "should have the Module_lock");
   Hashtable<Symbol*, mtModule>::add_entry(index, (HashtableEntry<Symbol*, mtModule>*)new_entry);
 }
 
@@ -268,7 +268,7 @@
                                                            Symbol* module_location,
                                                            ClassLoaderData* loader_data) {
   assert(module_name != NULL, "ModuleEntryTable locked_create_entry_or_null should never be called for unnamed module.");
-  assert_locked_or_safepoint(Module_lock);
+  assert(Module_lock->owned_by_self(), "should have the Module_lock");
   // Check if module already exists.
   if (lookup_only(module_name) != NULL) {
     return NULL;
@@ -309,7 +309,7 @@
 }
 
 void ModuleEntryTable::finalize_javabase(Handle module_handle, Symbol* version, Symbol* location) {
-  assert_locked_or_safepoint(Module_lock);
+  assert(Module_lock->owned_by_self(), "should have the Module_lock");
   ClassLoaderData* boot_loader_data = ClassLoaderData::the_null_class_loader_data();
   ModuleEntryTable* module_table = boot_loader_data->modules();
 
--- a/hotspot/src/share/vm/classfile/modules.cpp	Wed Jun 15 09:48:24 2016 -0400
+++ b/hotspot/src/share/vm/classfile/modules.cpp	Thu Jun 16 13:34:32 2016 -0400
@@ -568,8 +568,8 @@
                       to_module_entry->is_named() ?
                         to_module_entry->name()->as_C_string() : UNNAMED_MODULE);
 
-  // Do nothing if modules are the same or if package is already exported unqualifiedly.
-  if (from_module_entry != to_module_entry && !package_entry->is_unqual_exported()) {
+  // Do nothing if modules are the same.
+  if (from_module_entry != to_module_entry) {
     package_entry->set_exported(to_module_entry);
   }
 }
--- a/hotspot/src/share/vm/classfile/packageEntry.cpp	Wed Jun 15 09:48:24 2016 -0400
+++ b/hotspot/src/share/vm/classfile/packageEntry.cpp	Thu Jun 16 13:34:32 2016 -0400
@@ -49,7 +49,7 @@
 
 // Add a module to the package's qualified export list.
 void PackageEntry::add_qexport(ModuleEntry* m) {
-  assert_locked_or_safepoint(Module_lock);
+  assert(Module_lock->owned_by_self(), "should have the Module_lock");
   if (!has_qual_exports_list()) {
     // Lazily create a package's qualified exports list.
     // Initial size is small, do not anticipate export lists to be large.
@@ -157,7 +157,7 @@
 }
 
 PackageEntry* PackageEntryTable::new_entry(unsigned int hash, Symbol* name, ModuleEntry* module) {
-  assert_locked_or_safepoint(Module_lock);
+  assert(Module_lock->owned_by_self(), "should have the Module_lock");
   PackageEntry* entry = (PackageEntry*) NEW_C_HEAP_ARRAY(char, entry_size(), mtModule);
 
   // Initialize everything BasicHashtable would
@@ -180,14 +180,14 @@
 }
 
 void PackageEntryTable::add_entry(int index, PackageEntry* new_entry) {
-  assert_locked_or_safepoint(Module_lock);
+  assert(Module_lock->owned_by_self(), "should have the Module_lock");
   Hashtable<Symbol*, mtModule>::add_entry(index, (HashtableEntry<Symbol*, mtModule>*)new_entry);
 }
 
 // Create package in loader's package entry table and return the entry.
 // If entry already exists, return null.  Assume Module lock was taken by caller.
 PackageEntry* PackageEntryTable::locked_create_entry_or_null(Symbol* name, ModuleEntry* module) {
-  assert_locked_or_safepoint(Module_lock);
+  assert(Module_lock->owned_by_self(), "should have the Module_lock");
   // Check if package already exists.  Return NULL if it does.
   if (lookup_only(name) != NULL) {
     return NULL;
--- a/hotspot/src/share/vm/classfile/packageEntry.hpp	Wed Jun 15 09:48:24 2016 -0400
+++ b/hotspot/src/share/vm/classfile/packageEntry.hpp	Thu Jun 16 13:34:32 2016 -0400
@@ -40,11 +40,7 @@
 //     package is exported to.
 //
 // Packages can be exported in the following 3 ways:
-//   - not exported:        the package has not been explicitly qualified to a
-//                            particular module nor has it been specified to be
-//                            unqualifiedly exported to all modules. If all states
-//                            of exportedness are false, the package is considered
-//                            not exported.
+//   - not exported:        the package does not have qualified or unqualified exports.
 //   - qualified exports:   the package has been explicitly qualified to at least
 //                            one particular module or has been qualifiedly exported
 //                            to all unnamed modules.
@@ -125,6 +121,7 @@
     return _is_exported_unqualified;
   }
   void set_unqual_exported() {
+    assert(Module_lock->owned_by_self(), "should have the Module_lock");
     _is_exported_unqualified = true;
     _is_exported_allUnnamed = false;
     _qualified_exports = NULL;
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/hotspot/test/runtime/modules/CompilerUtils.java	Thu Jun 16 13:34:32 2016 -0400
@@ -0,0 +1,79 @@
+/*
+ * Copyright (c) 2016, 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 javax.tools.JavaCompiler;
+import javax.tools.StandardJavaFileManager;
+import javax.tools.StandardLocation;
+import javax.tools.ToolProvider;
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.Arrays;
+import java.util.List;
+import java.util.stream.Collectors;
+
+/**
+ * This class consists exclusively of static utility methods for invoking the
+ * java compiler.
+ *
+ * This class will eventually move to jdk.testlibrary.
+ */
+
+public final class CompilerUtils {
+    private CompilerUtils() { }
+
+    /**
+     * Compile all the java sources in {@code <source>/**} to
+     * {@code <destination>/**}. The destination directory will be created if
+     * it doesn't exist.
+     *
+     * All warnings/errors emitted by the compiler are output to System.out/err.
+     *
+     * @return true if the compilation is successful
+     *
+     * @throws IOException if there is an I/O error scanning the source tree or
+     *                     creating the destination directory
+     */
+    public static boolean compile(Path source, Path destination, String ... options)
+        throws IOException
+    {
+        JavaCompiler compiler = ToolProvider.getSystemJavaCompiler();
+        StandardJavaFileManager jfm = compiler.getStandardFileManager(null, null, null);
+
+        List<Path> sources
+            = Files.find(source, Integer.MAX_VALUE,
+                (file, attrs) -> (file.toString().endsWith(".java")))
+                .collect(Collectors.toList());
+
+        Files.createDirectories(destination);
+        jfm.setLocationFromPaths(StandardLocation.CLASS_OUTPUT,
+                                 Arrays.asList(destination));
+
+        List<String> opts = Arrays.asList(options);
+        JavaCompiler.CompilationTask task
+            = compiler.getTask(null, jfm, null, opts, null,
+                jfm.getJavaFileObjectsFromPaths(sources));
+
+        return task.call();
+    }
+}
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/hotspot/test/runtime/modules/ModuleStress/ExportModuleStressTest.java	Thu Jun 16 13:34:32 2016 -0400
@@ -0,0 +1,80 @@
+/*
+ * Copyright (c) 2016, 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.
+ */
+
+/**
+ * @test
+ * @bug 8156871
+ * @summary package in the boot layer is repeatedly exported to unique module created in layers on top of the boot layer
+ * @modules java.base/jdk.internal.misc
+ * @library /testlibrary /test/lib
+ * @compile ../CompilerUtils.java
+ * @build ExportModuleStressTest
+ * @run main/othervm ExportModuleStressTest
+ */
+
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import jdk.test.lib.*;
+
+public class ExportModuleStressTest {
+
+    private static final String TEST_SRC = System.getProperty("test.src");
+    private static final String TEST_CLASSES = System.getProperty("test.classes");
+
+    private static final Path SRC_DIR = Paths.get(TEST_SRC, "src");
+    private static final Path MODS_DIR = Paths.get(TEST_CLASSES, "mods");
+
+    /**
+     * Compiles all module definitions used by the test
+     */
+    public static void main(String[] args) throws Exception {
+
+        boolean compiled;
+        // Compile module jdk.test declaration
+        compiled = CompilerUtils.compile(
+            SRC_DIR.resolve("jdk.test"),
+            MODS_DIR.resolve("jdk.test"));
+        if (!compiled) {
+            throw new RuntimeException("Test failed to compile module jdk.test");
+        }
+
+        // Compile module jdk.translet declaration
+        compiled = CompilerUtils.compile(
+            SRC_DIR.resolve("jdk.translet"),
+            MODS_DIR.resolve("jdk.translet"),
+            "-XaddExports:jdk.test/test=jdk.translet",
+            "-mp", MODS_DIR.toString());
+        if (!compiled) {
+            throw new RuntimeException("Test failed to compile module jdk.translet");
+        }
+
+        // Sanity check that the test, jdk.test/test/Main.java
+        // runs without error.
+        ProcessBuilder pb = ProcessTools.createJavaProcessBuilder(
+            "-mp", MODS_DIR.toString(),
+            "-m", "jdk.test/test.Main");
+        OutputAnalyzer output = new OutputAnalyzer(pb.start());
+        output.shouldContain("failed: 0")
+              .shouldHaveExitValue(0);
+    }
+}
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/hotspot/test/runtime/modules/ModuleStress/src/jdk.test/module-info.java	Thu Jun 16 13:34:32 2016 -0400
@@ -0,0 +1,25 @@
+/*
+ * Copyright (c) 2016, 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.
+ */
+
+module jdk.test {
+}
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/hotspot/test/runtime/modules/ModuleStress/src/jdk.test/test/Main.java	Thu Jun 16 13:34:32 2016 -0400
@@ -0,0 +1,105 @@
+/*
+ * Copyright (c) 2016, 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.
+ */
+
+package test;
+
+import java.lang.module.Configuration;
+import java.lang.module.ModuleFinder;
+import java.lang.reflect.Layer;
+import java.lang.reflect.Method;
+import java.lang.reflect.Module;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.*;
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+
+public class Main {
+
+    private static final Path MODS_DIR = Paths.get(System.getProperty("jdk.module.path"));
+    static final String MODULE_NAME = "jdk.translet";
+
+    public static void main(String[] args) throws Exception {
+
+        ModuleFinder finder = ModuleFinder.of(MODS_DIR);
+        Layer layerBoot = Layer.boot();
+
+        Configuration cf = layerBoot
+                .configuration()
+                .resolveRequires(ModuleFinder.of(), finder, Set.of(MODULE_NAME));
+
+        Module testModule = Main.class.getModule();
+        ClassLoader scl = ClassLoader.getSystemClassLoader();
+
+        // Create an unique module/class loader in a layer above the boot layer.
+        // Export this module to the jdk.test/test package.
+        Callable<Void> task = new Callable<Void>() {
+            @Override
+            public Void call() throws Exception {
+                Layer layer = Layer.boot().defineModulesWithOneLoader(cf, scl);
+                Module transletModule = layer.findModule(MODULE_NAME).get();
+                testModule.addExports("test", transletModule);
+                Class<?> c = layer.findLoader(MODULE_NAME).loadClass("translet.Main");
+                Method method = c.getDeclaredMethod("go");
+                method.invoke(null);
+                return null;
+            }
+        };
+
+        List<Future<Void>> results = new ArrayList<>();
+
+        // Repeatedly create the layer above stressing the exportation of
+        // package jdk.test/test to several different modules.
+        ExecutorService pool = Executors.newFixedThreadPool(Math.min(100, Runtime.getRuntime().availableProcessors()*10));
+        try {
+            for (int i = 0; i < 10000; i++) {
+                results.add(pool.submit(task));
+            }
+        } finally {
+            pool.shutdown();
+        }
+
+        int passed = 0;
+        int failed = 0;
+
+        // The failed state should be 0, the created modules in layers above the
+        // boot layer should be allowed access to the contents of the jdk.test/test
+        // package since that package was exported to the transletModule above.
+        for (Future<Void> result : results) {
+            try {
+                result.get();
+                passed++;
+            } catch (Throwable x) {
+                x.printStackTrace();
+                failed++;
+            }
+        }
+
+        System.out.println("passed: " + passed);
+        System.out.println("failed: " + failed);
+    }
+
+    public static void callback() { }
+}
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/hotspot/test/runtime/modules/ModuleStress/src/jdk.translet/module-info.java	Thu Jun 16 13:34:32 2016 -0400
@@ -0,0 +1,27 @@
+/*
+ * Copyright (c) 2016, 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.
+ */
+
+module jdk.translet {
+    requires jdk.test;
+    exports translet;
+}
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/hotspot/test/runtime/modules/ModuleStress/src/jdk.translet/translet/Main.java	Thu Jun 16 13:34:32 2016 -0400
@@ -0,0 +1,30 @@
+/*
+ * Copyright (c) 2016, 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.
+ */
+
+package translet;
+
+public class Main {
+    public static void go() {
+        test.Main.callback();
+    }
+}