OpenLiberty / ci.maven

Maven plugins for managing Liberty profile servers #devops
Apache License 2.0
130 stars 91 forks source link

Could this plugin be marked thread safe? #1693

Closed blirp closed 10 months ago

blirp commented 1 year ago

Running with f.x. '-T 2C' gives the following: [WARNING] The following goals are not marked as thread-safe in Application [WARNING] io.openliberty.tools:liberty-maven-plugin:3.8.2:deploy [WARNING] io.openliberty.tools:liberty-maven-plugin:3.8.2:create [WARNING] io.openliberty.tools:liberty-maven-plugin:3.8.2:install-feature [WARNING] io.openliberty.tools:liberty-maven-plugin:3.8.2:install-server

I would expect code in 2023 to be thread safe given that all processors hva multiple cores. What's holding this plugin back?

cherylking commented 1 year ago

@blirp Thank you for bringing this to our attention. We would like to understand your particular use case. Can you describe your project configuration / setup?

We have no reason to believe that our goals (other than dev mode which is meant to be a single interactive session) are not thread safe, but we have not taken the step to document them as such.

cherylking commented 1 year ago

I have found two instances of static variables that should be looked at. One is in CommonLogger (this may be more tricky to change) and the other is in StartDebugMojoSupport (configFilesCopied variable).

blirp commented 1 year ago

I have a medium sized application with 44 modules in total, and about 500K of Java code. (And an unknown number of Spring XML configuration). The application is split into three main modules, 'common', 'frontend' and 'backend'. Mostly JSP. We use Liberty for both the frontend and backend modules.

My hope was that giving Maven some more threads, we could speed up build times a bit. That gave various warnings about non-thread-safe plugins. Most I could easily update, but not the liberty-maven-plugin.

bmarwell commented 10 months ago

I have found two instances of static variables that should be looked at. One is in CommonLogger (this may be more tricky to change) and the other is in StartDebugMojoSupport (configFilesCopied variable).

Why do you thing the static logger could be a problem? If it is final, I do not see a problem here.

and the other is in StartDebugMojoSupport (configFilesCopied variable).

This one: https://github.com/OpenLiberty/ci.maven/blob/4ee535ef2097c0d29c4ef7e3b7983cd874d254f4/liberty-maven-plugin/src/main/java/io/openliberty/tools/maven/server/StartDebugMojoSupport.java#L88

I don't see why not just make it non-static. If I am not mistaken, maven plugins are loaded in their own ClassLoader. That means as long as the plugin does not run twice in the same module at the same time with the same goal (which is unlikely for StartDebugMojoSupport), this should work.

cherylking commented 10 months ago

@bmarwell Thanks for looking at those. I agree that the static variable in StartDebugMojoSupport can just be made non-static.

As for the CommonLogger, I suppose I could make the logger static variable final. But the loggerImpl referenced by that static logger can change with every call to getInstance, which seems problematic to me if called from multiple threads. Maybe I am overthinking it?

cherylking commented 10 months ago

Just found another class that needs modifications for thread safety in ci.common. It is utilizing a lot of static variables. Not sure of the history there. Will look into changing those to non-static variables.