Bananeweizen commented on pull request #8: URL: https://github.com/apache/maven-toolchains-plugin/pull/8#issuecomment-1055207088
Yes, I reviewed the code. When making a maven plugin thread-safe, typical issues are the following: * static fields being used in the plugin. Since those are accessed from all concurrent invocations of the plugin simultaneously, they need proper synchronization. There is no such field here. * calling methods from the Maven core or Maven utility classes that are not thread-safe on their own, and therefore again would need proper synchronization per call in this plugin. The 3 Java files of this plugin look okay. There is one where I'm not perfectly sure, the call to ToolchainManagerPrivate.storeToolchainToBuildContext(...). If you still have a bad feeling about the change, there would be a simple alternative implementation to _definitely_ make it thread safe: The plugin would declare an additional lock object, and nest the complete code of the "execute" method via that lock object. That would lead to any additional concurrent invocation of this plugin waiting for the already running first invocation of the same plugin. That means Maven would still be able to execute all kinds of _other_ maven plugin mojos in parallel, only if this plugin would be called 2 times in parallel, one of those would wait for the other to finish. I can provide a sample implementation if necessary (from other maven plugins). -- 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-unsubscr...@maven.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
