You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by GitBox <gi...@apache.org> on 2021/06/02 06:40:27 UTC
[GitHub] [maven] gnodet opened a new pull request #476: [MNG-7156] Parallel build can cause issues between clean and forked g…
gnodet opened a new pull request #476:
URL: https://github.com/apache/maven/pull/476
…oals
Following this checklist to help us incorporate your
contribution quickly and easily:
- [ ] Make sure there is a [JIRA issue](https://issues.apache.org/jira/browse/MNG) filed
for the change (usually before you start working on it). Trivial changes like typos do not
require a JIRA issue. Your pull request should address just this issue, without
pulling in other changes.
- [ ] Each commit in the pull request should have a meaningful subject line and body.
- [ ] Format the pull request title like `[MNG-XXX] - Fixes bug in ApproximateQuantiles`,
where you replace `MNG-XXX` with the appropriate JIRA issue. Best practice
is to use the JIRA issue title in the pull request title and in the first line of the
commit message.
- [ ] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
- [ ] Run `mvn clean verify` to make sure basic checks pass. A more thorough check will
be performed on your pull request automatically.
- [ ] You have run the [Core IT][core-its] successfully.
If your pull request is about ~20 lines of code you don't need to sign an
[Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf) if you are unsure
please ask on the developers list.
To make clear that you license your contribution under
the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
you have to acknowledge this by using the following check-box.
- [ ] I hereby declare this contribution to be licenced under the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
- [ ] In any other case, please file an [Apache Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
[core-its]: https://maven.apache.org/core-its/core-it-suite/
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] gnodet commented on a change in pull request #476: [MNG-7156] Parallel build can cause issues between clean and forked g…
Posted by GitBox <gi...@apache.org>.
gnodet commented on a change in pull request #476:
URL: https://github.com/apache/maven/pull/476#discussion_r646178535
##########
File path: maven-core/src/main/java/org/apache/maven/execution/LockingEventSpy.java
##########
@@ -0,0 +1,96 @@
+package org.apache.maven.execution;
+
+/*
+ * 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.
+ */
+
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
+
+import javax.inject.Named;
+import javax.inject.Singleton;
+
+import org.apache.maven.eventspy.AbstractEventSpy;
+import org.apache.maven.eventspy.EventSpy;
+import org.apache.maven.project.MavenProject;
+import org.eclipse.aether.SessionData;
+import org.eclipse.sisu.Typed;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * EventSpy implementation that provides a per-project locking mechanism
+ * to make sure a given project can not be build twice concurrently.
+ * This case can happen when running parallel builds with forked lifecycles.
+ */
+@Singleton
+@Named
+@Typed( EventSpy.class )
+public class LockingEventSpy extends AbstractEventSpy
+{
+
+ private final Logger logger = LoggerFactory.getLogger( getClass() );
+
+ private static final Object LOCKS_KEY = new Object();
+
+ @SuppressWarnings( { "unchecked", "rawtypes" } )
+ private Lock getLock( ExecutionEvent event )
+ {
+ SessionData data = event.getSession().getRepositorySession().getData();
+ Map<MavenProject, Lock> locks = ( Map ) data.get( LOCKS_KEY );
+ if ( locks == null )
+ {
+ data.set( LOCKS_KEY, null, new ConcurrentHashMap<>() );
+ locks = ( Map ) data.get( LOCKS_KEY );
+ }
+ return locks.computeIfAbsent( event.getProject(), p -> new ReentrantLock() );
+ }
+
+ @Override
+ public void onEvent( Object event ) throws Exception
+ {
+ if ( event instanceof ExecutionEvent )
+ {
+ ExecutionEvent ee = ( ExecutionEvent ) event;
+ switch ( ee.getType() )
+ {
+ case ProjectStarted:
+ case ForkedProjectStarted:
+ Lock lock = getLock( ee );
+ if ( !lock.tryLock() )
+ {
+ logger.warn( "Suspending concurrent execution of project " + ee.getProject() );
+ getLock( ee ).lockInterruptibly();
+ logger.warn( "Resuming execution of project " + ee.getProject() );
+ }
+ break;
+ case ProjectSucceeded:
+ case ProjectFailed:
Review comment:
My understanding is the `ProjectSkipped` is send after a failure and in such a case, the project is... skipped, so there's no need to lock / unlock, especially as the `ProjectStarted` event is not sent in such cases.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] famod commented on pull request #476: [MNG-7156] Parallel build can cause issues between clean and forked g…
Posted by GitBox <gi...@apache.org>.
famod commented on pull request #476:
URL: https://github.com/apache/maven/pull/476#issuecomment-950134707
Two things:
- I think this more "urgent" (for 3.8.x) given the discussion in #578
- There could be a system property to opt out of this, just in case (for some incubation time)
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] cstamas commented on a change in pull request #476: [MNG-7156] Parallel build can cause issues between clean and forked g…
Posted by GitBox <gi...@apache.org>.
cstamas commented on a change in pull request #476:
URL: https://github.com/apache/maven/pull/476#discussion_r724151436
##########
File path: maven-core/src/main/java/org/apache/maven/execution/LockingEventSpy.java
##########
@@ -0,0 +1,96 @@
+package org.apache.maven.execution;
+
+/*
+ * 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.
+ */
+
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
+
+import javax.inject.Named;
+import javax.inject.Singleton;
+
+import org.apache.maven.eventspy.AbstractEventSpy;
+import org.apache.maven.eventspy.EventSpy;
+import org.apache.maven.project.MavenProject;
+import org.eclipse.aether.SessionData;
+import org.eclipse.sisu.Typed;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * EventSpy implementation that provides a per-project locking mechanism
+ * to make sure a given project can not be build twice concurrently.
+ * This case can happen when running parallel builds with forked lifecycles.
+ */
+@Singleton
+@Named
+@Typed( EventSpy.class )
+public class LockingEventSpy extends AbstractEventSpy
Review comment:
I think EventSpy for now will do, while Martin is right, seems "awkward". I am rather to change MavenCli and support discovery (should be trivial).... but for start, this PR is a "go-go-go" for me, and we can latest refactor this into listener...
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] Tibor17 commented on a change in pull request #476: [MNG-7156] Parallel build can cause issues between clean and forked g…
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #476:
URL: https://github.com/apache/maven/pull/476#discussion_r646182597
##########
File path: maven-core/src/main/java/org/apache/maven/execution/LockingEventSpy.java
##########
@@ -0,0 +1,96 @@
+package org.apache.maven.execution;
+
+/*
+ * 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.
+ */
+
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
+
+import javax.inject.Named;
+import javax.inject.Singleton;
+
+import org.apache.maven.eventspy.AbstractEventSpy;
+import org.apache.maven.eventspy.EventSpy;
+import org.apache.maven.project.MavenProject;
+import org.eclipse.aether.SessionData;
+import org.eclipse.sisu.Typed;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * EventSpy implementation that provides a per-project locking mechanism
+ * to make sure a given project can not be build twice concurrently.
+ * This case can happen when running parallel builds with forked lifecycles.
+ */
+@Singleton
+@Named
+@Typed( EventSpy.class )
+public class LockingEventSpy extends AbstractEventSpy
+{
+
+ private final Logger logger = LoggerFactory.getLogger( getClass() );
+
+ private static final Object LOCKS_KEY = new Object();
+
+ @SuppressWarnings( { "unchecked", "rawtypes" } )
+ private Lock getLock( ExecutionEvent event )
+ {
+ SessionData data = event.getSession().getRepositorySession().getData();
+ Map<MavenProject, Lock> locks = ( Map ) data.get( LOCKS_KEY );
+ if ( locks == null )
+ {
+ data.set( LOCKS_KEY, null, new ConcurrentHashMap<>() );
Review comment:
What mean `null` here?
Three parameters?
This line can be called concurrently. It should be treated somehow, like the semantics `trySet(): boolean`
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] Tibor17 commented on a change in pull request #476: [MNG-7156] Parallel build can cause issues between clean and forked g…
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #476:
URL: https://github.com/apache/maven/pull/476#discussion_r646182597
##########
File path: maven-core/src/main/java/org/apache/maven/execution/LockingEventSpy.java
##########
@@ -0,0 +1,96 @@
+package org.apache.maven.execution;
+
+/*
+ * 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.
+ */
+
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
+
+import javax.inject.Named;
+import javax.inject.Singleton;
+
+import org.apache.maven.eventspy.AbstractEventSpy;
+import org.apache.maven.eventspy.EventSpy;
+import org.apache.maven.project.MavenProject;
+import org.eclipse.aether.SessionData;
+import org.eclipse.sisu.Typed;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * EventSpy implementation that provides a per-project locking mechanism
+ * to make sure a given project can not be build twice concurrently.
+ * This case can happen when running parallel builds with forked lifecycles.
+ */
+@Singleton
+@Named
+@Typed( EventSpy.class )
+public class LockingEventSpy extends AbstractEventSpy
+{
+
+ private final Logger logger = LoggerFactory.getLogger( getClass() );
+
+ private static final Object LOCKS_KEY = new Object();
+
+ @SuppressWarnings( { "unchecked", "rawtypes" } )
+ private Lock getLock( ExecutionEvent event )
+ {
+ SessionData data = event.getSession().getRepositorySession().getData();
+ Map<MavenProject, Lock> locks = ( Map ) data.get( LOCKS_KEY );
+ if ( locks == null )
+ {
+ data.set( LOCKS_KEY, null, new ConcurrentHashMap<>() );
Review comment:
What does the `null` mean here?
Three parameters?
This line can be called concurrently. It should be treated somehow. Maybe use the semantics `trySet(): boolean` within critical section implemented in the method.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] Tibor17 commented on a change in pull request #476: [MNG-7156] Parallel build can cause issues between clean and forked g…
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #476:
URL: https://github.com/apache/maven/pull/476#discussion_r646963638
##########
File path: maven-core/src/main/java/org/apache/maven/execution/LockingEventSpy.java
##########
@@ -0,0 +1,96 @@
+package org.apache.maven.execution;
+
+/*
+ * 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.
+ */
+
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
+
+import javax.inject.Named;
+import javax.inject.Singleton;
+
+import org.apache.maven.eventspy.AbstractEventSpy;
+import org.apache.maven.eventspy.EventSpy;
+import org.apache.maven.project.MavenProject;
+import org.eclipse.aether.SessionData;
+import org.eclipse.sisu.Typed;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * EventSpy implementation that provides a per-project locking mechanism
+ * to make sure a given project can not be build twice concurrently.
+ * This case can happen when running parallel builds with forked lifecycles.
+ */
+@Singleton
+@Named
+@Typed( EventSpy.class )
+public class LockingEventSpy extends AbstractEventSpy
+{
+
+ private final Logger logger = LoggerFactory.getLogger( getClass() );
+
+ private static final Object LOCKS_KEY = new Object();
+
+ @SuppressWarnings( { "unchecked", "rawtypes" } )
+ private Lock getLock( ExecutionEvent event )
+ {
+ SessionData data = event.getSession().getRepositorySession().getData();
+ Map<MavenProject, Lock> locks = ( Map ) data.get( LOCKS_KEY );
+ if ( locks == null )
+ {
+ data.set( LOCKS_KEY, null, new ConcurrentHashMap<>() );
Review comment:
ok, if the session is implemented using `putIfAbsent` then it should be fine.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] gnodet commented on pull request #476: [MNG-7156] Parallel build can cause issues between clean and forked g…
Posted by GitBox <gi...@apache.org>.
gnodet commented on pull request #476:
URL: https://github.com/apache/maven/pull/476#issuecomment-984805756
> So this revolves around the idea that all elements subject to a project cannot be modified while the lock is obtained, so everything else concurrently is halted?
> So as far as I understand this PR is global while PR #578 is rather local?
I had a closer look at both problems and proposed solutions. They are indeed related but for two different use cases. The This one #476 address the use case for synchronising between a standard lifecycle and a forked lifecycle, while #578 synchronises between a mojo execution for a given project and a mojo execution that has the _reactor_ flag set.
Both are related because they both address concurrency execution between projects.
I'll work on a PR that addresses both problem at the same time.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] gnodet commented on a change in pull request #476: [MNG-7156] Parallel build can cause issues between clean and forked g…
Posted by GitBox <gi...@apache.org>.
gnodet commented on a change in pull request #476:
URL: https://github.com/apache/maven/pull/476#discussion_r646179884
##########
File path: maven-core/src/main/java/org/apache/maven/execution/LockingEventSpy.java
##########
@@ -0,0 +1,96 @@
+package org.apache.maven.execution;
+
+/*
+ * 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.
+ */
+
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
+
+import javax.inject.Named;
+import javax.inject.Singleton;
+
+import org.apache.maven.eventspy.AbstractEventSpy;
+import org.apache.maven.eventspy.EventSpy;
+import org.apache.maven.project.MavenProject;
+import org.eclipse.aether.SessionData;
+import org.eclipse.sisu.Typed;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * EventSpy implementation that provides a per-project locking mechanism
+ * to make sure a given project can not be build twice concurrently.
+ * This case can happen when running parallel builds with forked lifecycles.
+ */
+@Singleton
+@Named
+@Typed( EventSpy.class )
+public class LockingEventSpy extends AbstractEventSpy
+{
+
+ private final Logger logger = LoggerFactory.getLogger( getClass() );
+
+ private static final Object LOCKS_KEY = new Object();
+
+ @SuppressWarnings( { "unchecked", "rawtypes" } )
+ private Lock getLock( ExecutionEvent event )
+ {
+ SessionData data = event.getSession().getRepositorySession().getData();
+ Map<MavenProject, Lock> locks = ( Map ) data.get( LOCKS_KEY );
+ if ( locks == null )
+ {
+ data.set( LOCKS_KEY, null, new ConcurrentHashMap<>() );
+ locks = ( Map ) data.get( LOCKS_KEY );
+ }
+ return locks.computeIfAbsent( event.getProject(), p -> new ReentrantLock() );
+ }
+
+ @Override
+ public void onEvent( Object event ) throws Exception
+ {
+ if ( event instanceof ExecutionEvent )
+ {
+ ExecutionEvent ee = ( ExecutionEvent ) event;
Review comment:
Sure, sounds good.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] gnodet commented on a change in pull request #476: [MNG-7156] Parallel build can cause issues between clean and forked g…
Posted by GitBox <gi...@apache.org>.
gnodet commented on a change in pull request #476:
URL: https://github.com/apache/maven/pull/476#discussion_r726782484
##########
File path: maven-core/src/main/java/org/apache/maven/execution/LockingEventSpy.java
##########
@@ -0,0 +1,98 @@
+package org.apache.maven.execution;
+
+/*
+ * 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.
+ */
+
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
+
+import javax.inject.Named;
+import javax.inject.Singleton;
+
+import org.apache.maven.eventspy.AbstractEventSpy;
+import org.apache.maven.eventspy.EventSpy;
+import org.apache.maven.project.MavenProject;
+import org.eclipse.aether.SessionData;
+import org.eclipse.sisu.Typed;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * EventSpy implementation that provides a per-project locking mechanism
+ * to make sure a given project can not be built concurrently.
+ * This case can happen when running parallel builds with forked lifecycles.
+ */
+@Singleton
+@Named
+@Typed( EventSpy.class )
+public class LockingEventSpy extends AbstractEventSpy
+{
+
+ private static final Object LOCKS_KEY = new Object();
+
+ private final Logger logger = LoggerFactory.getLogger( getClass() );
+
+ @Override
+ public void onEvent( Object event ) throws Exception
+ {
+ if ( event instanceof ExecutionEvent )
+ {
+ ExecutionEvent executionEvent = ( ExecutionEvent ) event;
+ switch ( executionEvent.getType() )
+ {
+ case ProjectStarted:
+ case ForkedProjectStarted:
+ Lock lock = getLock( executionEvent );
+ if ( !lock.tryLock() )
+ {
+ logger.warn( "Suspending concurrent execution of project '{}'", executionEvent.getProject() );
Review comment:
Yes, I think the log statements can be toned down to _debug_. I'd rather keep the two steps locking for easier debugging, the penalty should be quite low as this is an edge case and most of the time, the lock can be obtained from the `tryLock` call.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] Tibor17 commented on a change in pull request #476: [MNG-7156] Parallel build can cause issues between clean and forked g…
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #476:
URL: https://github.com/apache/maven/pull/476#discussion_r646182597
##########
File path: maven-core/src/main/java/org/apache/maven/execution/LockingEventSpy.java
##########
@@ -0,0 +1,96 @@
+package org.apache.maven.execution;
+
+/*
+ * 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.
+ */
+
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
+
+import javax.inject.Named;
+import javax.inject.Singleton;
+
+import org.apache.maven.eventspy.AbstractEventSpy;
+import org.apache.maven.eventspy.EventSpy;
+import org.apache.maven.project.MavenProject;
+import org.eclipse.aether.SessionData;
+import org.eclipse.sisu.Typed;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * EventSpy implementation that provides a per-project locking mechanism
+ * to make sure a given project can not be build twice concurrently.
+ * This case can happen when running parallel builds with forked lifecycles.
+ */
+@Singleton
+@Named
+@Typed( EventSpy.class )
+public class LockingEventSpy extends AbstractEventSpy
+{
+
+ private final Logger logger = LoggerFactory.getLogger( getClass() );
+
+ private static final Object LOCKS_KEY = new Object();
+
+ @SuppressWarnings( { "unchecked", "rawtypes" } )
+ private Lock getLock( ExecutionEvent event )
+ {
+ SessionData data = event.getSession().getRepositorySession().getData();
+ Map<MavenProject, Lock> locks = ( Map ) data.get( LOCKS_KEY );
+ if ( locks == null )
+ {
+ data.set( LOCKS_KEY, null, new ConcurrentHashMap<>() );
Review comment:
What does the `null` mean here?
Three parameters?
This line can be called concurrently. It should be treated somehow, like the semantics `trySet(): boolean`
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] Tibor17 commented on a change in pull request #476: [MNG-7156] Parallel build can cause issues between clean and forked g…
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #476:
URL: https://github.com/apache/maven/pull/476#discussion_r646182597
##########
File path: maven-core/src/main/java/org/apache/maven/execution/LockingEventSpy.java
##########
@@ -0,0 +1,96 @@
+package org.apache.maven.execution;
+
+/*
+ * 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.
+ */
+
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
+
+import javax.inject.Named;
+import javax.inject.Singleton;
+
+import org.apache.maven.eventspy.AbstractEventSpy;
+import org.apache.maven.eventspy.EventSpy;
+import org.apache.maven.project.MavenProject;
+import org.eclipse.aether.SessionData;
+import org.eclipse.sisu.Typed;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * EventSpy implementation that provides a per-project locking mechanism
+ * to make sure a given project can not be build twice concurrently.
+ * This case can happen when running parallel builds with forked lifecycles.
+ */
+@Singleton
+@Named
+@Typed( EventSpy.class )
+public class LockingEventSpy extends AbstractEventSpy
+{
+
+ private final Logger logger = LoggerFactory.getLogger( getClass() );
+
+ private static final Object LOCKS_KEY = new Object();
+
+ @SuppressWarnings( { "unchecked", "rawtypes" } )
+ private Lock getLock( ExecutionEvent event )
+ {
+ SessionData data = event.getSession().getRepositorySession().getData();
+ Map<MavenProject, Lock> locks = ( Map ) data.get( LOCKS_KEY );
+ if ( locks == null )
+ {
+ data.set( LOCKS_KEY, null, new ConcurrentHashMap<>() );
Review comment:
What does the `null` mean here?
Three parameters?
This line can be called concurrently. It should be treated somehow. Maybe use the semantics `trySet(): boolean` with a critical section implemented in the method.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] MartinKanters commented on a change in pull request #476: [MNG-7156] Parallel build can cause issues between clean and forked g…
Posted by GitBox <gi...@apache.org>.
MartinKanters commented on a change in pull request #476:
URL: https://github.com/apache/maven/pull/476#discussion_r646850578
##########
File path: maven-core/src/main/java/org/apache/maven/execution/LockingEventSpy.java
##########
@@ -0,0 +1,96 @@
+package org.apache.maven.execution;
+
+/*
+ * 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.
+ */
+
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
+
+import javax.inject.Named;
+import javax.inject.Singleton;
+
+import org.apache.maven.eventspy.AbstractEventSpy;
+import org.apache.maven.eventspy.EventSpy;
+import org.apache.maven.project.MavenProject;
+import org.eclipse.aether.SessionData;
+import org.eclipse.sisu.Typed;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * EventSpy implementation that provides a per-project locking mechanism
+ * to make sure a given project can not be build twice concurrently.
+ * This case can happen when running parallel builds with forked lifecycles.
+ */
+@Singleton
+@Named
+@Typed( EventSpy.class )
+public class LockingEventSpy extends AbstractEventSpy
Review comment:
It's definitely less intrusive, but I'm not sure whether something that controls the build as much as this (temporarily pausing and resuming project builds) should be done in an `ExecutionListener` or `EventSpy`. My gut feeling says those classes should only extend the build with simple side-effects, like logging a project's phase. I would prefer your last suggestion. Then again, maybe this is the right direction. Perhaps some others can give their opinion on it?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] gnodet closed pull request #476: [MNG-7156] Parallel build can cause issues between clean and forked g…
Posted by GitBox <gi...@apache.org>.
gnodet closed pull request #476:
URL: https://github.com/apache/maven/pull/476
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] gnodet commented on a change in pull request #476: [MNG-7156] Parallel build can cause issues between clean and forked g…
Posted by GitBox <gi...@apache.org>.
gnodet commented on a change in pull request #476:
URL: https://github.com/apache/maven/pull/476#discussion_r646179864
##########
File path: maven-core/src/main/java/org/apache/maven/execution/LockingEventSpy.java
##########
@@ -0,0 +1,96 @@
+package org.apache.maven.execution;
+
+/*
+ * 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.
+ */
+
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
+
+import javax.inject.Named;
+import javax.inject.Singleton;
+
+import org.apache.maven.eventspy.AbstractEventSpy;
+import org.apache.maven.eventspy.EventSpy;
+import org.apache.maven.project.MavenProject;
+import org.eclipse.aether.SessionData;
+import org.eclipse.sisu.Typed;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * EventSpy implementation that provides a per-project locking mechanism
+ * to make sure a given project can not be build twice concurrently.
+ * This case can happen when running parallel builds with forked lifecycles.
+ */
+@Singleton
+@Named
+@Typed( EventSpy.class )
+public class LockingEventSpy extends AbstractEventSpy
Review comment:
Another possibility would be to change the `MavenCli` to support multiple `ExecutionListener`. The problem (minor) is that there's no discovery mechanism, so they are hard coded, which I wanted to avoid... Another possibility would be to define a component to hold the locks and inject it into the `LifeycleModuleBuilder` and `MojoExecutor` where those events are fired initially. I don't really mind, but the current solution was the less intrusive.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] famod commented on a change in pull request #476: [MNG-7156] Parallel build can cause issues between clean and forked g…
Posted by GitBox <gi...@apache.org>.
famod commented on a change in pull request #476:
URL: https://github.com/apache/maven/pull/476#discussion_r726609785
##########
File path: maven-core/src/main/java/org/apache/maven/execution/LockingEventSpy.java
##########
@@ -0,0 +1,98 @@
+package org.apache.maven.execution;
+
+/*
+ * 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.
+ */
+
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
+
+import javax.inject.Named;
+import javax.inject.Singleton;
+
+import org.apache.maven.eventspy.AbstractEventSpy;
+import org.apache.maven.eventspy.EventSpy;
+import org.apache.maven.project.MavenProject;
+import org.eclipse.aether.SessionData;
+import org.eclipse.sisu.Typed;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * EventSpy implementation that provides a per-project locking mechanism
+ * to make sure a given project can not be built concurrently.
+ * This case can happen when running parallel builds with forked lifecycles.
+ */
+@Singleton
+@Named
+@Typed( EventSpy.class )
+public class LockingEventSpy extends AbstractEventSpy
+{
+
+ private static final Object LOCKS_KEY = new Object();
+
+ private final Logger logger = LoggerFactory.getLogger( getClass() );
+
+ @Override
+ public void onEvent( Object event ) throws Exception
+ {
+ if ( event instanceof ExecutionEvent )
+ {
+ ExecutionEvent executionEvent = ( ExecutionEvent ) event;
+ switch ( executionEvent.getType() )
+ {
+ case ProjectStarted:
+ case ForkedProjectStarted:
+ Lock lock = getLock( executionEvent );
+ if ( !lock.tryLock() )
+ {
+ logger.warn( "Suspending concurrent execution of project '{}'", executionEvent.getProject() );
Review comment:
Is it really worth a warning? Looking at if from the perspective of aggregating mojo executions (#578) this is actually an expected case (or let's at least say "not uncommon").
I think `lock.lockInterruptibly();` should be called right away?
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] cstamas commented on a change in pull request #476: [MNG-7156] Parallel build can cause issues between clean and forked g…
Posted by GitBox <gi...@apache.org>.
cstamas commented on a change in pull request #476:
URL: https://github.com/apache/maven/pull/476#discussion_r724151436
##########
File path: maven-core/src/main/java/org/apache/maven/execution/LockingEventSpy.java
##########
@@ -0,0 +1,96 @@
+package org.apache.maven.execution;
+
+/*
+ * 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.
+ */
+
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
+
+import javax.inject.Named;
+import javax.inject.Singleton;
+
+import org.apache.maven.eventspy.AbstractEventSpy;
+import org.apache.maven.eventspy.EventSpy;
+import org.apache.maven.project.MavenProject;
+import org.eclipse.aether.SessionData;
+import org.eclipse.sisu.Typed;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * EventSpy implementation that provides a per-project locking mechanism
+ * to make sure a given project can not be build twice concurrently.
+ * This case can happen when running parallel builds with forked lifecycles.
+ */
+@Singleton
+@Named
+@Typed( EventSpy.class )
+public class LockingEventSpy extends AbstractEventSpy
Review comment:
I think EventSpy for now will do, while Martin is right, seems "awkward". I am rather to change MavenCli and support discovery (should be trivial).... but for start, this PR is a "go-go-go" for me, and we can latest refactor this into listener...
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] gnodet commented on pull request #476: [MNG-7156] Parallel build can cause issues between clean and forked g…
Posted by GitBox <gi...@apache.org>.
gnodet commented on pull request #476:
URL: https://github.com/apache/maven/pull/476#issuecomment-937813747
> Cool, I think I saw some of issues while using mvnd that should be solved...
Fwiw, it's not really urgent, as it has already been integrated into `mvnd` ...
https://github.com/mvndaemon/mvnd/blob/master/daemon/src/main/java/org/mvndaemon/mvnd/execution/LockingEventSpy.java
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] gnodet commented on pull request #476: [MNG-7156] Parallel build can cause issues between clean and forked g…
Posted by GitBox <gi...@apache.org>.
gnodet commented on pull request #476:
URL: https://github.com/apache/maven/pull/476#issuecomment-937813747
> Cool, I think I saw some of issues while using mvnd that should be solved...
Fwiw, it's not really urgent, as it has already been integrated into `mvnd` ...
https://github.com/mvndaemon/mvnd/blob/master/daemon/src/main/java/org/mvndaemon/mvnd/execution/LockingEventSpy.java
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] famod edited a comment on pull request #476: [MNG-7156] Parallel build can cause issues between clean and forked g…
Posted by GitBox <gi...@apache.org>.
famod edited a comment on pull request #476:
URL: https://github.com/apache/maven/pull/476#issuecomment-950134707
Two things:
- I think this is now more "urgent" (for 3.8.x) given the discussion in #578
- There could be a system property to opt out of this, just in case (for some incubation time)
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] MartinKanters commented on a change in pull request #476: [MNG-7156] Parallel build can cause issues between clean and forked g…
Posted by GitBox <gi...@apache.org>.
MartinKanters commented on a change in pull request #476:
URL: https://github.com/apache/maven/pull/476#discussion_r646170192
##########
File path: maven-core/src/main/java/org/apache/maven/execution/LockingEventSpy.java
##########
@@ -0,0 +1,96 @@
+package org.apache.maven.execution;
+
+/*
+ * 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.
+ */
+
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
+
+import javax.inject.Named;
+import javax.inject.Singleton;
+
+import org.apache.maven.eventspy.AbstractEventSpy;
+import org.apache.maven.eventspy.EventSpy;
+import org.apache.maven.project.MavenProject;
+import org.eclipse.aether.SessionData;
+import org.eclipse.sisu.Typed;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * EventSpy implementation that provides a per-project locking mechanism
+ * to make sure a given project can not be build twice concurrently.
Review comment:
```suggestion
* to make sure a given project can not be built concurrently.
```
##########
File path: maven-core/src/main/java/org/apache/maven/execution/LockingEventSpy.java
##########
@@ -0,0 +1,96 @@
+package org.apache.maven.execution;
+
+/*
+ * 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.
+ */
+
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
+
+import javax.inject.Named;
+import javax.inject.Singleton;
+
+import org.apache.maven.eventspy.AbstractEventSpy;
+import org.apache.maven.eventspy.EventSpy;
+import org.apache.maven.project.MavenProject;
+import org.eclipse.aether.SessionData;
+import org.eclipse.sisu.Typed;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * EventSpy implementation that provides a per-project locking mechanism
+ * to make sure a given project can not be build twice concurrently.
+ * This case can happen when running parallel builds with forked lifecycles.
+ */
+@Singleton
+@Named
+@Typed( EventSpy.class )
+public class LockingEventSpy extends AbstractEventSpy
+{
+
+ private final Logger logger = LoggerFactory.getLogger( getClass() );
+
+ private static final Object LOCKS_KEY = new Object();
+
+ @SuppressWarnings( { "unchecked", "rawtypes" } )
+ private Lock getLock( ExecutionEvent event )
+ {
+ SessionData data = event.getSession().getRepositorySession().getData();
+ Map<MavenProject, Lock> locks = ( Map ) data.get( LOCKS_KEY );
+ if ( locks == null )
+ {
+ data.set( LOCKS_KEY, null, new ConcurrentHashMap<>() );
+ locks = ( Map ) data.get( LOCKS_KEY );
+ }
+ return locks.computeIfAbsent( event.getProject(), p -> new ReentrantLock() );
+ }
+
+ @Override
+ public void onEvent( Object event ) throws Exception
+ {
+ if ( event instanceof ExecutionEvent )
+ {
+ ExecutionEvent ee = ( ExecutionEvent ) event;
Review comment:
Can we come up with a more descriptive name than this? Perhaps just `executionEvent`?
##########
File path: maven-core/src/main/java/org/apache/maven/execution/LockingEventSpy.java
##########
@@ -0,0 +1,96 @@
+package org.apache.maven.execution;
+
+/*
+ * 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.
+ */
+
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
+
+import javax.inject.Named;
+import javax.inject.Singleton;
+
+import org.apache.maven.eventspy.AbstractEventSpy;
+import org.apache.maven.eventspy.EventSpy;
+import org.apache.maven.project.MavenProject;
+import org.eclipse.aether.SessionData;
+import org.eclipse.sisu.Typed;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * EventSpy implementation that provides a per-project locking mechanism
+ * to make sure a given project can not be build twice concurrently.
+ * This case can happen when running parallel builds with forked lifecycles.
+ */
+@Singleton
+@Named
+@Typed( EventSpy.class )
+public class LockingEventSpy extends AbstractEventSpy
Review comment:
I think it's quite an elegant solution, but I'm not sure if using an `EventSpy` for this purpose is how it should be done in `maven-core`. I don't think there are other implementations of it in core.
##########
File path: maven-core/src/main/java/org/apache/maven/execution/LockingEventSpy.java
##########
@@ -0,0 +1,96 @@
+package org.apache.maven.execution;
+
+/*
+ * 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.
+ */
+
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
+
+import javax.inject.Named;
+import javax.inject.Singleton;
+
+import org.apache.maven.eventspy.AbstractEventSpy;
+import org.apache.maven.eventspy.EventSpy;
+import org.apache.maven.project.MavenProject;
+import org.eclipse.aether.SessionData;
+import org.eclipse.sisu.Typed;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * EventSpy implementation that provides a per-project locking mechanism
+ * to make sure a given project can not be build twice concurrently.
+ * This case can happen when running parallel builds with forked lifecycles.
+ */
+@Singleton
+@Named
+@Typed( EventSpy.class )
+public class LockingEventSpy extends AbstractEventSpy
+{
+
+ private final Logger logger = LoggerFactory.getLogger( getClass() );
+
+ private static final Object LOCKS_KEY = new Object();
+
+ @SuppressWarnings( { "unchecked", "rawtypes" } )
+ private Lock getLock( ExecutionEvent event )
+ {
+ SessionData data = event.getSession().getRepositorySession().getData();
+ Map<MavenProject, Lock> locks = ( Map ) data.get( LOCKS_KEY );
+ if ( locks == null )
+ {
+ data.set( LOCKS_KEY, null, new ConcurrentHashMap<>() );
+ locks = ( Map ) data.get( LOCKS_KEY );
+ }
+ return locks.computeIfAbsent( event.getProject(), p -> new ReentrantLock() );
+ }
+
+ @Override
+ public void onEvent( Object event ) throws Exception
+ {
+ if ( event instanceof ExecutionEvent )
+ {
+ ExecutionEvent ee = ( ExecutionEvent ) event;
+ switch ( ee.getType() )
+ {
+ case ProjectStarted:
+ case ForkedProjectStarted:
+ Lock lock = getLock( ee );
+ if ( !lock.tryLock() )
+ {
+ logger.warn( "Suspending concurrent execution of project " + ee.getProject() );
+ getLock( ee ).lockInterruptibly();
+ logger.warn( "Resuming execution of project " + ee.getProject() );
+ }
+ break;
+ case ProjectSucceeded:
+ case ProjectFailed:
Review comment:
I think we might be missing ProjectSkipped right now, did your leave this out on purpose?
##########
File path: maven-core/src/main/java/org/apache/maven/execution/LockingEventSpy.java
##########
@@ -0,0 +1,96 @@
+package org.apache.maven.execution;
+
+/*
+ * 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.
+ */
+
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
+
+import javax.inject.Named;
+import javax.inject.Singleton;
+
+import org.apache.maven.eventspy.AbstractEventSpy;
+import org.apache.maven.eventspy.EventSpy;
+import org.apache.maven.project.MavenProject;
+import org.eclipse.aether.SessionData;
+import org.eclipse.sisu.Typed;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * EventSpy implementation that provides a per-project locking mechanism
+ * to make sure a given project can not be build twice concurrently.
+ * This case can happen when running parallel builds with forked lifecycles.
+ */
+@Singleton
+@Named
+@Typed( EventSpy.class )
+public class LockingEventSpy extends AbstractEventSpy
+{
+
+ private final Logger logger = LoggerFactory.getLogger( getClass() );
+
+ private static final Object LOCKS_KEY = new Object();
+
+ @SuppressWarnings( { "unchecked", "rawtypes" } )
+ private Lock getLock( ExecutionEvent event )
+ {
+ SessionData data = event.getSession().getRepositorySession().getData();
+ Map<MavenProject, Lock> locks = ( Map ) data.get( LOCKS_KEY );
+ if ( locks == null )
+ {
+ data.set( LOCKS_KEY, null, new ConcurrentHashMap<>() );
+ locks = ( Map ) data.get( LOCKS_KEY );
+ }
+ return locks.computeIfAbsent( event.getProject(), p -> new ReentrantLock() );
+ }
+
+ @Override
+ public void onEvent( Object event ) throws Exception
+ {
+ if ( event instanceof ExecutionEvent )
+ {
+ ExecutionEvent ee = ( ExecutionEvent ) event;
+ switch ( ee.getType() )
+ {
+ case ProjectStarted:
+ case ForkedProjectStarted:
+ Lock lock = getLock( ee );
+ if ( !lock.tryLock() )
+ {
+ logger.warn( "Suspending concurrent execution of project " + ee.getProject() );
+ getLock( ee ).lockInterruptibly();
+ logger.warn( "Resuming execution of project " + ee.getProject() );
Review comment:
```suggestion
logger.warn( "Suspending concurrent execution of project '{}'", ee.getProject() );
getLock( ee ).lockInterruptibly();
logger.warn( "Resuming execution of project '{}'", ee.getProject() );
```
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] gnodet commented on pull request #476: [MNG-7156] Parallel build can cause issues between clean and forked g…
Posted by GitBox <gi...@apache.org>.
gnodet commented on pull request #476:
URL: https://github.com/apache/maven/pull/476#issuecomment-988519526
Superseded by #628 for 3.x and #627 for master.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] gnodet commented on pull request #476: [MNG-7156] Parallel build can cause issues between clean and forked g…
Posted by GitBox <gi...@apache.org>.
gnodet commented on pull request #476:
URL: https://github.com/apache/maven/pull/476#issuecomment-852804630
> Will this also resolve `-DdeployAtEnd=true`?
I don't understand. This only adds a locking mechanism to avoid conflicts caused by parallel execution between the normal lifecycle and forked lifecycles, which can cause the same project to be build twice concurrently. This has nothing to do with `deployAtEnd` afaik.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] gnodet commented on a change in pull request #476: [MNG-7156] Parallel build can cause issues between clean and forked g…
Posted by GitBox <gi...@apache.org>.
gnodet commented on a change in pull request #476:
URL: https://github.com/apache/maven/pull/476#discussion_r646312402
##########
File path: maven-core/src/main/java/org/apache/maven/execution/LockingEventSpy.java
##########
@@ -0,0 +1,96 @@
+package org.apache.maven.execution;
+
+/*
+ * 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.
+ */
+
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
+
+import javax.inject.Named;
+import javax.inject.Singleton;
+
+import org.apache.maven.eventspy.AbstractEventSpy;
+import org.apache.maven.eventspy.EventSpy;
+import org.apache.maven.project.MavenProject;
+import org.eclipse.aether.SessionData;
+import org.eclipse.sisu.Typed;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * EventSpy implementation that provides a per-project locking mechanism
+ * to make sure a given project can not be build twice concurrently.
+ * This case can happen when running parallel builds with forked lifecycles.
+ */
+@Singleton
+@Named
+@Typed( EventSpy.class )
+public class LockingEventSpy extends AbstractEventSpy
+{
+
+ private final Logger logger = LoggerFactory.getLogger( getClass() );
+
+ private static final Object LOCKS_KEY = new Object();
+
+ @SuppressWarnings( { "unchecked", "rawtypes" } )
+ private Lock getLock( ExecutionEvent event )
+ {
+ SessionData data = event.getSession().getRepositorySession().getData();
+ Map<MavenProject, Lock> locks = ( Map ) data.get( LOCKS_KEY );
+ if ( locks == null )
+ {
+ data.set( LOCKS_KEY, null, new ConcurrentHashMap<>() );
Review comment:
I don't think there's a concurrency issue here.
The code does the following:
```
Map<MavenProject, Lock> locks = ( Map ) data.get( LOCKS_KEY );
if ( locks == null )
{
data.set( LOCKS_KEY, null, new ConcurrentHashMap<>() );
locks = ( Map ) data.get( LOCKS_KEY );
}
```
The `data.set(k, null, v)` performs a `map.putIfAbsent(k, v)`, so this can not overwrite the value. In case there is a concurrent access to this very method, the following line does a `get`, so I think there's no way the code can use a wrong map.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] MartinKanters commented on a change in pull request #476: [MNG-7156] Parallel build can cause issues between clean and forked g…
Posted by GitBox <gi...@apache.org>.
MartinKanters commented on a change in pull request #476:
URL: https://github.com/apache/maven/pull/476#discussion_r646844509
##########
File path: maven-core/src/main/java/org/apache/maven/execution/LockingEventSpy.java
##########
@@ -0,0 +1,96 @@
+package org.apache.maven.execution;
+
+/*
+ * 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.
+ */
+
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
+
+import javax.inject.Named;
+import javax.inject.Singleton;
+
+import org.apache.maven.eventspy.AbstractEventSpy;
+import org.apache.maven.eventspy.EventSpy;
+import org.apache.maven.project.MavenProject;
+import org.eclipse.aether.SessionData;
+import org.eclipse.sisu.Typed;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * EventSpy implementation that provides a per-project locking mechanism
+ * to make sure a given project can not be build twice concurrently.
+ * This case can happen when running parallel builds with forked lifecycles.
+ */
+@Singleton
+@Named
+@Typed( EventSpy.class )
+public class LockingEventSpy extends AbstractEventSpy
+{
+
+ private final Logger logger = LoggerFactory.getLogger( getClass() );
+
+ private static final Object LOCKS_KEY = new Object();
+
+ @SuppressWarnings( { "unchecked", "rawtypes" } )
+ private Lock getLock( ExecutionEvent event )
+ {
+ SessionData data = event.getSession().getRepositorySession().getData();
+ Map<MavenProject, Lock> locks = ( Map ) data.get( LOCKS_KEY );
+ if ( locks == null )
+ {
+ data.set( LOCKS_KEY, null, new ConcurrentHashMap<>() );
+ locks = ( Map ) data.get( LOCKS_KEY );
+ }
+ return locks.computeIfAbsent( event.getProject(), p -> new ReentrantLock() );
+ }
+
+ @Override
+ public void onEvent( Object event ) throws Exception
+ {
+ if ( event instanceof ExecutionEvent )
+ {
+ ExecutionEvent ee = ( ExecutionEvent ) event;
+ switch ( ee.getType() )
+ {
+ case ProjectStarted:
+ case ForkedProjectStarted:
+ Lock lock = getLock( ee );
+ if ( !lock.tryLock() )
+ {
+ logger.warn( "Suspending concurrent execution of project " + ee.getProject() );
+ getLock( ee ).lockInterruptibly();
+ logger.warn( "Resuming execution of project " + ee.getProject() );
+ }
+ break;
+ case ProjectSucceeded:
+ case ProjectFailed:
Review comment:
I checked the code and it looks like you are right :)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] bmarwell commented on pull request #476: [MNG-7156] Parallel build can cause issues between clean and forked g…
Posted by GitBox <gi...@apache.org>.
bmarwell commented on pull request #476:
URL: https://github.com/apache/maven/pull/476#issuecomment-852784225
Will this also resolve `-DdeployAtEnd=true`?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] michael-o commented on a change in pull request #476: [MNG-7156] Parallel build can cause issues between clean and forked g…
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #476:
URL: https://github.com/apache/maven/pull/476#discussion_r734995702
##########
File path: maven-core/src/main/java/org/apache/maven/execution/LockingEventSpy.java
##########
@@ -0,0 +1,98 @@
+package org.apache.maven.execution;
+
+/*
+ * 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.
+ */
+
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
+
+import javax.inject.Named;
+import javax.inject.Singleton;
+
+import org.apache.maven.eventspy.AbstractEventSpy;
+import org.apache.maven.eventspy.EventSpy;
+import org.apache.maven.project.MavenProject;
+import org.eclipse.aether.SessionData;
+import org.eclipse.sisu.Typed;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * EventSpy implementation that provides a per-project locking mechanism
+ * to make sure a given project can not be built concurrently.
+ * This case can happen when running parallel builds with forked lifecycles.
+ */
+@Singleton
+@Named
+@Typed( EventSpy.class )
+public class LockingEventSpy extends AbstractEventSpy
+{
+
+ private static final Object LOCKS_KEY = new Object();
+
+ private final Logger logger = LoggerFactory.getLogger( getClass() );
+
+ @Override
+ public void onEvent( Object event ) throws Exception
+ {
+ if ( event instanceof ExecutionEvent )
+ {
+ ExecutionEvent executionEvent = ( ExecutionEvent ) event;
+ switch ( executionEvent.getType() )
+ {
+ case ProjectStarted:
+ case ForkedProjectStarted:
+ Lock lock = getLock( executionEvent );
+ if ( !lock.tryLock() )
+ {
+ logger.debug( "Suspending concurrent execution of project '{}'", executionEvent.getProject() );
+ lock.lockInterruptibly();
+ logger.debug( "Resuming execution of project '{}'", executionEvent.getProject() );
+ }
+ break;
+ case ProjectSucceeded:
+ case ProjectFailed:
+ case ForkedProjectSucceeded:
+ case ForkedProjectFailed:
+ getLock( executionEvent ).unlock();
+ break;
+ default:
+ break;
+ }
+ }
+ }
+
+ @SuppressWarnings( { "unchecked", "rawtypes" } )
+ private Lock getLock( ExecutionEvent event )
+ {
+ SessionData data = event.getSession().getRepositorySession().getData();
+ Map<MavenProject, Lock> locks = ( Map ) data.get( LOCKS_KEY );
+ // initialize the value if not already done (in case of a concurrent access) to the method
+ if ( locks == null )
Review comment:
Would it make sense to use `SessionData#computeIfAbsent()` if we release Resolver and integrate?
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org