Revert 58 to fix spotbugs/spotbugs!819 #100

KengoTODA · 2019-01-28T06:13:17Z

This PR solves a reported problem at the core project.
To avoid potential issues, it also bumps up supported Gradle version from 4.0 to 5.0.

This reverts commit a0ceea6. It seems that this change introduced a multi-threading problem reported at spotbugs/spotbugs#819 so I revert that commit to make gradle plugin stable.

this is necessary because worker process needs them to run SpotBugs which depends on SLF4J.

KengoTODA · 2019-02-10T07:05:43Z

@spotbugs/everyone could you review these changes? thank you!

src/main/java/com/github/spotbugs/internal/spotbugs/SpotBugsExecuter.java

ThrawnCA · 2019-02-10T23:09:05Z

build.gradle

@@ -14,6 +14,7 @@ apply from: "$rootDir/gradle/deploy.gradle"
 version = '1.6.10-SNAPSHOT'
 group = "com.github.spotbugs"
 def spotBugsVersion = '3.1.11'
+def slf4jVersion = '1.8.0-beta2'


I think we need to seriously consider dropping SLF4J and just using JUL, for maximum compatibility.

If you mean the trouble around gradle plugin, I think all the reported issues can be solved if we stop depending on Gradle internal API that excludes SLF4J from the dependency. This PR also solves a part of them by bumping up minimum supported version to 5.0. So I still think that SLF4J is better than that horrible JUL...

+1 for JUL.. it just works.

KengoTODA · 2019-02-11T03:25:55Z

Updated, please review this PR again @ThrawnCA

oehme · 2019-02-13T12:57:38Z

Why revert using the worker API? Now you depend on internal APIs again, so your plugin could break at any time. Also, using a single request worker for every task will make your plugin much, much slower. Shouldn't instead the usage of static state be removed?

DPUkyle · 2019-02-13T17:12:27Z

I agree with @oehme; using Gradle internal APIs is a step backwards.

If the spotbugs tool can not be quickly remedied to avoid preservation of static state, I suggest that we preserve the public API of SpotBugsTask while avoiding the Worker API or internal APIs. Instead, the @TaskAction method will invoke Project#javaexec(Action); that spawned JVM will then die at the conclusion of the task execution. While this isn't ideal from a performance POV, it should prevent the static state issues identified by @Vampire.

KengoTODA · 2019-02-14T10:24:17Z

I also believe that it's mandatory to remove dependency on Gradle internal API. This is why I made #104 recently.

At the same time, we need to make this product working. The reverted code in this PR introduced a trouble. To make it stable asap, I merged this PR after the user test.

Let's find the way to realize stable feature in both of feature and compatibility :)
I believe it's possible even though SpotBugs itself is thread unsafe.

KengoTODA · 2019-02-14T10:25:53Z

JavaExec mentioned by DPUkyle seems nice. It will be nice option to solve this issue.

Vampire · 2019-02-14T22:38:40Z

Maybe even a combination of worker api and javaExec could make sense.
Because by using the worker api, tasks are done in parallel up to max-workers amount.
The worker implementation could then spawn a separate process.
This way you have the parallelity and still the isolation.
With the current solution I think the tasks would again be done sequentially only, as would be by only doing a javaExec.

davidburstromspotify · 2019-02-20T08:14:48Z

I must admit I haven't looked closely at the holistic issue, but what's problematic about dealing with the printout or dual registration at the point it happens? I don't see a dysfunction in the checkers where they themselves keep static state during analysis.

Vampire · 2019-02-20T09:03:44Z

The problem is the same why in 80% of the cases when you do an invokeLater in Swing to fix a bug you even make it worse.
It is symptom treatment, not bug fixing and you just shift the problem to a less obvious and harder to solve place.
I don't know of whether there are other static things that are already broken in more subtle ways, but speaking of the plugins, imagine two projects A and B that both run with the same Gradle version and thus on the same daemon and the Spotbugs task will run on the same worker (e. g. wih --max-workers 1).
Project A uses the find-sec-bugs plugin, Project B does not.
Now you analyze project B and you get no bugs found.
Next you analyze project A, not warnings, no bugs found.
Now you analyze project B again without any change and (even without warning as there is no double registration) suddenly you get issues found.

