[MTOOLCHAINS-31] - Not threadsafe for parallel execution

[ThomasReinhardt]

No description provided.

[michael-o]

And this will make it threadsafe?

[ThomasReinhardt]

It already was threadsafe. The change only marks it as such and thus removes the (false) warning.

[ThomasReinhardt]

See the jira page for my (brief) comment why this is true:
https://issues.apache.org/jira/browse/MTOOLCHAINS-31?jql=project%20%3D%20MTOOLCHAINS

[Bananeweizen]

Can someone with the necessary powers please retrigger the build? To me the change seems perfectly valid, so this is probably rather a hickup in the infrastructure.

[michael-o]

How will this flag make it threadsafe? Did you review the code?

[ThomasReinhardt]

This plugin is already threadsafe (see below). It probably was since a very long time but nobody cared to investigate. This change is only marking the plugin as threadsafe. It changes absolutely nothing execution-wise.

Why do I think it is threadsafe? As I wrote on the jira page it does not hold any state and gets instantiated PER_LOOKUP. So there is no way it can share state across instantiations. Which also means there are no collisions, races etc. In fact, the plugin should most likely be optimized, because now every invokation reads toolchains.xml which is quite inefficient. But it is threadsafe and this is what this PR is about.

[Bananeweizen]

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).

[Bananeweizen]

An example change to use a local lock object for synchronization can be seen in the first file change of eclipse-tycho/tycho@8afcd61. That's not the only maven plugin where this technique has been applied, so it can be trusted for sure.

[ThomasReinhardt]

Thanks @Bananeweizen. I added a synchronized block.

[michael-o]

Remove the swap file please.

[ThomasReinhardt]

Sorry. Its gone.

[tivervac]

@michael-o This seems ready for merge now. Is anything still missing? If it does get merged, when can we reasonably expect to see this in a new release?

[viltbalint]

It would be also my question, that do you have a planned date for the release? I couldn't find any information about that.

[michael-o]

@tivervac @cstamas and me are working on 3.1.0. He'll adapt for Maven 3.2.5 and I'll do the release.