Skip to content

[MTOOLCHAINS-31] - Not threadsafe for parallel execution#8

Merged
slachiewicz merged 2 commits intoapache:masterfrom
ThomasReinhardt:master
May 2, 2022
Merged

[MTOOLCHAINS-31] - Not threadsafe for parallel execution#8
slachiewicz merged 2 commits intoapache:masterfrom
ThomasReinhardt:master

Conversation

@ThomasReinhardt
Copy link
Contributor

No description provided.

@michael-o
Copy link
Member

And this will make it threadsafe?

@ThomasReinhardt
Copy link
Contributor Author

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

@ThomasReinhardt
Copy link
Contributor Author

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
Copy link

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
Copy link
Member

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

@ThomasReinhardt
Copy link
Contributor Author

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
Copy link

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
Copy link

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
Copy link
Contributor Author

Thanks @Bananeweizen. I added a synchronized block.

@michael-o
Copy link
Member

Remove the swap file please.

@ThomasReinhardt
Copy link
Contributor Author

Sorry. Its gone.

@tivervac
Copy link

tivervac commented May 2, 2022

@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?

@slachiewicz slachiewicz merged commit d4f73c0 into apache:master May 2, 2022
@viltbalint
Copy link

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
Copy link
Member

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

@jira-importer
Copy link

Resolve #95

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants