Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JDK-8223322: Improve concurrency in jpackage instances #1829

Closed
wants to merge 3 commits into from

Conversation

@andyherrick
Copy link

@andyherrick andyherrick commented Dec 17, 2020

Remove all non final static variables in jpackage java code (using InheritableThreadLocal for Logger and Argument instances) and remove sychronization in JPackageToolProvider.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8223322: Improve concurrency in jpackage instances

Download

$ git fetch https://git.openjdk.java.net/jdk pull/1829/head:pull/1829
$ git checkout pull/1829

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Dec 17, 2020

👋 Welcome back herrick! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

@openjdk openjdk bot commented Dec 17, 2020

@andyherrick The following label will be automatically applied to this pull request:

  • core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the core-libs label Dec 17, 2020
@andyherrick andyherrick marked this pull request as ready for review Dec 17, 2020
@openjdk openjdk bot added the rfr label Dec 17, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Dec 17, 2020

Webrevs

@sashamatveev
Copy link
Member

@sashamatveev sashamatveev commented Dec 17, 2020

Changes looks fine, but are we sure that external 3rd party tools used by jpackage will work concurrently without any issues?

final JPackageCommand cmd1 =
JPackageCommand.helloAppImage("com.other/com.other.Hello")
.useToolProvider(true)
.setPackageType(PackageType.getDefault())

This comment has been minimized.

@alexeysemenyukoracle

alexeysemenyukoracle Dec 17, 2020
Member

.removeArgumentWithValue("--type") can be used to make jpackage create default native package. This will eliminate need to introduce packageType.getDefault() method.

However, I'd rather replace JPackageCommand with PackageTest in this test scenario. Something like this:

final PackageTest cmd1 = new PackageTest()
.configureHelloApp()
.addInitializer(cmd -> {
    cmd.setArgumentValue("--name", "ConcurrentOtherInstaller");
});

cmd1.run(PackageTest.Action.CREATE); would result in a package creation but will no unpack or install will be attempted.

PackageTest is better as it will run tests on created package making sure it is valid.

@alexeysemenyukoracle
Copy link
Member

@alexeysemenyukoracle alexeysemenyukoracle commented Dec 17, 2020

I'd propose to update the test to run more that two concurrent instance of jpackage command.

+ times[0] + " and times[1] is" + times[1]);

cmd1.useToolProvider(false);
cmd1.useToolProvider(false);

This comment has been minimized.

@alexeysemenyukoracle

alexeysemenyukoracle Dec 17, 2020
Member

Shouldn't this be cmd2?

This comment has been minimized.

@alexeysemenyukoracle

alexeysemenyukoracle Dec 17, 2020
Member

I'd parametrize the test function with useToolProvider parameter. This will help to avoid copy/paste of TKit.assertTrue(times...)

@andyherrick
Copy link
Author

@andyherrick andyherrick commented Dec 18, 2020

Changes looks fine, but are we sure that external 3rd party tools used by jpackage will work concurrently without any issues?

I don't think we can be sure of thread-safe behavior of the third party tools, the best thing we can do is thoroughly test (as requested below, run a lot more instances simultaneously). Main thing we are implementing and testing is that jpackage code itself is thread safe.

@andyherrick
Copy link
Author

@andyherrick andyherrick commented Dec 18, 2020

I'd propose to update the test to run more that two concurrent instance of jpackage command.

yes - I'm planning for a lot more in next revision, your suggestion to use only PackageTest.Action.CREATE will go a long way to making that possible, but I have had problems with test infrastructure fixing the workDir based on test class and method, and then getting IOExceptions as various PackageTest's try to write the same content to the same work dir. I need to create infrastructure to allow each PackageTest to operate on separate workDir.

@andyherrick
Copy link
Author

@andyherrick andyherrick commented Dec 18, 2020

Reverting this PR to DRAFT while the above suggestions are implemented.

@andyherrick andyherrick marked this pull request as draft Dec 18, 2020
@openjdk openjdk bot removed the rfr label Dec 18, 2020
@andyherrick
Copy link
Author

@andyherrick andyherrick commented Dec 18, 2020

.removeArgumentWithValue("--type") can be used to make jpackage create default native package. This will eliminate need to introduce packageType.getDefault() method.

The above will cause an exception from JPackageCommand.execute() > ... > JPackageCommand.verifyIsOfType(), but perhaps that problem will go away if only JPackageTest.run(PackageTest.Action.CREATE) is used.

@alexeysemenyukoracle
Copy link
Member

@alexeysemenyukoracle alexeysemenyukoracle commented Dec 18, 2020

I have had problems with test infrastructure fixing the workDir based on test class and method, and then getting IOExceptions as various PackageTest's try to write the same content to the same work dir

I don't think we need changes to test infrastructure to run PackageTest instances concurrently.

This is how to create input for jpackage:

final Path inputDir = TKit.workDir().resolve("input");
HelloApp.createBundle(JavaAppDesc.parse("hellp.jar:Hello"), inputDir);

This is the function to initialize PackegTest instance that will use hello.jar initialized above to build a package and write output in unique directory inside of test work directory:

private PackageTest initTest(Path inputDir, bool useToolProvider) {
    // Just in case if TKit.createTempDirectory is not reliable enough to be executed concurrently
    final Path outputDir;
    synchronized (this) {
        outputDir = TKit.createTempDirectory("output");
    }
    return new PackageTest()
        .addInitializer(cmd -> {
            cmd.useToolProvider(useToolProvider);
            cmd.setArgumentValue("--input", inputDir);
            cmd.setArgumentValue("--main-class", "Hello");
            cmd.setArgumentValue("--main-jar", "hello.jar");
            cmd.setArgumentValue("--dest", outputDir);
        });
}

Glue it all together:

@Test
@Parameter("true")
@Parameter("false")
public void testIt(bool useToolProvider) {
    final Path inputDir = TKit.workDir().resolve("input");
    HelloApp.createBundle(JavaAppDesc.parse("hellp.jar:Hello"), inputDir);

   final int TEST_COUNT = 5;
    List<Runnable> tasks = new ArrayList<>();
    for (int i = 0; i != TEST_COUNT; i++) {
        tests.push(ThrowingRunnable.toRunnable(() -> initTest(inputDir, useToolProvider).run(PackageTest.Action.CREATE)));
    }

    ExecutorService exec = Executors.newCachedThreadPool();
    tasks.stream().forEach(exec::execute);
    exec.shutdown();
}
@andyherrick andyherrick deleted the andyherrick:JDK-8223322 branch Jan 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants