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

Resource leak when using Maven plugin #559

Closed
J-N-K opened this issue Apr 27, 2020 · 20 comments · Fixed by #571
Closed

Resource leak when using Maven plugin #559

J-N-K opened this issue Apr 27, 2020 · 20 comments · Fixed by #571
Labels

Comments

@J-N-K
Copy link
Contributor

J-N-K commented Apr 27, 2020

Recently we started using spotless in openHAB (http://github.com/openhab/openhab-addons). We are currently not able to do a full build (around 260 sub-projects) with checks enabled due to resource issues. It seems that this does not depend on the OS as it shows on our Unix-based CI and also on local builds under Windows.

  • [3.6.1] maven version
  • [1.30.0] spotless version
  • [4.13.0] eclipse version

A first analysis can be found here: openhab/openhab-addons#7449 (comment). Looking at a heap dump it seems that the SpotlessCache is instantiated with a new FeatureClassloader each time a check is called but never cleared.

The pom including the configuration can be found here: https://github.com/openhab/openhab-addons/blob/2.5.x/pom.xml

@nedtwigg nedtwigg added the bug label Apr 28, 2020
@nedtwigg
Copy link
Member

Interesting, thanks for the detailed profiling! It should be the case that the classloaders are cached across projects, so there should only be a few created across the entire build. You get a big speedup from the JIT by reusing these classloaders as much as you can - for the gradle plugin we only ever clear them when someone runs clean, otherwise they get reused by the daemon.

the spotless Plugin created around 18 daemon threads per project

That is really surprising to me!! To my knowledge we don't manually spawn a single thread. My best guess at what's happening is:

  • maven spawns threads which it uses to call plugins from
  • spotless is doing something which prevents those threads from shutting down

Independent of that, I'm also suspicious that if a maven plugin declares a static variable, it might not be shared across maven subprojects. If not, we have nothing to gain from our cache, and can easily disable it for maven builds.

I know it's been a long time since you worked on this, but any thoughts @lutovich?

@wborn
Copy link

wborn commented Apr 28, 2020

I'm also suspicious that if a maven plugin declares a static variable, it might not be shared across maven subprojects. If not, we have nothing to gain from our cache, and can easily disable it for maven builds.

Maven will create different classloaders and reuse them depending on the (plugin) configuration used in Maven projects. In parallel Maven builds (-T 1C) it may also create several classloaders for the same configuration so threads can use them in parallel. So using static variables for caching and locking usually doesn't work well with Maven.

@lutovich
Copy link
Contributor

Hey!

Yeah, it feels like with parallel builds and multiple classloaders SpotlessCache.instance can be instantiated more than once and cause problems. I can investigate this issue closer to the end of this week if that's not too late :)

@J-N-K
Copy link
Contributor Author

J-N-K commented Apr 28, 2020

I can investigate this issue closer to the end of this week if that's not too late :)

Thanks alot. Since it took us several month to get to the point where we are now, a week or two doesn't matter. We can always apply the formatter to single projects only which is not a problem.

@lutovich
Copy link
Contributor

lutovich commented May 3, 2020

Seems like this problem is caused by the way SpotlessCache keys are created and is specific to Eclipse-based steps. Each key a serialized instance of EclipseBasedStepBuilder.State and includes a list of settings files. This list is set by every Ecpilse-based step using EclipseBasedStepBuilder#setPreferences(). Steps locate settings files using FileLocator. Inconveniently, FileLocator does not just return the same file for every Maven module (e.g. openhab_codestyle.xml). Instead, it copies the configured file into the target directory of every module and returns this copied temporary file. This makes all SpotlessCache keys constructed for every module unique and results in zero reuse of FeatureClassLoaders. For every Eclipse-based step the created FeatureClassLoader loads a new SpotlessEclipseFramework with its own org.eclipse.core.internal.jobs.JobManager thread pool and a bunch of other helper threads. It looks like the number of active threads increases with every processed module and eventually brings the whole build to a halt.

Running spotless with Google code formatter works fine and the problem manifests once I re-add at least one Eclipse-based formatter.

Here is my investigation branch https://github.com/lutovich/spotless/tree/fix-spotless-cache-leak with some printouts and a hack that makes Eclipse Java formatter step pass when building openhab-addons. The hack is a very dirty one and a real fix would require some more effort.

An idea for a fix: instead of using the "physical" settings files for cache keys use "logical" files. For example, every cache key for Eclipse Java format step would use the configured openhab_codestyle.xml instead of a copied temporary per-module XML file.

Any ideas or thoughts about this are greatly appreciated!

@nedtwigg
Copy link
Member

nedtwigg commented May 4, 2020

Aha! We had a similar problem (very different symptom though) when adding tsfmt support to maven-spotless (#553). The problem there is that package.json and tsconfig.json need to be in the project root, and we expected them to be there, but they were getting copied into a temp directory with a random name. We fixed it by adding public File locateLocal(String path), which doesn't copy the file, to go along with public File locateFile(String path), which does copy the file.

https://github.com/diffplug/spotless/pull/553/files#diff-fb2ef4a51d4ea619ed63830522709278

I see now in my notes that I left myself a todo, which was:

  • I don't see why we can't just delete File locateFile(String path), which copies the file
  • and replace every invocation of it with File locateLocal(String path) which doesn't copy it
  • but there must be some reason we're copying, so I should ask first

But it never got to the top of my todo :). So whaddya think - can we replace every call to locateFile with locateLocal? Why not? Fwiw, the gradle plugin never copies settings files.

@lutovich
Copy link
Contributor

lutovich commented May 4, 2020

FileLocator#locateFile() is quite powerful and can load the given file from the local file system, URL, or extract from a JAR file. I think JAR file could be especially useful for multi-module Maven projects where Spotless settings file lives in a dedicated module. URL could be useful when the settings file lives on GitHub, even though it is probably a bit weird to download setting files during every build.

Spotbugs and Checkstyle Maven plugins seem to also use ResourceManager for loading configuration files:

I remember looking at them for inspiration :)

