Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mark all goals as thread-safe #90

Open
ManuelB opened this issue Aug 12, 2019 · 5 comments
Open

Mark all goals as thread-safe #90

ManuelB opened this issue Aug 12, 2019 · 5 comments

Comments

@ManuelB
Copy link

ManuelB commented Aug 12, 2019

We are using parallized maven builds (mvn -T1C). We do not experience any issues but we are currently getting the following Warning:

mvn -T1C -X install
...
[WARNING] *****************************************************************
[WARNING] * Your build is requesting parallel execution, but project      *
[WARNING] * contains the following plugin(s) that have goals not marked   *
[WARNING] * as @threadSafe to support parallel building.                  *
[WARNING] * While this /may/ work fine, please look for plugin updates    *
[WARNING] * and/or request plugins be made thread-safe.                   *
[WARNING] * If reporting an issue, report it against the plugin in        *
[WARNING] * question, not against maven-core                              *
[WARNING] *****************************************************************
[WARNING] The following goals are not marked @threadSafe in appstore:
[WARNING] com.lazerycode.selenium:driver-binary-downloader-maven-plugin:1.0.17:selenium

Is it possible to add @threadsafe to the selenium goal?

ManuelB added a commit to ManuelB/driver-binary-downloader-maven-plugin that referenced this issue Aug 12, 2019
@Ardesco
Copy link
Owner

Ardesco commented Aug 12, 2019

The plugin is not thread safe. It uses a static download location, and it extracts binaries to a single location, if multiple instances of the plugin try to download/extract at the same time you could end up with corrupted downloads/binaries.

ManuelB added a commit to ManuelB/driver-binary-downloader-maven-plugin that referenced this issue Aug 12, 2019
ManuelB added a commit to ManuelB/driver-binary-downloader-maven-plugin that referenced this issue Aug 12, 2019
@ManuelB
Copy link
Author

ManuelB commented Aug 12, 2019

@Ardesco I reviewed the mentioned parts and added synchronized blocks.

ManuelB@833c0ee

We are already running the plugin for multiple month with mvn -T1C and it works without problems.

Is there anything else that I have to do?

@mpkorstanje
Copy link
Contributor

@Ardesco
Copy link
Owner

Ardesco commented Aug 12, 2019

To start off with, the fact you have had no problems running with multiple threads over x period of time does not mean that the plugin is thread safe, it just means you haven't got unlucky yet.

Making the code thread safe is rarely as simple as sticking a synchronised keyword in a few places, all you are doing is forcing the code to queue up multiple requests that will be executed in a single thread (In other words you are blocking the code from running in multiple threads).

To make the plugin thread safe we would need to do the following as a minimum:

  • Check all dependencies are thread safe and change them/get their authors to make them threads safe and update them if they are not.
  • Check all static member variables are using thread safe data types and change them if they are not.
  • To ensure acceptable performance levels you should limit the use of synchronized and instead make the code smarter e.g. if multiple threads want to download a file, one thread should download it and then report to the other threads that it was downloaded successfully, you don't want to end up downloading the same file 5 times in sequence (Or in the case of your PR depending upon which call gets in next we may perform 2 downloads, then an unzip, then another download, then two more unzips, etc, etc).
  • The plugin works with external files that are not part of the JVM, we should really be locking them from an os level before working on them (this would also protect you against two maven runs running in parallel in separate terminals).

It is possible to make the plugin thread safe, but it's going to take time, effort and require lots of testing.

Also please do bear in mind that what you are seeing in your maven console is simply a warning, if everything works fine for you, you can just disregard it; it is a warning after all.

@ManuelB
Copy link
Author

ManuelB commented Aug 12, 2019

@Ardesco thanks a lot for your description. I think I won't get the time from my employer to do the necessary work. For now I will just ignore the warning.

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

No branches or pull requests

3 participants