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 2020/12/20 21:30:58 UTC
[GitHub] [maven] famod opened a new pull request #413: [MNG-6843] Thread-safe artifacts in MavenProject
famod opened a new pull request #413:
URL: https://github.com/apache/maven/pull/413
Avoids concurrency issues when aggregator plugins are involved.
Might also fix:
- https://issues.apache.org/jira/browse/MNG-4996
- https://issues.apache.org/jira/browse/MNG-5750
- https://issues.apache.org/jira/browse/MNG-5960
This is an alternative to #310 in which @rfscholte raised concerns whether cloning is the right approach.
I went with a wrapper and a single `ThreadLocal` because mutliple TLs would bloat the code and would theoretically increase overhead (haven't measured it, though).
I had to be creative with removing empty lines because Checkstyle was complaining about the class exceeding 2000 lines.
PS: I removed the checklist to avoid noise (checked everything, ICLA is present).
----------------------------------------------------------------
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 pull request #413: [MNG-6843] Thread-safe artifacts in MavenProject
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #413:
URL: https://github.com/apache/maven/pull/413#issuecomment-748679583
What error message do you get?
----------------------------------------------------------------
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 edited a comment on pull request #413: [MNG-6843] Thread-safe artifacts in MavenProject
Posted by GitBox <gi...@apache.org>.
famod edited a comment on pull request #413:
URL: https://github.com/apache/maven/pull/413#issuecomment-749497802
All green! 🚀
----------------------------------------------------------------
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 pull request #413: [MNG-6843] Thread-safe artifacts in MavenProject
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #413:
URL: https://github.com/apache/maven/pull/413#issuecomment-841721284
Then the usage of ThreadLocal is still wrong because one thread may see multiple Mojos. If the filter should be different for each Mojo, then it is design problem. Why to handle Mojo's variables outside? Maybe some Memento object should be apart of MavenProject.
--
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 pull request #413: [MNG-6843] Thread-safe artifacts in MavenProject
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #413:
URL: https://github.com/apache/maven/pull/413#issuecomment-749412292
PR is 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 #413: [MNG-6843] Thread-safe artifacts in MavenProject
Posted by GitBox <gi...@apache.org>.
gnodet commented on pull request #413:
URL: https://github.com/apache/maven/pull/413#issuecomment-841721374
> Then the usage of ThreadLocal is still wrong because one thread may see multiple Mojos. If the filter should be different for each Mojo, then it is design problem. Why to handle Mojo's variables outside? Maybe some Memento object should be apart of MavenProject.
How could that happen ? A thread can not execute multiple mojo concurrently...
--
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 pull request #413: [MNG-6843] Thread-safe artifacts in MavenProject
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #413:
URL: https://github.com/apache/maven/pull/413#issuecomment-767523804
Please ping me next week, I am current incapable to do anything fruitful. Alternatively, go through Core ITs commits from me and check Plugin changes.
----------------------------------------------------------------
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 pull request #413: [MNG-6843] Thread-safe artifacts in MavenProject
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #413:
URL: https://github.com/apache/maven/pull/413#issuecomment-840140360
Why we do it so complicated? The threads do not share their objects with ThreadLocal-s. So every Thread will see another copy.
If I was fixing this issue, I would use thread-safe collections and `synchronized` critical section `setArtifacts()` because first you have to clear the collection and then set new content in the critical section. That's it.
But I am convinced that the issues in Maven are due to Java Memory Model that objects in `ArrayList` are weakly consistent. So you have to use the COWAL collection instead. Example, even if you use any treatment in this class, you pass a reference of the Set to the setter `setArtifacts()` you still may have a problem because the caller thread used thread-unsafe `Set`. So the caller must create ideally immutable Set or thread safe Set. If a copy of the content is acceptable then refuse copying the reference and the caller can be any thread unsafe object.
--
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 #413: [MNG-6843] Thread-safe artifacts in MavenProject
Posted by GitBox <gi...@apache.org>.
famod commented on pull request #413:
URL: https://github.com/apache/maven/pull/413#issuecomment-748678853
I'm also getting this `MavenITmng3259DepsDroppedInMultiModuleBuildTest` failure on JDK16 locally _on `master`_.
So I don't think my changes have anything to do with 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] famod commented on pull request #413: [MNG-6843] Thread-safe artifacts in MavenProject
Posted by GitBox <gi...@apache.org>.
famod commented on pull request #413:
URL: https://github.com/apache/maven/pull/413#issuecomment-767452439
@michael-o friendly reminder:
> Can you point me to such a test with a custom mojo? I would then try to take a (timeboxed) stab at it.
> Thanks!
----------------------------------------------------------------
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 edited a comment on pull request #413: [MNG-6843] Thread-safe artifacts in MavenProject
Posted by GitBox <gi...@apache.org>.
gnodet edited a comment on pull request #413:
URL: https://github.com/apache/maven/pull/413#issuecomment-841721374
> Then the usage of ThreadLocal is still wrong because one thread may see multiple Mojos. If the filter should be different for each Mojo, then it is design problem. Why to handle Mojo's variables outside? Maybe some Memento object should be apart of MavenProject.
How could that happen ? A thread can not execute multiple mojo concurrently... And yes, there's a design problem, but this PR is about fixing a bug imho, not changing the design of the core maven API.
--
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 pull request #413: [MNG-6843] Thread-safe artifacts in MavenProject
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #413:
URL: https://github.com/apache/maven/pull/413#issuecomment-754665591
I guess so, that's the problem. What if one writes a sleep mojo?
----------------------------------------------------------------
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 #413: [MNG-6843] Thread-safe artifacts in MavenProject
Posted by GitBox <gi...@apache.org>.
gnodet commented on pull request #413:
URL: https://github.com/apache/maven/pull/413#issuecomment-841842316
> @famod
> @gnodet
> Speaking for myself, I would propose the following
>
> 1. deprecate the method `getArtifacts()`, and
> 2. introduce a new method getArtifacts( ArtifactFilter artifactFilter )
>
> It looks to me that the plugin itself is the owner of the object `ArtifactFilter` and not the object `MavenProject`. WDYT?
That was my initial thought too. But I think it does not really work, as the `ArtifactFilter` is not known directly my the mojo being executed (it's constructed by maven internals through mojo annotations / javadoc tags iirc) and the call sites for `getArtifacts()` usually have no knowledge of the mojo being executed 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 edited a comment on pull request #413: [MNG-6843] Thread-safe artifacts in MavenProject
Posted by GitBox <gi...@apache.org>.
gnodet edited a comment on pull request #413:
URL: https://github.com/apache/maven/pull/413#issuecomment-841842316
> @famod
> @gnodet
> Speaking for myself, I would propose the following
>
> 1. deprecate the method `getArtifacts()`, and
> 2. introduce a new method getArtifacts( ArtifactFilter artifactFilter )
>
> It looks to me that the plugin itself is the owner of the object `ArtifactFilter` and not the object `MavenProject`. WDYT?
That was my initial thought too. But I think it does not really work, as the `ArtifactFilter` is not known directly by the mojo being executed (it's constructed by maven internals through mojo annotations / javadoc tags iirc) and the call sites for `getArtifacts()` usually have no knowledge of the mojo being executed 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] michael-o commented on pull request #413: [MNG-6843] Thread-safe artifacts in MavenProject
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #413:
URL: https://github.com/apache/maven/pull/413#issuecomment-748856685
This likes needs an XStream update...
----------------------------------------------------------------
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 pull request #413: [MNG-6843] Thread-safe artifacts in MavenProject
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #413:
URL: https://github.com/apache/maven/pull/413#issuecomment-754661032
I would at least have @rfscholte and @khmarbaise look over. @famod Do you think an IT is possible here?
----------------------------------------------------------------
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 #413: [MNG-6843] Thread-safe artifacts in MavenProject
Posted by GitBox <gi...@apache.org>.
gnodet commented on pull request #413:
URL: https://github.com/apache/maven/pull/413#issuecomment-841720517
@Tibor17 this is not a memory model problem here. The analysis done by @famod shows that when using a parallel build, multiple mojos may use the fields on the MavenProject concurrently but with different values (the artifactFilter may be different for each mojo, hence the resulting artifacts list). So synchronizing on those variables would not help, as one mojo may write a value but later read a non coherent artifacts list. The problem here is that the `artifactFilter` and `artifacts` are specific to a given mojo execution.
--
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 #413: [MNG-6843] Thread-safe artifacts in MavenProject
Posted by GitBox <gi...@apache.org>.
famod commented on pull request #413:
URL: https://github.com/apache/maven/pull/413#issuecomment-754670207
That could work, with a serious amount of effort though. Are there any exiting ITs that come with their own custom mojos?
----------------------------------------------------------------
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 #413: [MNG-6843] Thread-safe artifacts in MavenProject
Posted by GitBox <gi...@apache.org>.
famod commented on pull request #413:
URL: https://github.com/apache/maven/pull/413#issuecomment-841332349
@gnodet @Tibor17 Have you guys read https://github.com/apache/maven/pull/310#issuecomment-617783528? (and maybe also https://github.com/apache/maven/pull/310#issuecomment-612659076)?
The main problem is here: https://github.com/apache/maven/blob/c3cf29438e3d65d6ee5c5726f8611af99d9a649a/maven-core/src/main/java/org/apache/maven/lifecycle/internal/MojoExecutor.java#L282
I don't see how a fine grained synchronization or a COWAL in `MavenProject` can help here. The execution of an aggregation goal would have to lock all other invocations of `getArtifacts()` etc. (however those may look like) until done with the artifact processing.
Overhauling the (problematic) design of aggregation goal handling would be the most sustainable solution. But neither me nor the others that took a stab at this have the knowledge or capacity to do that.
But even then: `MavenProject` is highly mutable, there are setters everywhere (which is good for Maven extensions that enhance the Maven experience but often bad for concurrency stability).
--
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 pull request #413: [MNG-6843] Thread-safe artifacts in MavenProject
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #413:
URL: https://github.com/apache/maven/pull/413#issuecomment-841717561
--
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 pull request #413: [MNG-6843] Thread-safe artifacts in MavenProject
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #413:
URL: https://github.com/apache/maven/pull/413#issuecomment-868311020
Will check this night.
--
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 pull request #413: [MNG-6843] Thread-safe artifacts in MavenProject
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #413:
URL: https://github.com/apache/maven/pull/413#issuecomment-841721568
but for me, a fix means removing the design problem
--
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] rfscholte closed pull request #413: [MNG-6843] Thread-safe artifacts in MavenProject
Posted by GitBox <gi...@apache.org>.
rfscholte closed pull request #413:
URL: https://github.com/apache/maven/pull/413
--
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] rfscholte commented on pull request #413: [MNG-6843] Thread-safe artifacts in MavenProject
Posted by GitBox <gi...@apache.org>.
rfscholte commented on pull request #413:
URL: https://github.com/apache/maven/pull/413#issuecomment-868723067
Merged with https://github.com/apache/maven/commit/73e00ed85df84ba0c557dd020740812b2453f2d3
--
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 commented on pull request #413: [MNG-6843] Thread-safe artifacts in MavenProject
Posted by GitBox <gi...@apache.org>.
famod commented on pull request #413:
URL: https://github.com/apache/maven/pull/413#issuecomment-754664587
Thanks for your quick answer.
> Do you think an IT is possible here?
One that is trying to provoke the concurrency issue? I fear such a test would be rather flaky...
----------------------------------------------------------------
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 pull request #413: [MNG-6843] Thread-safe artifacts in MavenProject
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #413:
URL: https://github.com/apache/maven/pull/413#issuecomment-841815009
@famod
@gnodet
Speaking for myself, I would propose the following
1. deprecate the method `getArtifacts()`, and
2. introduce a new method getArtifacts( ArtifactFilter artifactFilter )
It looks to me that the plugin itself is the owner of the object `ArtifactFilter` and not the object `MavenProject`. WDYT?
--
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 edited a comment on pull request #413: [MNG-6843] Thread-safe artifacts in MavenProject
Posted by GitBox <gi...@apache.org>.
Tibor17 edited a comment on pull request #413:
URL: https://github.com/apache/maven/pull/413#issuecomment-840140360
Why we do it so complicated? The threads do not share their objects with ThreadLocal-s. So every Thread will see another copy.
If I was fixing this issue, I would use thread-safe collections and `synchronized` critical section `setArtifacts()` because first you have to clear the collection and then set new content in the critical section. That's it.
But I am convinced that the issues in Maven are due to Java Memory Model that objects in `ArrayList` are weakly consistent. So you have to use the COWAL collection instead. Example, even if you use any treatment in this class, you pass a reference of the Set to the setter `setArtifacts()` you still may have a problem because the caller thread used thread-unsafe `Set`. So the caller must create ideally immutable Set or thread safe Set in order to copy the reference. If a copy of the content of `Set` is acceptable then refuse copying the reference and the caller can be any thread unsafe object.
--
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 #413: [MNG-6843] Thread-safe artifacts in MavenProject
Posted by GitBox <gi...@apache.org>.
gnodet commented on pull request #413:
URL: https://github.com/apache/maven/pull/413#issuecomment-841721932
That
> but for me, a fix means removing the design problem
A fix would require to remove the `MavenProject.getArtifacts()` method somehow. It's part of the core mojo API, so I agree not sure how that can be done in a short time frame. In the mean time, any acceptable bug to be able to use parallel builds in a more robust way would be handy...
--
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 edited a comment on pull request #413: [MNG-6843] Thread-safe artifacts in MavenProject
Posted by GitBox <gi...@apache.org>.
gnodet edited a comment on pull request #413:
URL: https://github.com/apache/maven/pull/413#issuecomment-841721932
That
> but for me, a fix means removing the design problem
A fix would require to remove the `MavenProject.getArtifacts()` method somehow. It's part of the core mojo API, so not sure how that can be done in a short time frame. In the mean time, any acceptable bug to be able to use parallel builds in a more robust way would be handy...
--
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 edited a comment on pull request #413: [MNG-6843] Thread-safe artifacts in MavenProject
Posted by GitBox <gi...@apache.org>.
famod edited a comment on pull request #413:
URL: https://github.com/apache/maven/pull/413#issuecomment-841332349
@gnodet @Tibor17 Have you guys read https://github.com/apache/maven/pull/310#issuecomment-617783528? (and maybe also https://github.com/apache/maven/pull/310#issuecomment-612659076)?
The main problem is here: https://github.com/apache/maven/blob/c3cf29438e3d65d6ee5c5726f8611af99d9a649a/maven-core/src/main/java/org/apache/maven/lifecycle/internal/MojoExecutor.java#L280-L283
I don't see how a fine grained synchronization or a COWAL in `MavenProject` can help here. The execution of an aggregation goal would have to lock all other invocations of `getArtifacts()` etc. (however those may look like) until done with the artifact processing.
Overhauling the (problematic) design of aggregation goal handling would be the most sustainable solution. But neither me nor the others that took a stab at this have the knowledge or capacity to do that.
But even then: `MavenProject` is highly mutable, there are setters everywhere (which is good for Maven extensions that enhance the Maven experience but often bad for concurrency stability).
--
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 #413: [MNG-6843] Thread-safe artifacts in MavenProject
Posted by GitBox <gi...@apache.org>.
gnodet commented on pull request #413:
URL: https://github.com/apache/maven/pull/413#issuecomment-840080964
@famod In your experiments, is there any case where the output of the computation of the artifacts list is not always the same for a given project ? I mean, why choosing a TLS rather than simply adding a `synchronized` on the `getArtifacts()` method (or any fine grained synchronization fwiw) ?
--
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 #413: [MNG-6843] Thread-safe artifacts in MavenProject
Posted by GitBox <gi...@apache.org>.
gnodet commented on pull request #413:
URL: https://github.com/apache/maven/pull/413#issuecomment-852829527
@Tibor17 See https://github.com/apache/maven/pull/475
--
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 pull request #413: [MNG-6843] Thread-safe artifacts in MavenProject
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #413:
URL: https://github.com/apache/maven/pull/413#issuecomment-841717561
@famod
Many people thing that the problem is always about writes. Wrong! The concurrency is also about reads, which you did not show me! It is not about happens before relationship in your code because it is about this theorem in the memory and you never know when the (internal index: int, array: [] of ArrayList) appears in CPU registers and cache (Java working copy), or RAM (Java main shared memory). If you use synchronization on MavenProject's methods or thread safe collections, you know that the happens before theorem works as always, and you can just sleep well.
--
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 edited a comment on pull request #413: [MNG-6843] Thread-safe artifacts in MavenProject
Posted by GitBox <gi...@apache.org>.
gnodet edited a comment on pull request #413:
URL: https://github.com/apache/maven/pull/413#issuecomment-841721374
> Then the usage of ThreadLocal is still wrong because one thread may see multiple Mojos. If the filter should be different for each Mojo, then it is design problem. Why to handle Mojo's variables outside? Maybe some Memento object should be apart of MavenProject.
How could that happen ? A thread can not execute multiple mojo concurrently...
And yes, there's a design problem, but this PR is about fixing a bug imho, not changing the design of the core maven API.
--
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 #413: [MNG-6843] Thread-safe artifacts in MavenProject
Posted by GitBox <gi...@apache.org>.
gnodet commented on pull request #413:
URL: https://github.com/apache/maven/pull/413#issuecomment-841720517
--
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 #413: [MNG-6843] Thread-safe artifacts in MavenProject
Posted by GitBox <gi...@apache.org>.
famod commented on pull request #413:
URL: https://github.com/apache/maven/pull/413#issuecomment-749497802
All green! 🎉
----------------------------------------------------------------
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 #413: [MNG-6843] Thread-safe artifacts in MavenProject
Posted by GitBox <gi...@apache.org>.
famod commented on pull request #413:
URL: https://github.com/apache/maven/pull/413#issuecomment-761856773
@rfscholte Thanks!
@michael-o
> I need search because I have touched one recently.
Can you point me to such a test with a custom mojo? I would then try to take a (timeboxed) stab at it.
Thanks!
----------------------------------------------------------------
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 edited a comment on pull request #413: [MNG-6843] Thread-safe artifacts in MavenProject
Posted by GitBox <gi...@apache.org>.
gnodet edited a comment on pull request #413:
URL: https://github.com/apache/maven/pull/413#issuecomment-841721374
--
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 #413: [MNG-6843] Thread-safe artifacts in MavenProject
Posted by GitBox <gi...@apache.org>.
gnodet commented on pull request #413:
URL: https://github.com/apache/maven/pull/413#issuecomment-842039952
> > @famod
> > @gnodet
> > Speaking for myself, I would propose the following
> >
> > 1. deprecate the method `getArtifacts()`, and
> > 2. introduce a new method getArtifacts( ArtifactFilter artifactFilter )
> >
> > It looks to me that the plugin itself is the owner of the object `ArtifactFilter` and not the object `MavenProject`. WDYT?
>
> That was my initial thought too. But I think it does not really work, as the `ArtifactFilter` is not known directly by the mojo being executed (it's constructed by maven internals through mojo annotations / javadoc tags iirc) and the call sites for `getArtifacts()` usually have no knowledge of the mojo being executed afaik...
I may be wrong about the call sites, they may be inside the mojos most of the cases.
One possibility could be to inject the `ArtifactFilter` into mojos, in which case, they could easily replace the `getArtifacts()` or similar calls with `getArtifacts(artifactFilter)`. Other methods like `getCompileClasspathElements()`, `getTestClasspathElements()`, `getRuntimeClasspathElements()` will need some investigation too.
--
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 pull request #413: [MNG-6843] Thread-safe artifacts in MavenProject
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #413:
URL: https://github.com/apache/maven/pull/413#issuecomment-754679802
Yes, there are. I need search because I have touched one recently.
----------------------------------------------------------------
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 #413: [MNG-6843] Thread-safe artifacts in MavenProject
Posted by GitBox <gi...@apache.org>.
famod commented on pull request #413:
URL: https://github.com/apache/maven/pull/413#issuecomment-754660148
@michael-o So how do we move on with this? Who needs to approve?
----------------------------------------------------------------
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 edited a comment on pull request #413: [MNG-6843] Thread-safe artifacts in MavenProject
Posted by GitBox <gi...@apache.org>.
famod edited a comment on pull request #413:
URL: https://github.com/apache/maven/pull/413#issuecomment-749262668
@michael-o How come other PRs don't show this failure? I also had a brief look at your Jenkins jobs and I couldn't find anything there either (but I may have looked in the wrong place).
I have a fix, simply updating xstream to 1.4.15 in `core-it-suite/src/test/resources/mng-3259/parent/pom.xml` does the trick.
~~Do you want a separate PR for that?~~ _Edit: Of course you do, it's a separate repo._ 🤦 If yes, I hope I don't need a MNG ticket...?
----------------------------------------------------------------
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 #413: [MNG-6843] Thread-safe artifacts in MavenProject
Posted by GitBox <gi...@apache.org>.
gnodet commented on pull request #413:
URL: https://github.com/apache/maven/pull/413#issuecomment-841434518
> #310 (comment)
Ah, got it, thx.
--
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 #413: [MNG-6843] Thread-safe artifacts in MavenProject
Posted by GitBox <gi...@apache.org>.
famod commented on pull request #413:
URL: https://github.com/apache/maven/pull/413#issuecomment-749262668
@michael-o How come other PRs don't show this failure? I also had a brief look at your Jenkins jobs and I couldn't find anything there either (but I may have looked in the wrong place).
I have a fix, simply updating xstream to 1.4.15 in `core-it-suite/src/test/resources/mng-3259/parent/pom.xml` does the trick.
Do you want a separate PR for that? If yes, I hope I don't need a MNG ticket...?
----------------------------------------------------------------
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 #413: [MNG-6843] Thread-safe artifacts in MavenProject
Posted by GitBox <gi...@apache.org>.
famod commented on pull request #413:
URL: https://github.com/apache/maven/pull/413#issuecomment-748679993
@michael-o
```
Exit code was non-zero: 1
...
[INFO] --- maven-surefire-plugin:2.3:test (default-test) @ mng-module5-XXX ---
[INFO] Surefire report directory: C:\_dev\git\maven-integration-testing\core-it-suite\target\test-classes\mng-3259\module5\target\surefire-reports
-------------------------------------------------------
T E S T S
-------------------------------------------------------
Running mng.Module5Test
Tests run: 2, Failures: 0, Errors: 2, Skipped: 0, Time elapsed: 0.056 sec <<< FAILURE!
testJMockAvailable(mng.Module5Test) Time elapsed: 0.044 sec <<< ERROR!
java.lang.ExceptionInInitializerError
at com.thoughtworks.xstream.XStream.setupConverters(XStream.java:584)
at com.thoughtworks.xstream.XStream.<init>(XStream.java:375)
at com.thoughtworks.xstream.XStream.<init>(XStream.java:301)
at mng.XStreamTestCase.setUp(XStreamTestCase.java:13)
at org.jmock.core.VerifyingTestCase.runBare(VerifyingTestCase.java:37)
at junit.framework.TestResult$1.protect(TestResult.java:106)
at junit.framework.TestResult.runProtected(TestResult.java:124)
at junit.framework.TestResult.run(TestResult.java:109)
at junit.framework.TestCase.run(TestCase.java:120)
at junit.framework.TestSuite.runTest(TestSuite.java:230)
at junit.framework.TestSuite.run(TestSuite.java:225)
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:78)
at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.base/java.lang.reflect.Method.invoke(Method.java:567)
at org.apache.maven.surefire.junit.JUnitTestSet.execute(JUnitTestSet.java:213)
at org.apache.maven.surefire.suite.AbstractDirectoryTestSuite.executeTestSet(AbstractDirectoryTestSuite.java:138)
at org.apache.maven.surefire.suite.AbstractDirectoryTestSuite.execute(AbstractDirectoryTestSuite.java:125)
at org.apache.maven.surefire.Surefire.run(Surefire.java:132)
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:78)
at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.base/java.lang.reflect.Method.invoke(Method.java:567)
at org.apache.maven.surefire.booter.SurefireBooter.runSuitesInProcess(SurefireBooter.java:290)
at org.apache.maven.surefire.booter.SurefireBooter.main(SurefireBooter.java:818)
Caused by: java.lang.reflect.InaccessibleObjectException: Unable to make field protected volatile java.util.Properties java.util.Properties.defaults accessible: module java.base does not "opens java.util" to unnamed module @5f205aa
at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:357)
at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:297)
at java.base/java.lang.reflect.Field.checkCanSetAccessible(Field.java:177)
at java.base/java.lang.reflect.Field.setAccessible(Field.java:171)
at com.thoughtworks.xstream.core.util.Fields.find(Fields.java:15)
at com.thoughtworks.xstream.converters.collections.PropertiesConverter.<clinit>(PropertiesConverter.java:29)
... 25 more
```
CI looks the same, AFAICT.
----------------------------------------------------------------
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