Do you think it is possible to use actual file content in FileSignature instead of the combination of file names, sizes, and last modified timestamps? POC commit link. I think this could solve the given issue, though I haven't verified it :)

@lutovich
Copy link
Contributor

lutovich commented May 4, 2020

Added another commit to that branch to handle hashing for files and directories differently: diff. Input is much appreciated!

@nedtwigg
Copy link
Member

nedtwigg commented May 4, 2020

We can definitely change FileSignature. We actually have an open issue to make it machine-independent for the gradle build cache (no absolute paths or file timestamps) #566

With that in mind, a few points on the diff:

  1. The FileSignature constructor does care about order (e.g. classpath order matters, thus signAsList vs signAsSet, as you noted)
  2. FileSignature.signAsSet uses the natural ordering of File, which sorts on path. If the files are going to have random filenames, then we need to sort on their hash or something else.
  3. Copying resources to build dir is okay, but can they at least have the same filename? Debugging is easier if the filenames stay human-readable. Also would allow us to sort by filename rather than hash.

@lutovich
Copy link
Contributor

Making Maven plugin use human-readable names for output files would be nice. I'm not sure how to do this nicely though. Creating a name for the output file is easy when a step is configured with a local file:

<eclipse>
   <file>${basedir}/eclipse-fmt.xml</file>
</eclipse>

then we could just name the output file eclipse-fmt.xml.

However, it could be a bit tricky when a step is configured with a URL:

<eclipse>
   <file>http://my-site.com/eclipse-fmt.xml</file>
</eclipse>

where URLs might have redirects or query params.

I might be overthinking it, any ideas are appreciated!

Please take a look at the attached PR. It makes output file names predictable but not human-readable. Thus it is possible to sort on file names. This PR makes it possible to run mvn spotless:apply -T 2C locally on openhab-addons project and the number of threads remains constant.

@nedtwigg
Copy link
Member

nedtwigg commented Jun 5, 2020

We still don't have a root fix for you. However, we do have an unfinished PR which would apply Spotless only to files which have changed since some git reference (e.g. origin/master). I don't have time to finish it, but if anyone wants the feature and has a few spare cycles #603 is the WIP.

@nedtwigg
Copy link
Member

nedtwigg commented Jul 2, 2020

We just published 2.0.0. This release should fix all of the problems identified here, please let us know how it works for you. The breaking change is just about removing old deprecated features, shouldn't affect you.

@J-N-K
Copy link
Contributor Author

J-N-K commented Jul 2, 2020

That‘s good news. I‘ll check in the next days. Thanks for your support.

@nedtwigg
Copy link
Member

nedtwigg commented Jul 2, 2020

Also, @lutovich did all the work to make this happen, I just pressed the "release" button.

@J-N-K
Copy link
Contributor Author

J-N-K commented Jul 3, 2020

Unfortunately it doesn't work. If I run mvn spotless:apply in the root of the project (Win64, Zulu Java 11), spotless fails:

