From c78bc8d75c1735976f5eb6f7f868076d84cd1c73 Mon Sep 17 00:00:00 2001 From: Andy Herrick Date: Thu, 17 Dec 2020 20:30:36 +0000 Subject: [PATCH 1/3] JDK-8223322: Improve concurrency in jpackage instances --- .../jpackage/internal/MacAppImageBuilder.java | 19 ++++--- .../jpackage/internal/MacAppStoreBundler.java | 5 +- .../jdk/jpackage/internal/Arguments.java | 25 +++----- .../internal/JPackageToolProvider.java | 3 +- .../classes/jdk/jpackage/internal/Log.java | 57 ++++++------------- .../share/classes/jdk/jpackage/main/Main.java | 15 ++--- .../jdk/jpackage/test/PackageType.java | 15 +++++ 7 files changed, 59 insertions(+), 80 deletions(-) diff --git a/src/jdk.jpackage/macosx/classes/jdk/jpackage/internal/MacAppImageBuilder.java b/src/jdk.jpackage/macosx/classes/jdk/jpackage/internal/MacAppImageBuilder.java index 4dc58ce40e8de..49e60ff53e5d6 100644 --- a/src/jdk.jpackage/macosx/classes/jdk/jpackage/internal/MacAppImageBuilder.java +++ b/src/jdk.jpackage/macosx/classes/jdk/jpackage/internal/MacAppImageBuilder.java @@ -86,8 +86,6 @@ public class MacAppImageBuilder extends AbstractAppImageBuilder { private final Path runtimeDir; private final Path runtimeRoot; - private static List keyChains; - public static final BundlerParamInfo MAC_CONFIGURE_LAUNCHER_IN_PLIST = new StandardBundlerParam<>( "mac.configure-launcher-in-plist", @@ -324,10 +322,12 @@ private void copyRuntimeFiles(Map params) } private void sign(Map params) throws IOException { + List keyChains = new ArrayList<>(); + if (Optional.ofNullable( SIGN_BUNDLE.fetchFrom(params)).orElse(Boolean.TRUE)) { try { - addNewKeychain(params); + addNewKeychain(keyChains, params); } catch (InterruptedException e) { Log.error(e.getMessage()); } @@ -339,7 +339,7 @@ private void sign(Map params) throws IOException { BUNDLE_ID_SIGNING_PREFIX.fetchFrom(params), getConfig_Entitlements(params)); } - restoreKeychainList(params); + restoreKeychainList(keyChains, params); } } @@ -540,8 +540,9 @@ private void writePkgInfo(Path file) throws IOException { } } - public static void addNewKeychain(Map params) - throws IOException, InterruptedException { + public static void addNewKeychain(List keyChains, + Map params) + throws IOException, InterruptedException { if (Platform.getMajorVersion() < 10 || (Platform.getMajorVersion() == 10 && Platform.getMinorVersion() < 12)) { @@ -571,7 +572,7 @@ public static void addNewKeychain(Map params) return; } - keyChains = new ArrayList<>(); + keyChains.clear(); // remove " keychainList.forEach((String s) -> { String path = s.trim(); @@ -593,8 +594,8 @@ public static void addNewKeychain(Map params) IOUtils.exec(pb); } - public static void restoreKeychainList(Map params) - throws IOException{ + public static void restoreKeychainList(List keyChains, + Map params) throws IOException { if (Platform.getMajorVersion() < 10 || (Platform.getMajorVersion() == 10 && Platform.getMinorVersion() < 12)) { diff --git a/src/jdk.jpackage/macosx/classes/jdk/jpackage/internal/MacAppStoreBundler.java b/src/jdk.jpackage/macosx/classes/jdk/jpackage/internal/MacAppStoreBundler.java index d74e23570401a..8357c8a95baea 100644 --- a/src/jdk.jpackage/macosx/classes/jdk/jpackage/internal/MacAppStoreBundler.java +++ b/src/jdk.jpackage/macosx/classes/jdk/jpackage/internal/MacAppStoreBundler.java @@ -106,6 +106,7 @@ public class MacAppStoreBundler extends MacBaseInstallerBundler { public Path bundle(Map params, Path outdir) throws PackagerException { + List keyChains = new ArrayList<>(); Log.verbose(MessageFormat.format(I18N.getString( "message.building-bundle"), APP_NAME.fetchFrom(params))); @@ -121,7 +122,7 @@ public Path bundle(Map params, Files.createDirectories(appImageDir); try { - MacAppImageBuilder.addNewKeychain(params); + MacAppImageBuilder.addNewKeychain(keyChains, params); } catch (InterruptedException e) { Log.error(e.getMessage()); } @@ -138,7 +139,7 @@ public Path bundle(Map params, MacAppImageBuilder.signAppBundle(params, appLocation, signingIdentity, identifierPrefix, MacAppImageBuilder.getConfig_Entitlements(params)); - MacAppImageBuilder.restoreKeychainList(params); + MacAppImageBuilder.restoreKeychainList(keyChains, params); ProcessBuilder pb; diff --git a/src/jdk.jpackage/share/classes/jdk/jpackage/internal/Arguments.java b/src/jdk.jpackage/share/classes/jdk/jpackage/internal/Arguments.java index ae44d28b8f7a8..574d49cef3720 100644 --- a/src/jdk.jpackage/share/classes/jdk/jpackage/internal/Arguments.java +++ b/src/jdk.jpackage/share/classes/jdk/jpackage/internal/Arguments.java @@ -100,12 +100,12 @@ public class Arguments { private String buildRoot = null; private String mainJarPath = null; - private static boolean runtimeInstaller = false; + private boolean runtimeInstaller = false; private List addLaunchers = null; - private static Map argIds = new HashMap<>(); - private static Map argShortIds = new HashMap<>(); + private static final Map argIds = new HashMap<>(); + private static final Map argShortIds = new HashMap<>(); static { // init maps for parsing arguments @@ -117,7 +117,12 @@ public class Arguments { }); } + private static final InheritableThreadLocal instance = + new InheritableThreadLocal(); + public Arguments(String[] args) { + instance.set(this); + argList = new ArrayList(args.length); for (String arg : args) { argList.add(arg); @@ -392,16 +397,8 @@ private CLIOptions(String id, String shortId, this.category = category; } - static void setContext(Arguments context) { - argContext = context; - } - public static Arguments context() { - if (argContext != null) { - return argContext; - } else { - throw new RuntimeException("Argument context is not set."); - } + return instance.get(); } public String getId() { @@ -462,10 +459,6 @@ enum OptionCategories { public boolean processArguments() { try { - - // init context of arguments - CLIOptions.setContext(this); - // parse cmd line String arg; CLIOptions option; diff --git a/src/jdk.jpackage/share/classes/jdk/jpackage/internal/JPackageToolProvider.java b/src/jdk.jpackage/share/classes/jdk/jpackage/internal/JPackageToolProvider.java index 18f7f5df21a9c..6173bc696ccd2 100644 --- a/src/jdk.jpackage/share/classes/jdk/jpackage/internal/JPackageToolProvider.java +++ b/src/jdk.jpackage/share/classes/jdk/jpackage/internal/JPackageToolProvider.java @@ -40,8 +40,7 @@ public String name() { return "jpackage"; } - public synchronized int run( - PrintWriter out, PrintWriter err, String... args) { + public int run(PrintWriter out, PrintWriter err, String... args) { try { return new jdk.jpackage.main.Main().execute(out, err, args); } catch (RuntimeException re) { diff --git a/src/jdk.jpackage/share/classes/jdk/jpackage/internal/Log.java b/src/jdk.jpackage/share/classes/jdk/jpackage/internal/Log.java index c20066075d233..3bef0f0fee53b 100644 --- a/src/jdk.jpackage/share/classes/jdk/jpackage/internal/Log.java +++ b/src/jdk.jpackage/share/classes/jdk/jpackage/internal/Log.java @@ -75,16 +75,12 @@ public void flush() { public void info(String msg) { if (out != null) { out.println(msg); - } else { - System.out.println(msg); } } public void fatalError(String msg) { if (err != null) { err.println(msg); - } else { - System.err.println(msg); } } @@ -92,8 +88,6 @@ public void error(String msg) { msg = addTimestamp(msg); if (err != null) { err.println(msg); - } else { - System.err.println(msg); } } @@ -101,9 +95,6 @@ public void verbose(Throwable t) { if (out != null && verbose) { out.print(addTimestamp("")); t.printStackTrace(out); - } else if (verbose) { - System.out.print(addTimestamp("")); - t.printStackTrace(System.out); } } @@ -111,8 +102,6 @@ public void verbose(String msg) { msg = addTimestamp(msg); if (out != null && verbose) { out.println(msg); - } else if (verbose) { - System.out.println(msg); } } @@ -142,62 +131,50 @@ private String addTimestamp(String msg) { } } - private static Logger delegate = null; + private static final InheritableThreadLocal instance = + new InheritableThreadLocal() { + @Override protected Logger initialValue() { + return new Logger(); + } + }; - public static void setLogger(Logger logger) { - delegate = (logger != null) ? logger : new Logger(); + public static void setPrintWriter (PrintWriter out, PrintWriter err) { + instance.get().setPrintWriter(out, err); } public static void flush() { - if (delegate != null) { - delegate.flush(); - } + instance.get().flush(); } public static void info(String msg) { - if (delegate != null) { - delegate.info(msg); - } + instance.get().info(msg); } public static void fatalError(String msg) { - if (delegate != null) { - delegate.fatalError(msg); - } + instance.get().fatalError(msg); } public static void error(String msg) { - if (delegate != null) { - delegate.error(msg); - } + instance.get().error(msg); } public static void setVerbose() { - if (delegate != null) { - delegate.setVerbose(); - } + instance.get().setVerbose(); } public static boolean isVerbose() { - return (delegate != null) ? delegate.isVerbose() : false; + return instance.get().isVerbose(); } public static void verbose(String msg) { - if (delegate != null) { - delegate.verbose(msg); - } + instance.get().verbose(msg); } public static void verbose(Throwable t) { - if (delegate != null) { - delegate.verbose(t); - } + instance.get().verbose(t); } public static void verbose(List strings, List out, int ret) { - if (delegate != null) { - delegate.verbose(strings, out, ret); - } + instance.get().verbose(strings, out, ret); } - } diff --git a/src/jdk.jpackage/share/classes/jdk/jpackage/main/Main.java b/src/jdk.jpackage/share/classes/jdk/jpackage/main/Main.java index 1381902f52ac2..467e13571342a 100644 --- a/src/jdk.jpackage/share/classes/jdk/jpackage/main/Main.java +++ b/src/jdk.jpackage/share/classes/jdk/jpackage/main/Main.java @@ -46,10 +46,10 @@ public class Main { * @param args command line arguments */ public static void main(String... args) throws Exception { - // Create logger with default system.out and system.err - Log.setLogger(null); - int status = new jdk.jpackage.main.Main().execute(args); + PrintWriter out = new PrintWriter(System.out); + PrintWriter err = new PrintWriter(System.err); + int status = new jdk.jpackage.main.Main().execute(out, err, args); System.exit(status); } @@ -62,15 +62,8 @@ public static void main(String... args) throws Exception { * @return an exit code. 0 means success, non-zero means an error occurred. */ public int execute(PrintWriter out, PrintWriter err, String... args) { - // Create logger with provided streams - Log.Logger logger = new Log.Logger(); - logger.setPrintWriter(out, err); - Log.setLogger(logger); + Log.setPrintWriter(out, err); - return execute(args); - } - - private int execute(String... args) { try { String[] newArgs; try { diff --git a/test/jdk/tools/jpackage/helpers/jdk/jpackage/test/PackageType.java b/test/jdk/tools/jpackage/helpers/jdk/jpackage/test/PackageType.java index 26a1b7786a4a5..9c8a8bda09b70 100644 --- a/test/jdk/tools/jpackage/helpers/jdk/jpackage/test/PackageType.java +++ b/test/jdk/tools/jpackage/helpers/jdk/jpackage/test/PackageType.java @@ -60,6 +60,21 @@ public enum PackageType { } } + public static PackageType getDefault() { + if (TKit.isWindows()) { + return WIN_EXE; + } else if (TKit.isOSX()) { + return MAC_DMG; + } else if (TKit.isLinux()) { + if (LINUX_DEB.isSupported()) { + return LINUX_DEB; + } else if (LINUX_RPM.isSupported()) { + return LINUX_RPM; + } + } + throw new RuntimeException("Failed to determine default package type"); + } + PackageType(String bundleSuffix, String bundlerClass) { this(bundleSuffix.substring(1), bundleSuffix, bundlerClass); } From dfa7c5072f5a341aeaddd855823357f958d1073a Mon Sep 17 00:00:00 2001 From: Andy Herrick Date: Thu, 17 Dec 2020 20:51:27 +0000 Subject: [PATCH 2/3] JDK-8223322: Improve concurrency in jpackage instances --- .../tools/jpackage/share/ConcurrentTest.java | 103 ++++++++++++++++++ 1 file changed, 103 insertions(+) create mode 100644 test/jdk/tools/jpackage/share/ConcurrentTest.java diff --git a/test/jdk/tools/jpackage/share/ConcurrentTest.java b/test/jdk/tools/jpackage/share/ConcurrentTest.java new file mode 100644 index 0000000000000..558fe2a47dd62 --- /dev/null +++ b/test/jdk/tools/jpackage/share/ConcurrentTest.java @@ -0,0 +1,103 @@ +/* + * Copyright (c) 2018, 2020, 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 java.util.Date; +import jdk.jpackage.test.TKit; +import jdk.jpackage.test.PackageType; +import jdk.jpackage.test.JPackageCommand; +import jdk.jpackage.test.Annotations.Test; + +/** + * Concurrent test. Using ToolProvider, run several jpackage test concurrently + */ + +/* + * @test + * @summary Concurrent jpackage command runs using ToolProvider + * @library ../helpers + * @key jpackagePlatformPackage + * @build jdk.jpackage.test.* + * @modules jdk.jpackage/jdk.jpackage.internal + * @compile ConcurrentTest.java + * @run main/othervm/timeout=360 -Xmx512m jdk.jpackage.test.Main + * --jpt-run=ConcurrentTest + */ +public class ConcurrentTest { + + @Test + public static void test() { + + final JPackageCommand cmd1 = + JPackageCommand.helloAppImage("com.other/com.other.Hello") + .useToolProvider(true) + .setPackageType(PackageType.getDefault()) + .setArgumentValue("--name", "ConcurrentOtherInstaller"); + + final JPackageCommand cmd2 = + JPackageCommand.helloAppImage("Hello") + .useToolProvider(true) + .setPackageType(PackageType.IMAGE) + .setArgumentValue("--name", "ConcurrentAppImage"); + + Date[] times = race(cmd1, cmd2); + TKit.assertTrue(times[0].after(times[1]), + "We expected app-image command to finish first, but times[0] is " + + times[0] + " and times[1] is" + times[1]); + + cmd1.useToolProvider(false); + cmd1.useToolProvider(false); + + times = race(cmd1, cmd2); + TKit.assertTrue(times[0].after(times[1]), + "We expected app-image command to finish first, but times[0] is " + + times[0] + " and times[1] is" + times[1]); + } + + private static Date[] race(JPackageCommand cmd1, JPackageCommand cmd2) { + final Date[] times = new Date[2]; + + Thread t1 = new Thread(new Runnable() { + public void run() { + cmd1.execute(); + times[0] = new Date(); + } + }); + + Thread t2 = new Thread(new Runnable() { + public void run() { + cmd2.execute(); + times[1] = new Date(); + } + }); + try { + t1.start(); + t2.start(); + + t1.join(); + t2.join(); + return times; + } catch (Exception e) { + throw new RuntimeException(e); + } + } +} From 2baef3238d63bbbf4dce196d067273b7866fda57 Mon Sep 17 00:00:00 2001 From: Andy Herrick Date: Mon, 4 Jan 2021 15:07:32 +0000 Subject: [PATCH 3/3] JDK-8223322: Improve concurrency in jpackage instances --- .../jdk/jpackage/internal/IOUtils.java | 2 +- .../jpackage/internal/JLinkBundlerHelper.java | 2 +- .../jdk/jpackage/test/PackageType.java | 15 --- .../tools/jpackage/share/ConcurrentTest.java | 111 +++++++++--------- 4 files changed, 60 insertions(+), 70 deletions(-) diff --git a/src/jdk.jpackage/share/classes/jdk/jpackage/internal/IOUtils.java b/src/jdk.jpackage/share/classes/jdk/jpackage/internal/IOUtils.java index a82f5ff457279..4b48ace3d6a86 100644 --- a/src/jdk.jpackage/share/classes/jdk/jpackage/internal/IOUtils.java +++ b/src/jdk.jpackage/share/classes/jdk/jpackage/internal/IOUtils.java @@ -173,7 +173,7 @@ static void exec(ProcessBuilder pb, boolean testForPresenceOnly, exec(pb, testForPresenceOnly, consumer, false, Executor.INFINITE_TIMEOUT); } - static void exec(ProcessBuilder pb, boolean testForPresenceOnly, + static synchronized void exec(ProcessBuilder pb, boolean testForPresenceOnly, PrintStream consumer, boolean writeOutputToFile, long timeout) throws IOException { List output = new ArrayList<>(); diff --git a/src/jdk.jpackage/share/classes/jdk/jpackage/internal/JLinkBundlerHelper.java b/src/jdk.jpackage/share/classes/jdk/jpackage/internal/JLinkBundlerHelper.java index 343c469f93160..104f35403a1a1 100644 --- a/src/jdk.jpackage/share/classes/jdk/jpackage/internal/JLinkBundlerHelper.java +++ b/src/jdk.jpackage/share/classes/jdk/jpackage/internal/JLinkBundlerHelper.java @@ -154,7 +154,7 @@ private static Set createModuleList(List paths, return modules; } - private static void runJLink(Path output, List modulePath, + private static synchronized void runJLink(Path output, List modulePath, Set modules, Set limitModules, List options) throws PackagerException, IOException { diff --git a/test/jdk/tools/jpackage/helpers/jdk/jpackage/test/PackageType.java b/test/jdk/tools/jpackage/helpers/jdk/jpackage/test/PackageType.java index 9c8a8bda09b70..26a1b7786a4a5 100644 --- a/test/jdk/tools/jpackage/helpers/jdk/jpackage/test/PackageType.java +++ b/test/jdk/tools/jpackage/helpers/jdk/jpackage/test/PackageType.java @@ -60,21 +60,6 @@ public enum PackageType { } } - public static PackageType getDefault() { - if (TKit.isWindows()) { - return WIN_EXE; - } else if (TKit.isOSX()) { - return MAC_DMG; - } else if (TKit.isLinux()) { - if (LINUX_DEB.isSupported()) { - return LINUX_DEB; - } else if (LINUX_RPM.isSupported()) { - return LINUX_RPM; - } - } - throw new RuntimeException("Failed to determine default package type"); - } - PackageType(String bundleSuffix, String bundlerClass) { this(bundleSuffix.substring(1), bundleSuffix, bundlerClass); } diff --git a/test/jdk/tools/jpackage/share/ConcurrentTest.java b/test/jdk/tools/jpackage/share/ConcurrentTest.java index 558fe2a47dd62..8fce294b75d27 100644 --- a/test/jdk/tools/jpackage/share/ConcurrentTest.java +++ b/test/jdk/tools/jpackage/share/ConcurrentTest.java @@ -21,11 +21,20 @@ * questions. */ -import java.util.Date; import jdk.jpackage.test.TKit; -import jdk.jpackage.test.PackageType; -import jdk.jpackage.test.JPackageCommand; +import jdk.jpackage.test.PackageTest; +import jdk.jpackage.test.JavaAppDesc; import jdk.jpackage.test.Annotations.Test; +import jdk.jpackage.test.Annotations.Parameter; +import jdk.jpackage.test.Functional; +import jdk.jpackage.test.HelloApp; +import java.util.ArrayList; +import java.util.List; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.Executors; +import java.util.concurrent.ExecutorService; +import java.nio.file.Path; + /** * Concurrent test. Using ToolProvider, run several jpackage test concurrently @@ -39,65 +48,61 @@ * @build jdk.jpackage.test.* * @modules jdk.jpackage/jdk.jpackage.internal * @compile ConcurrentTest.java - * @run main/othervm/timeout=360 -Xmx512m jdk.jpackage.test.Main + * @run main/othervm/timeout=480 -Xmx512m jdk.jpackage.test.Main * --jpt-run=ConcurrentTest */ public class ConcurrentTest { - @Test - public static void test() { - - final JPackageCommand cmd1 = - JPackageCommand.helloAppImage("com.other/com.other.Hello") - .useToolProvider(true) - .setPackageType(PackageType.getDefault()) - .setArgumentValue("--name", "ConcurrentOtherInstaller"); + final int TEST_COUNT = 3; // default number of jpackage commands to run - final JPackageCommand cmd2 = - JPackageCommand.helloAppImage("Hello") - .useToolProvider(true) - .setPackageType(PackageType.IMAGE) - .setArgumentValue("--name", "ConcurrentAppImage"); - - Date[] times = race(cmd1, cmd2); - TKit.assertTrue(times[0].after(times[1]), - "We expected app-image command to finish first, but times[0] is " - + times[0] + " and times[1] is" + times[1]); + @Test + public void test() throws Exception { + final Path inputDir = TKit.workDir().resolve("input"); + int count = TEST_COUNT; + String propValue = System.getProperty("jpackage.concurrent.count"); + if (propValue != null) { + try { + count = Integer.parseInt(propValue); + } catch (Exception e) { + // ignore - use default count + } + } + long timeout = 2L * count; // minutes to run tests before timeout + HelloApp.createBundle(JavaAppDesc.parse("hello.jar:Hello"), inputDir); - cmd1.useToolProvider(false); - cmd1.useToolProvider(false); + List tasks = new ArrayList<>(); + for (int i = 0; i < count; i++) { + tasks.add(Functional.ThrowingRunnable.toRunnable(() -> + initTest(inputDir).run( + PackageTest.Action.CREATE))); + } - times = race(cmd1, cmd2); - TKit.assertTrue(times[0].after(times[1]), - "We expected app-image command to finish first, but times[0] is " - + times[0] + " and times[1] is" + times[1]); + ExecutorService exec = Executors.newCachedThreadPool(); + tasks.stream().forEach(exec::execute); + exec.shutdown(); + boolean finished = exec.awaitTermination(timeout, TimeUnit.MINUTES); + // even if we are throwing assertion below we need to try to stop these + // threads before exiting + if (!finished) { + exec.shutdownNow(); + } + TKit.assertTrue(finished, "Executing jpackage " + count + + " times timed out after " + timeout + " minutes."); } - private static Date[] race(JPackageCommand cmd1, JPackageCommand cmd2) { - final Date[] times = new Date[2]; - - Thread t1 = new Thread(new Runnable() { - public void run() { - cmd1.execute(); - times[0] = new Date(); - } - }); - - Thread t2 = new Thread(new Runnable() { - public void run() { - cmd2.execute(); - times[1] = new Date(); - } - }); - try { - t1.start(); - t2.start(); - - t1.join(); - t2.join(); - return times; - } catch (Exception e) { - throw new RuntimeException(e); + private PackageTest initTest(Path inputDir) + throws Exception { + final Path outputDir; + synchronized (this) { + outputDir = TKit.createTempDirectory("output"); } + return new PackageTest().addInitializer(cmd -> { + cmd.useToolProvider(true); + cmd.setArgumentValue("--input", inputDir); + cmd.setArgumentValue("--main-class", "Hello"); + cmd.setArgumentValue("--main-jar", "hello.jar"); + cmd.setArgumentValue("--dest", outputDir); + cmd.addArguments("--verbose"); + }); } }