spotbugs / spotbugs-gradle-plugin

https://plugins.gradle.org/plugin/com.github.spotbugs
Apache License 2.0
178 stars 68 forks source link

Multiple runs and even builds for different projects influence each other #113

Open Vampire opened 5 years ago

Vampire commented 5 years ago

SpotBugs uses static stuff which influences multiple runs and even builds for different projects and thus makes things different depending on situation and actually unpredictable.

Where I observed it is with spotbugs plugins as it gets pretty obviously and loudly displayed in this case, but there could of course be other and more subtle occurances that are not as obvious in effect.

Here two independent simple ways to reproduce the problem (simpler usage also triggers this, but with these instructions it should be 100% reproducible):

For all, add one or more spotbugs plugins, and make sure at least one bug will be found in each of the tasks run (somhow couldn't reproduce on first try without found bugs, didn't investigate why)

  1. one task in multiple runs in the same daemon and same worker

    • run gradlew --stop
    • run gradlew -i --daemon --max-workers 1 spotbugsMain
    • everything went like expected
    • run gradlew -i --daemon --max-workers 1 spotbugsMain
    • now you get a bunch of Trying to add already registered factory warnings (if not, make sure that the last run had as one of its first lines Starting 2nd build in daemon)
  2. multiple tasks in one run even without daemon in the same worker

    • run gradlew -i --no-daemon --continue --max-workers 1 spotbugsMain spotbugsTest
    • spotbugsMain will go like expected
    • for spotbugsTest you get a bunch of Trying to add already registered factory warnings

The cause here is, that the plugins are loaded with DetectorFactoryCollection.instance().loadPlugin(plugin); into a static singleton, so if the same worker JVM is reused, the previously registered plugins are still there. Besides the harmless warning (as long as the settings are the same), this of course also means that the plugins stay loaded, even if a different build does not want them. This can happen with different SpotBugs tasks in the same project that are configured differently, but even worse it can happen with totally independent projects that are built in the same Gradle daemon where the SpotBugs tasks are run on the same worker JVMs.

In case of the DetectorFactoryCollection, it could maybe be worked around by using edu.umd.cs.findbugs.DetectorFactoryCollection#resetInstance, but according to its documentation it is public for tests only! and maybe there are other stuff that is kept statically too.

So I guess to fix this, either SpotBugs has to be refactored to not rely on static stuff or at least provide some means to reset all static stuff, or the isolation for the SpotBugs runner has to be increased, so that each SpotBugs run is done in its own forked JVM and not in reused worker JVMs. This will probably make the analysis a bit slower, but imho it is worth the wait if you in turn get more reliable, reproducable and expected results. The best would of course be the first option, to make SpotBugs properly work with different settings in the same JVM mutliple times, but as this is not a problem with "normal" SpotBugs usage per-se, I thought here might be the more appropriate place to report it.

Vampire commented 5 years ago

Actually with IsolationMode.CLASSLOADER, it works fine, as then the classes are in a separate class loader and so the static fields don't influence each other. I tried this with some breakpoint magic that that sets the isolation mode to CLASSLOADER after you have set it to PROCESS. This of course also implicitly sets the fork mode to NEVER, so you loose the ability to specify custom JVM args and the max heap size but instead would need to do this on the Gradle daemon itself and to debug the execution you would debug the Gradle run itself. But at least then the runs are independent from each other.

Actually without refactoring SpotBugs you would probably need an even tighter isolation mode that combines CLASSLOADER within PROCESS so that the stuff gets a separate classloader in the worker daemon.


A working workaround that forces separate worker processes to be spawned is this:

tasks.withType(SpotBugsTask) {
    doFirst {
        jvmArgs "-DworkerUniquifier=${UUID.randomUUID()}"
    }
}

As it runs in the execution phase, it does not influence the cache key for the build cache or the up-to-date check, but if the task will be run due to out-of-dateness and cache-miss, it will get its own exclusive worker process as on each invocation the JVM args will be different and so a new process needs to be spawned.

KengoTODA commented 5 years ago

100 should fix this issue, we'll release next patch version soon so please try it later.

Vampire commented 5 years ago

Ah, yes, I see you switch to have one worker process per run, that should fix it, yes. Sad that it only supports 5.0 though, I'm still on 4.10.2 on some projects, but I'll switch to 5+ soon anyway. :-)

Vampire commented 5 years ago

Hm, do you have any estimate when "soon" will be approximately? My workaround works, but it kills the system eventually. As Gradle does not know the worker is not needed anymore, the process stays idling around and eats up the system memory quite fast if you have a couple of SpotBugs tasks or execute them repeatedly. I guess with your fix this will not happen as Gradle will know the worker is not necessary anymore and hopefully kills it after the task.

oehme commented 5 years ago

I've commented on #100, I think it's not a good idea. Spotbugs should be fixed instead to not use static state.

KengoTODA commented 5 years ago

"soon" means right after #106 has been merged. I've requested @spotbugs/everyone to merge that.

Spotbugs should be fixed instead to not use static state.

Yes! It's my dearest for this several years and we're open for PR that realizes it.

shevek commented 5 years ago

Running spotbugs in separate child worker JVMs kills parallel builds on any high-core-count system. This is slightly related to https://github.com/gradle/gradle/issues/7047 but the real solution is to make spotbugs truly thread/instance-safe so we can run parallel builds internally.

jscancella commented 4 years ago

@Vampire now that this has been merged, it is still an issue? Please try using the latest version.

jscancella commented 3 years ago

please reopen if still an issue

Vampire commented 3 years ago

People cannot reopen tickets that you closed. Non-members can only reopen tickets they closed themselves. What did you mean with "this" in "now that this has been merged"? I don't see any associated PR or similar or in which version this could probably have been fixed.

jscancella commented 3 years ago

Ok, I can reopen if you want. The merge I was referring to was the one @KengoTODA mentioned above #106. Please try using the latest version of the plugin and see if you still have an issue. If you do please create a minimum project that reproduces the issue.

Vampire commented 3 years ago

I don't see how #106 should be related to this issue. confused

jscancella commented 3 years ago

Oops, sorry I meant #100 (https://github.com/spotbugs/spotbugs-gradle-plugin/issues/113#issuecomment-462732598)

Vampire commented 3 years ago

Ah, that one, I see. I didn't try it out but from a quick look at the sources, I'd say the problem is most probably there again. It seems the revert got reverted with some options added. And as it seems the default behavior is to use a worker with process isolation that will have the faulty behaviour again unless Spotbugs was fixed in the meantime to not depend on static state. The other options that are present now are to use the correct but slow sequential way that was reintroduced by #100 and the proper fix that I suggested long ago in #100 already, that the worker api is used and the workers start an external process. But it is implemented in a way that it only works in Gradle 6 and only if enabled explicitly. So the correct behavior is possible, it is just not enabled by default and not compatible with Gradle <6 the way it is implemented.

KengoTODA commented 2 years ago

In the beta branch, we launch a java process from worker for each SpotBugsTask. So I hope that the problem has been fixed; each SpotBugs process is isolated and nothing shared except for the filesystem.