Fix race condition if tests are executed in parallel #195

melix · 2022-02-01T16:00:50Z

This commit makes use of the native-image-configure tool to merge
the results of agent files. Before this commit, several process could
be writing to the same output directory. Now, each process will use
a separate directory and the tool is used to merge their outputs.

Because the native-image-configure tool might not be present,
it's also going to invoke the command which generates it.

See #194

melix · 2022-02-03T13:17:29Z

@lazar-mitrovic PTAL, all tests are green now. Maven was again quite challenging.

lazar-mitrovic · 2022-02-07T15:10:46Z

native-gradle-plugin/src/main/java/org/graalvm/buildtools/gradle/MergeAgentFiles.java

+                        spec.args(args);
+                        spec.setStandardOutput(System.out);
+                        spec.setErrorOutput(System.err);
+                    });


IMO we should migrate logic that is currently located in ProcessGeneratedGraalResourceFiles to this native-image-configure invocation.
So something like:

args.add("--exclude-classes=java.**"); args.add("--exclude-classes=org.gradle.**");

(we should also probably make that configurable somehow).

That would be useful as a workaround for oracle/graal#3968 that is one of blockers for Mockito support (cc @raphw):

args.add("--exclude-classes=net.bytebuddy.utility.Invoker$Dispatcher");

We should then probably change MergeAgentFiles to something like ProcessAgentFiles.

I am not convinced we should merge them. The "merge" logic can be seen as an internal implementation detail of the agent implementation: instead of outputting n files, it should output a single file. Therefore, this logic needs to be invoked as part of the task execution itself, so that, for example, we don't mix files from different invocations.

The "process agent files" task, on the other hand, is, as its name states, a post processing task which, yes, could be made more configurable, but I don't think we should collate both aspects.

That sounds reasonable. I have no strong opinions about this, so exact steps in my proposal aren't set in stone. My logic was that we might want to do it in a single native-image-configure invocation so it sounded like a job for a single task.

I however believe that we should (somehow) utilize native-image-configure to filter out Gradle entries, and add a way for end user to append values into getFilterableEntries.

You seem to imply that native-image-configure can be used to filter entries. Is this documented somewhere?

$ native-image-configure --help

contains the following argument:

--exclude-classes=<class-pattern> adds a single rule to exclude classes matching the specified pattern, potentially overriding rules from preceding parameters. Can be specified several times.

I see. Let's start with what we have now and we can always change the implementation if needed, I'm not too worried.

lazar-mitrovic · 2022-02-07T15:28:20Z

...le-plugin/src/main/java/org/graalvm/buildtools/gradle/internal/AgentCommandLineProvider.java

@@ -82,7 +83,7 @@ public AgentCommandLineProvider() {
            if (agentOptions.stream().map(s -> s.split("=")[0]).anyMatch(s -> s.contains("config-output-dir"))) {
                throw new IllegalStateException("config-output-dir cannot be supplied as an agent option");


Looking at #189, in context of this PR, it seems that native-image-configure approach will break #190 (since config-merge-dir now has absolutely no meaning).

Not a blocker for merging this PR, but something to think about. We might want to add wrapper Gradle/Maven agent properties for following agentlib parameters:

trace-output

config-output-dir

config-merge-dir

The idea with #190 is now to have an enum which tells what we want to generate: trace output or config output. I'm not sure what config merge dir is used for. In any case, this should probably not be mixed into this PR.

From docs:

It can be necessary to run the target application more than once with different inputs to trigger separate execution paths for a better coverage of dynamic accesses. The agent supports this with the config-merge-dir option which adds the intercepted accesses to an existing set of configuration files:

$JAVA_HOME/bin/java -agentlib:native-image-agent=config-merge-dir=/path/to/config-dir/ ... ^^^^^

If the specified target directory or configuration files in it are missing when using config-merge-dir, the agent creates them and prints a warning.

Basically we would need to reproduce this behavior using native-image-configure invocation.

I agree that this should not be mixed with this PR, but it is something to keep in mind when designing tasks related to native-image-configure invocation.

I see, in combination with this race condition this starts to become bind blowing.

lazar-mitrovic · 2022-02-07T15:57:25Z

Other than those two comments, this LGTM. Thank you for fixing this so quickly!

This commit makes use of the `native-image-configure` tool to merge the results of agent files. Before this commit, several process could be writing to the same output directory. Now, each process will use a separate directory and the tool is used to merge their outputs. Because the `native-image-configure` tool might not be present, it's also going to invoke the command which generates it. See #194

Fixes #194

This is a workaround for goal execution ordering problems with Maven. For tests, we can add a synthetic goal which is executed in the same phase and will merge the agent files generated by the test execution. Unfortunately, this approach doesn't work if we want to instrument the main execution, which is sometimes the case. This is not doable because we won't have a phase to hook into and a goal to execute in order to merge the files. Even if we synthetize a goal and try to add it to the session execution, this would cause ordering issues because we don't know in which order the plugins are defined in the build file. In addition, this would simply not work for multi-module builds. As a workaround, we simply disable the split agent output directories in case we instrument the main execution. This will hopefully work for most invocations.

melix added bug Something isn't working gradle-plugin Related to Gradle plugin labels Feb 1, 2022

melix added this to the 0.9.10 milestone Feb 1, 2022

melix requested a review from lazar-mitrovic February 1, 2022 16:00

melix self-assigned this Feb 1, 2022

graalvmbot added the oracle-emp label Feb 1, 2022

melix force-pushed the cc/issue-194 branch 2 times, most recently from 721952f to dc00048 Compare February 3, 2022 08:55

melix force-pushed the cc/issue-194 branch from 5bf8da8 to e2ffa52 Compare February 4, 2022 12:20

lazar-mitrovic added the maven-plugin Related to Maven plugin label Feb 7, 2022

lazar-mitrovic reviewed Feb 7, 2022

View reviewed changes

melix added 4 commits February 9, 2022 09:46

Add support for merging agent files in Maven

a8f6f1c

Fixes #194

Add documentation about bugfix

bd9d033

melix force-pushed the cc/issue-194 branch from e2ffa52 to bd9d033 Compare February 9, 2022 08:47

melix merged commit 2e396d7 into master Feb 9, 2022

melix deleted the cc/issue-194 branch February 9, 2022 09:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix race condition if tests are executed in parallel #195

Fix race condition if tests are executed in parallel #195

melix commented Feb 1, 2022

melix commented Feb 3, 2022

lazar-mitrovic Feb 7, 2022

melix Feb 8, 2022

lazar-mitrovic Feb 8, 2022

melix Feb 8, 2022

lazar-mitrovic Feb 8, 2022

melix Feb 9, 2022

lazar-mitrovic Feb 7, 2022

melix Feb 8, 2022

lazar-mitrovic Feb 8, 2022

melix Feb 9, 2022

lazar-mitrovic commented Feb 7, 2022

		@@ -82,7 +83,7 @@ public AgentCommandLineProvider() {
		if (agentOptions.stream().map(s -> s.split("=")[0]).anyMatch(s -> s.contains("config-output-dir"))) {
		throw new IllegalStateException("config-output-dir cannot be supplied as an agent option");

Fix race condition if tests are executed in parallel #195

Fix race condition if tests are executed in parallel #195

Conversation

melix commented Feb 1, 2022

melix commented Feb 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lazar-mitrovic commented Feb 7, 2022