[GitHub] [maven] gnodet commented on pull request #413: [MNG-6843] Thread-safe artifacts in MavenProject
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: us...@infra.apache.org
[GitHub] [maven] gnodet commented on pull request #413: [MNG-6843] Thread-safe artifacts in MavenProject
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: us...@infra.apache.org
[GitHub] [maven] gnodet commented on pull request #413: [MNG-6843] Thread-safe artifacts in MavenProject
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: us...@infra.apache.org
[GitHub] [maven] gnodet commented on pull request #413: [MNG-6843] Thread-safe artifacts in MavenProject
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: us...@infra.apache.org
[GitHub] [maven] gnodet commented on pull request #413: [MNG-6843] Thread-safe artifacts in MavenProject
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: us...@infra.apache.org
[GitHub] [maven] gnodet commented on pull request #413: [MNG-6843] Thread-safe artifacts in MavenProject
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: us...@infra.apache.org
[GitHub] [maven] gnodet commented on pull request #413: [MNG-6843] Thread-safe artifacts in MavenProject
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: us...@infra.apache.org
[GitHub] [maven] gnodet commented on pull request #413: [MNG-6843] Thread-safe artifacts in MavenProject
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: us...@infra.apache.org
[GitHub] [maven] gnodet commented on pull request #413: [MNG-6843] Thread-safe artifacts in MavenProject
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: us...@infra.apache.org