Or you analyze A and B with different version of the same plugin, and so on.

davidburstromspotify · 2019-02-20T10:55:14Z

@Vampire I totally agree with that assessment. What I'm meaning is that instead of printing a warning for every time a factory is re-registered, why not be pragmatic and warn only once? That doesn't hide the symptom, and it doesn't spam the logs, and you'll get the speedup associated with the worker API.

For sure, there could be cases where your isolation is broken. Is that an issue that people are currently struggling with, or is it more of a theoretical problem at this point?

Vampire · 2019-02-20T11:45:18Z

What do you gain with speedup if you get wrong results? o_O

Besides that, if he follows my suggestion to combine worker-api and javaExec, you are just a bit slower as you need to start a VM each time, but it will still be done in parallel.

davidburstromspotify · 2019-02-20T13:43:56Z

@Vampire If people actually do get wrong results, it's not a gain, agreed. In our projects we run the same extensions for SpotBugs for all (hundreds of) modules, so the execution time problem is much more important to solve. The speedup with the worker API and in daemon processing was substantial, reducing the execution time with more than 75%. That being said, we can perform tests with your proposed solution and see if it performs well enough. If it is at least 50% faster than the baseline I'd consider it a success from our point of view, otherwise not.

Vampire · 2019-02-20T14:50:53Z

It was not in-daemon with the worker API.
The isolation level was set to PROCESS, so the tasks are done in separate worker processes, not in the daemon process.
Those were reused though.
I'd guess the most speed gain was because of the parallel execution, so it might speed up enough for you with my proposal, but I might be wrong of course.

davidburstromspotify · 2019-02-20T15:00:19Z

You're right, I was under the wrong impression with how the worker API was used.

I'd be happy to test the proposal, so we can know for sure.

ctmay4 · 2019-03-18T12:35:22Z

I'd like to add to what @davidburstromspotify said that the slowdown is a big deal for larger projects. We had to roll back to a prior version (1.6.9) of the plugin because it made such a huge difference in our in CI builds.

KengoTODA · 2019-03-18T13:26:40Z

Feel free to share your idea to solve this problem via PR! :)

KengoTODA · 2019-04-20T07:24:24Z

note: It is possible to launch SpotBugs by project.javaexec, however it cannot create SpotBugsResult instance because we cannot touch IFindBugsEngine instance after execution.

      ExecResult result = project.javaexec(new Action<JavaExecSpec>() {
		@Override
		public void execute(JavaExecSpec javaSpec) {
			javaSpec.args(spec.getArguments());
			javaSpec.setMain("edu.umd.cs.findbugs.LaunchAppropriateUI");
		}
      });

To keep current feature, we need to avoid using JavaExec, then probably need to hack a classloader?

KengoTODA added 4 commits January 9, 2019 14:55

Revert "Fix #57. Implement Worker API support."

112407e

This reverts commit a0ceea6. It seems that this change introduced a multi-threading problem reported at spotbugs/spotbugs#819 so I revert that commit to make gradle plugin stable.

add SLF4J API and bridge into spotbugsClasspath

6e9bcd9

this is necessary because worker process needs them to run SpotBugs which depends on SLF4J.

Require Gradle 5 to avoid SLF4J related problem

040d91f

add entries into CHANGELOG

4d41992

KengoTODA self-assigned this Jan 28, 2019

KengoTODA and others added 2 commits January 28, 2019 14:14

Merge branch 'master' into revert-58

9347e93

add missing javadoc introduced at issue 58

b99526b

ThrawnCA previously approved these changes Feb 10, 2019

View reviewed changes

KengoTODA added 3 commits February 11, 2019 10:48

rename Executer to Executor

5be0e04

Merge remote-tracking branch 'origin/master' into revert-58