[ERROR] Failed to execute goal com.diffplug.spotless:spotless-maven-plugin:2.0.0:apply (default-cli) on project org.openhab.addons.reactor.bundles: Execution default-cli of goal com.diffplug.spotless:spotless-maven-plugin:2.0.0:apply failed: Expected 'C:\Users\Jan\Documents\openhab-master\git\openhab2-addons\bundles\pom.xml' to start with 'C:\Users\Jan\Documents\openhab-master\git\openhab2-addons\bundles/' -> [Help 1]
org.apache.maven.lifecycle.LifecycleExecutionException: Failed to execute goal com.diffplug.spotless:spotless-maven-plugin:2.0.0:apply (default-cli) on project org.openhab.addons.reactor.bundles: Execution default-cli of goal com.diffplug.spotless:spotless-maven-plugin:2.0.0:apply failed: Expected 'C:\Users\Jan\Documents\openhab-master\git\openhab2-addons\bundles\pom.xml' to start with 'C:\Users\Jan\Documents\openhab-master\git\openhab2-addons\bundles/'
    at org.apache.maven.lifecycle.internal.MojoExecutor.execute (MojoExecutor.java:215)
    at org.apache.maven.lifecycle.internal.MojoExecutor.execute (MojoExecutor.java:156)
    at org.apache.maven.lifecycle.internal.MojoExecutor.execute (MojoExecutor.java:148)
    at org.apache.maven.lifecycle.internal.LifecycleModuleBuilder.buildProject (LifecycleModuleBuilder.java:117)
    at org.apache.maven.lifecycle.internal.LifecycleModuleBuilder.buildProject (LifecycleModuleBuilder.java:81)
    at org.apache.maven.lifecycle.internal.builder.singlethreaded.SingleThreadedBuilder.build (SingleThreadedBuilder.java:56)
    at org.apache.maven.lifecycle.internal.LifecycleStarter.execute (LifecycleStarter.java:128)
    at org.apache.maven.DefaultMaven.doExecute (DefaultMaven.java:305)
    at org.apache.maven.DefaultMaven.doExecute (DefaultMaven.java:192)
    at org.apache.maven.DefaultMaven.execute (DefaultMaven.java:105)
    at org.apache.maven.cli.MavenCli.execute (MavenCli.java:956)
    at org.apache.maven.cli.MavenCli.doMain (MavenCli.java:288)
    at org.apache.maven.cli.MavenCli.main (MavenCli.java:192)
    at jdk.internal.reflect.NativeMethodAccessorImpl.invoke0 (Native Method)
    at jdk.internal.reflect.NativeMethodAccessorImpl.invoke (NativeMethodAccessorImpl.java:62)
    at jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke (DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke (Method.java:566)
    at org.codehaus.plexus.classworlds.launcher.Launcher.launchEnhanced (Launcher.java:282)
    at org.codehaus.plexus.classworlds.launcher.Launcher.launch (Launcher.java:225)
    at org.codehaus.plexus.classworlds.launcher.Launcher.mainWithExitCode (Launcher.java:406)
    at org.codehaus.plexus.classworlds.launcher.Launcher.main (Launcher.java:347)
Caused by: org.apache.maven.plugin.PluginExecutionException: Execution default-cli of goal com.diffplug.spotless:spotless-maven-plugin:2.0.0:apply failed: Expected 'C:\Users\Jan\Documents\openhab-master\git\openhab2-addons\bundles\pom.xml' to start with 'C:\Users\Jan\Documents\openhab-master\git\openhab2-addons\bundles/'
    at org.apache.maven.plugin.DefaultBuildPluginManager.executeMojo (DefaultBuildPluginManager.java:148)
    at org.apache.maven.lifecycle.internal.MojoExecutor.execute (MojoExecutor.java:210)
    at org.apache.maven.lifecycle.internal.MojoExecutor.execute (MojoExecutor.java:156)
    at org.apache.maven.lifecycle.internal.MojoExecutor.execute (MojoExecutor.java:148)
    at org.apache.maven.lifecycle.internal.LifecycleModuleBuilder.buildProject (LifecycleModuleBuilder.java:117)
    at org.apache.maven.lifecycle.internal.LifecycleModuleBuilder.buildProject (LifecycleModuleBuilder.java:81)
    at org.apache.maven.lifecycle.internal.builder.singlethreaded.SingleThreadedBuilder.build (SingleThreadedBuilder.java:56)
    at org.apache.maven.lifecycle.internal.LifecycleStarter.execute (LifecycleStarter.java:128)
    at org.apache.maven.DefaultMaven.doExecute (DefaultMaven.java:305)
    at org.apache.maven.DefaultMaven.doExecute (DefaultMaven.java:192)
    at org.apache.maven.DefaultMaven.execute (DefaultMaven.java:105)
    at org.apache.maven.cli.MavenCli.execute (MavenCli.java:956)
    at org.apache.maven.cli.MavenCli.doMain (MavenCli.java:288)
    at org.apache.maven.cli.MavenCli.main (MavenCli.java:192)
    at jdk.internal.reflect.NativeMethodAccessorImpl.invoke0 (Native Method)
    at jdk.internal.reflect.NativeMethodAccessorImpl.invoke (NativeMethodAccessorImpl.java:62)
    at jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke (DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke (Method.java:566)
    at org.codehaus.plexus.classworlds.launcher.Launcher.launchEnhanced (Launcher.java:282)
    at org.codehaus.plexus.classworlds.launcher.Launcher.launch (Launcher.java:225)
    at org.codehaus.plexus.classworlds.launcher.Launcher.mainWithExitCode (Launcher.java:406)
    at org.codehaus.plexus.classworlds.launcher.Launcher.main (Launcher.java:347)
Caused by: java.lang.IllegalArgumentException: Expected 'C:\Users\Jan\Documents\openhab-master\git\openhab2-addons\bundles\pom.xml' to start with 'C:\Users\Jan\Documents\openhab-master\git\openhab2-addons\bundles/'
    at com.diffplug.spotless.FileSignature.subpath (FileSignature.java:195)
    at com.diffplug.spotless.extra.GitAttributesLineEndings$CachedEndings.endingFor (GitAttributesLineEndings.java:125)
    at com.diffplug.spotless.extra.GitAttributesLineEndings$RelocatablePolicy.getEndingFor (GitAttributesLineEndings.java:95)
    at com.diffplug.spotless.Formatter.computeLineEndings (Formatter.java:211)
    at com.diffplug.spotless.PaddedCell.calculateDirtyState (PaddedCell.java:203)
    at com.diffplug.spotless.PaddedCell.calculateDirtyState (PaddedCell.java:188)
    at com.diffplug.spotless.maven.SpotlessApplyMojo.process (SpotlessApplyMojo.java:45)
    at com.diffplug.spotless.maven.AbstractSpotlessMojo.execute (AbstractSpotlessMojo.java:142)
    at com.diffplug.spotless.maven.AbstractSpotlessMojo.execute (AbstractSpotlessMojo.java:127)
    at org.apache.maven.plugin.DefaultBuildPluginManager.executeMojo (DefaultBuildPluginManager.java:137)
    at org.apache.maven.lifecycle.internal.MojoExecutor.execute (MojoExecutor.java:210)
    at org.apache.maven.lifecycle.internal.MojoExecutor.execute (MojoExecutor.java:156)
    at org.apache.maven.lifecycle.internal.MojoExecutor.execute (MojoExecutor.java:148)
    at org.apache.maven.lifecycle.internal.LifecycleModuleBuilder.buildProject (LifecycleModuleBuilder.java:117)
    at org.apache.maven.lifecycle.internal.LifecycleModuleBuilder.buildProject (LifecycleModuleBuilder.java:81)
    at org.apache.maven.lifecycle.internal.builder.singlethreaded.SingleThreadedBuilder.build (SingleThreadedBuilder.java:56)
    at org.apache.maven.lifecycle.internal.LifecycleStarter.execute (LifecycleStarter.java:128)
    at org.apache.maven.DefaultMaven.doExecute (DefaultMaven.java:305)
    at org.apache.maven.DefaultMaven.doExecute (DefaultMaven.java:192)
    at org.apache.maven.DefaultMaven.execute (DefaultMaven.java:105)
    at org.apache.maven.cli.MavenCli.execute (MavenCli.java:956)
    at org.apache.maven.cli.MavenCli.doMain (MavenCli.java:288)
    at org.apache.maven.cli.MavenCli.main (MavenCli.java:192)
    at jdk.internal.reflect.NativeMethodAccessorImpl.invoke0 (Native Method)
    at jdk.internal.reflect.NativeMethodAccessorImpl.invoke (NativeMethodAccessorImpl.java:62)
    at jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke (DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke (Method.java:566)
    at org.codehaus.plexus.classworlds.launcher.Launcher.launchEnhanced (Launcher.java:282)
    at org.codehaus.plexus.classworlds.launcher.Launcher.launch (Launcher.java:225)
    at org.codehaus.plexus.classworlds.launcher.Launcher.mainWithExitCode (Launcher.java:406)
    at org.codehaus.plexus.classworlds.launcher.Launcher.main (Launcher.java:347)

The interesting thing is: If i change to bundles\ and run the same command, it works. I have not been able to figure out what causes this, as mvn help:system correctly reports crlf as lineending (and core.eol is also set to crlf), so LineEnding.nativeIsWin() should in any case be true and the \ should be replaced by /, but it seems that is not the case.

@lutovich
Copy link
Contributor

lutovich commented Jul 3, 2020

@J-N-K sorry about this! Unfortunately, I do not have access to a Windows machine at the moment. You are right, the code looks like it should replace all separators with Unix-style separators.

Do you think you could try to debug this a bit with jdb to investigate a bit more? It is indeed curios what does LineEnding.nativeIsWin() return.

Maybe you could in one shell run:

> mvnDebug spotless:check

and in another shell run:

> jdb -attach 8000
> stop at com.diffplug.spotless.FileSignature:195
> run
// it should now have a breakpoint set and eventually hit it and print:
// Breakpoint hit: "thread=main", com.diffplug.spotless.FileSignature.subpath(), line=195 bci=0
// now it's possible to evaluate and print expressions
> print com.diffplug.spotless.LineEnding.nativeIsWin()
> print com.diffplug.spotless.FileSignature.pathNativeToUnix(root)
> print com.diffplug.spotless.FileSignature.pathNativeToUnix(child)
> print child.startsWith(root)

I tried this debugging locally on a different line in FileSignature and this is the output:

jdb -attach 8000
Set uncaught java.lang.Throwable
Set deferred uncaught java.lang.Throwable
Initializing jdb ...
>
VM Started: No frames on the current call stack

main[1] stop at com.diffplug.spotless.FileSignature:192
Deferring breakpoint com.diffplug.spotless.FileSignature:192.
It will be set after the class is loaded.
main[1] run
> Set deferred breakpoint com.diffplug.spotless.FileSignature:192

Breakpoint hit: "thread=main", com.diffplug.spotless.FileSignature.subpath(), line=192 bci=0

main[1] print com.diffplug.spotless.LineEnding.nativeIsWin()
 com.diffplug.spotless.LineEnding.nativeIsWin() = false
main[1] print com.diffplug.spotless.FileSignature.pathNativeToUnix(root)
 com.diffplug.spotless.FileSignature.pathNativeToUnix(root) = "/Users/lutovich/Projects/openhab-addons/"
main[1] print com.diffplug.spotless.FileSignature.pathNativeToUnix(child)
 com.diffplug.spotless.FileSignature.pathNativeToUnix(child) = "/Users/lutovich/Projects/openhab-addons/pom.xml"
main[1] print child.startsWith(root)
 child.startsWith(root) = true
main[1]

@nedtwigg
Copy link
Member

nedtwigg commented Jul 3, 2020

Would be very interesting to have that debug info. I just pushed up a possible solution in #639, which exposes a surprising test failure. We run our unit tests on windows & unix, but it appears there was a windows-only line-endings regression in 2.0 that went untested. I will investigate...

@J-N-K
Copy link
Contributor Author

J-N-K commented Jul 4, 2020

I didn't manage to use jdb from the command line, but used the IntelliJ remote debugging instead.

LineEnding.nativeIsWin() indeed returns false and LineEnding._platformNative is \n.

Additionally I checked that on the first project in that build (the previous one is 6/277) and there

LineEnding.nativeIsWin() returns true and LineEnding._platformNative is \r\n.

I digged a little bit deeper. And if I disable the karaf-maven-plugin, everything works. So it seems that either our configuration needs to be adapted or that plugin changes the line-ending property of System. Which would be very bad IMO.

@J-N-K
Copy link
Contributor Author

J-N-K commented Jul 4, 2020

In fact, it‘s not even the karaf-maven-plugin. It only happens if the extensions of that plugin are enabled. Could be as well a maven issue. Closing here since the leaks are fixed. Thanks again for your support, @nedtwigg and @lutovich

@J-N-K J-N-K closed this as completed Jul 4, 2020
@nedtwigg
Copy link
Member

nedtwigg commented Jul 5, 2020

We've released a fix in plugin-maven 2.0.1 and plugin-gradle 4.5.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants