gnodet-bot commented on code in PR #13397: URL: https://github.com/apache/maven/pull/13397#discussion_r4236573143
########## its/core-it-suite/src/test/resources/mng-7525-non-thread-safe-mojo/plugin/pom.xml: ########## @@ -0,0 +1,56 @@ +<?xml version="1.0" encoding="UTF-8"?> +<!-- +Licensed to the Apache Software Foundation (ASF) under one +or more contributor license agreements. See the NOTICE file +distributed with this work for additional information +regarding copyright ownership. The ASF licenses this file +to you under the Apache License, Version 2.0 (the +"License"); you may not use this file except in compliance +with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, +software distributed under the License is distributed on an +"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +KIND, either express or implied. See the License for the +specific language governing permissions and limitations +under the License. +--> +<project xmlns="http://maven.apache.org/POM/4.0.0"; xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"; xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 https://maven.apache.org/xsd/maven-4.0.0.xsd";> + <modelVersion>4.0.0</modelVersion> + + <parent> + <groupId>org.apache.maven.its.mng7525</groupId> + <artifactId>mng-7525-parent</artifactId> + <version>1.0-SNAPSHOT</version> + </parent> + + <artifactId>mng-7525-plugin</artifactId> + <packaging>maven-plugin</packaging> + + <dependencies> + <dependency> + <groupId>org.apache.maven</groupId> + <artifactId>maven-plugin-api</artifactId> + <version>3.9.12</version> + </dependency> + <dependency> + <groupId>org.apache.maven.plugin-tools</groupId> + <artifactId>maven-plugin-annotations</artifactId> + <version>3.15.2</version> + <scope>provided</scope> + </dependency> + </dependencies> + + <build> + <plugins> + <plugin> + <artifactId>maven-plugin-plugin</artifactId> + <configuration> Review Comment: 💡 **`maven-plugin-plugin` declared without explicit `<version>`** This relies on version inheritance from the parent POM. If the IT plugin is built standalone or the parent changes, the behavior becomes unpredictable. Add an explicit `<version>` to make the IT self-contained and reproducible. ########## its/core-it-suite/src/test/java/org/apache/maven/it/MavenITmng7525NonThreadSafeMojoTest.java: ########## @@ -0,0 +1,41 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.maven.it; + +import java.nio.file.Path; + +import org.junit.jupiter.api.Test; + +/** + * Verifies that parallel reactor builds serialize executions of a mojo that is not marked as thread-safe. + */ +class MavenITmng7525NonThreadSafeMojoTest extends AbstractMavenIntegrationTestCase { + + @Test + void serializesNonThreadSafeMojoExecutions() throws Exception { + Path projectDir = extractResources("mng-7525-non-thread-safe-mojo"); + + Verifier verifier = newVerifier(projectDir); + verifier.execute("install", "-pl", "plugin"); + verifier.verifyErrorFreeLog(); + + verifier.execute("-T2", "validate"); + verifier.verifyErrorFreeLog(); Review Comment: ⚠️ **Verifier reuse may accumulate arguments** The same `Verifier` instance is reused for both `execute()` calls. If `Verifier.execute(String... args)` appends to an internal args list without clearing between calls, the second invocation runs Maven with accumulated arguments from the first (i.e. `install -pl plugin -T2 validate`), making the test semantically different from what is intended. Create a new `Verifier` for the second invocation: ```suggestion verifier = newVerifier(projectDir); verifier.execute("-T2", "validate"); ``` ########## impl/maven-core/src/main/java/org/apache/maven/plugin/DefaultBuildPluginManager.java: ########## @@ -205,6 +206,25 @@ public void executeMojo(MavenSession session, MojoExecution mojoExecution) } } + private void executeMojo( + MojoDescriptor mojoDescriptor, Mojo mojo, Object mojoLock, MojoExecutionEvent mojoExecutionEvent) + throws MojoExecutionException, MojoFailureException { + if (mojoDescriptor.isThreadSafe()) { + executeMojo(mojo, mojoExecutionEvent); + } else { + synchronized (mojoLock) { + executeMojo(mojo, mojoExecutionEvent); + } + } + } + + private void executeMojo(Mojo mojo, MojoExecutionEvent mojoExecutionEvent) Review Comment: ⚠️ **Lock scope includes listener callbacks — narrows serialization but widens contention window** Both `beforeMojoExecution()` and `afterMojoExecutionSuccess()` execute inside the `synchronized(mojoLock)` block (inherited from the calling overload). If any `MojoExecutionListener` implementation acquires its own lock, this creates lock-ordering risk. Consider moving only `mojo.execute()` inside the synchronized block: ```java private void executeMojo( MojoDescriptor mojoDescriptor, Mojo mojo, Object mojoLock, MojoExecutionEvent mojoExecutionEvent) throws MojoExecutionException, MojoFailureException { mojoExecutionListener.beforeMojoExecution(mojoExecutionEvent); if (mojoDescriptor.isThreadSafe()) { mojo.execute(); } else { synchronized (mojoLock) { mojo.execute(); } } mojoExecutionListener.afterMojoExecutionSuccess(mojoExecutionEvent); } ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