b5f7360

drop needless code to backward compatibility for Gradle < 4.7

067a55c

KengoTODA dismissed ThrawnCA’s stale review via 067a55c February 11, 2019 02:53

KengoTODA mentioned this pull request Feb 11, 2019

Gradle plugin is using a deprecated feature to be removed in Gradle 6.0 #105

Closed

henrik242 approved these changes Feb 11, 2019

View reviewed changes

ThrawnCA approved these changes Feb 11, 2019

View reviewed changes

KengoTODA merged commit 1fa592c into master Feb 11, 2019

KengoTODA deleted the revert-58 branch February 11, 2019 07:03

KengoTODA mentioned this pull request Feb 12, 2019

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

Open

KengoTODA added this to the 1.6.10 milestone Feb 14, 2019

KengoTODA mentioned this pull request Feb 20, 2019

Gradle-daemon: Trying to add already registered factory fills stdout spotbugs/spotbugs#819

Closed

KengoTODA mentioned this pull request May 27, 2019

Usage of workers is killing parallel builds #149

Closed

DPUkyle mentioned this pull request Nov 4, 2019

Spotbugs 2.0 breaks if Guava is on the plugin classpath #166

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revert 58 to fix spotbugs/spotbugs!819 #100

Revert 58 to fix spotbugs/spotbugs!819 #100

KengoTODA commented Jan 28, 2019

KengoTODA commented Feb 10, 2019

ThrawnCA Feb 10, 2019

KengoTODA Feb 11, 2019 •

edited

Loading

swpalmer Mar 14, 2019

KengoTODA commented Feb 11, 2019

oehme commented Feb 13, 2019 •

edited

Loading

DPUkyle commented Feb 13, 2019

KengoTODA commented Feb 14, 2019 •

edited

Loading

KengoTODA commented Feb 14, 2019 •

edited

Loading

Vampire commented Feb 14, 2019

davidburstromspotify commented Feb 20, 2019

Vampire commented Feb 20, 2019

davidburstromspotify commented Feb 20, 2019 •

edited

Loading

Vampire commented Feb 20, 2019

davidburstromspotify commented Feb 20, 2019

Vampire commented Feb 20, 2019

davidburstromspotify commented Feb 20, 2019

ctmay4 commented Mar 18, 2019

KengoTODA commented Mar 18, 2019

KengoTODA commented Apr 20, 2019

Revert 58 to fix spotbugs/spotbugs!819 #100

Revert 58 to fix spotbugs/spotbugs!819 #100

Conversation

KengoTODA commented Jan 28, 2019

KengoTODA commented Feb 10, 2019

ThrawnCA Feb 10, 2019

Choose a reason for hiding this comment

KengoTODA Feb 11, 2019 • edited Loading

Choose a reason for hiding this comment

swpalmer Mar 14, 2019

Choose a reason for hiding this comment

KengoTODA commented Feb 11, 2019

oehme commented Feb 13, 2019 • edited Loading

DPUkyle commented Feb 13, 2019

KengoTODA commented Feb 14, 2019 • edited Loading

KengoTODA commented Feb 14, 2019 • edited Loading

Vampire commented Feb 14, 2019

davidburstromspotify commented Feb 20, 2019

Vampire commented Feb 20, 2019

davidburstromspotify commented Feb 20, 2019 • edited Loading

Vampire commented Feb 20, 2019

davidburstromspotify commented Feb 20, 2019

Vampire commented Feb 20, 2019

davidburstromspotify commented Feb 20, 2019

ctmay4 commented Mar 18, 2019

KengoTODA commented Mar 18, 2019

KengoTODA commented Apr 20, 2019

KengoTODA Feb 11, 2019 •

edited

Loading

oehme commented Feb 13, 2019 •

edited

Loading

KengoTODA commented Feb 14, 2019 •

edited

Loading

KengoTODA commented Feb 14, 2019 •

edited

Loading

davidburstromspotify commented Feb 20, 2019 •

edited

Loading