Formatting can fail depending on the number of input files

[asapha]

🐛 Describe the bug

Running ktfmtFormatMain on multiple files fails.

⚠️ Current behavior

The task manages to format a few files, then it hangs with error messages such as [ktfmt] e: Generic error during file processing or [ktfmt] Failed to analyse:

e.g:

[ktfmt] Reformatting...: /private/var/folders/8d/j0hhhkpn2_1_zvd92bxhz2300000gn/T/junit13498793146131357027/src/main/java/TestFile3.kt
[ktfmt] Reformatting...: /private/var/folders/8d/j0hhhkpn2_1_zvd92bxhz2300000gn/T/junit13498793146131357027/src/main/java/TestFile16.kt
[ktfmt] Reformatting...: /private/var/folders/8d/j0hhhkpn2_1_zvd92bxhz2300000gn/T/junit13498793146131357027/src/main/java/TestFile22.kt
[ktfmt] Reformatting...: /private/var/folders/8d/j0hhhkpn2_1_zvd92bxhz2300000gn/T/junit13498793146131357027/src/main/java/TestFile12.kt
[ktfmt] Reformatting...: /private/var/folders/8d/j0hhhkpn2_1_zvd92bxhz2300000gn/T/junit13498793146131357027/src/main/java/TestFile13.kt
[ktfmt] Reformatting...: /private/var/folders/8d/j0hhhkpn2_1_zvd92bxhz2300000gn/T/junit13498793146131357027/src/main/java/TestFile6.kt
[ktfmt] Reformatting...: /private/var/folders/8d/j0hhhkpn2_1_zvd92bxhz2300000gn/T/junit13498793146131357027/src/main/java/TestFile7.kt
[ktfmt] Reformatting...: /private/var/folders/8d/j0hhhkpn2_1_zvd92bxhz2300000gn/T/junit13498793146131357027/src/main/java/TestFile26.kt
[ktfmt] Failed to analyse: /private/var/folders/8d/j0hhhkpn2_1_zvd92bxhz2300000gn/T/junit13498793146131357027/src/main/java/TestFile29.kt
[ktfmt] Failed to analyse: /private/var/folders/8d/j0hhhkpn2_1_zvd92bxhz2300000gn/T/junit13498793146131357027/src/main/java/TestFile19.kt
[ktfmt] e: Generic error during file processing
[ktfmt] e: Generic error during file processing
[ktfmt] Failed to analyse: /private/var/folders/8d/j0hhhkpn2_1_zvd92bxhz2300000gn/T/junit13498793146131357027/src/main/java/TestFile23.kt
[ktfmt] e: Generic error during file processing
[ktfmt] Failed to analyse: /private/var/folders/8d/j0hhhkpn2_1_zvd92bxhz2300000gn/T/junit13498793146131357027/src/main/java/TestFile17.kt
[ktfmt] e: Generic error during file processing
[ktfmt] Failed to analyse: /private/var/folders/8d/j0hhhkpn2_1_zvd92bxhz2300000gn/T/junit13498793146131357027/src/main/java/TestFile18.kt
[ktfmt] e: Generic error during file processing

💣 Steps to reproduce

Run this test in KtfmtFormatTaskIntegrationTest

    @ParameterizedTest
    @ValueSource(ints = [10, 15, 30, 50, 100, 1000])
    fun `format task can format multiple files`(n: Int) {
        repeat(n) { index ->
            createTempFile(content = "val answer${index}=42\n", fileName = "TestFile$index.kt")
        }
        val result =
            GradleRunner.create()
                .withProjectDir(tempDir)
                .withPluginClasspath()
                .withArguments("ktfmtFormatMain", "--info")
                .forwardOutput()
                .build()

        assertThat(result.task(":ktfmtFormatMain")?.outcome).isEqualTo(SUCCESS)
    }

It fails when n is as low as 30 on my machine.

📱 Tech info

• git rev-parse HEAD -> e8f927180ffc388978c9e663fb4f35fcdeba19f2
• openjdk 20.0.2 2023-07-18
• macOS Version 13.6

When upgrading to 0.14, I can no longer run ktfmt on my whole codebase.

[simonhauck]

I can reproduce this issue in our project. With 0.0.13 the tasks finishes as expected.

[njuro]

Can confirm this issue. Getting java.lang.OutOfMemoryError: Compressed class space when running ktfmtFormat over the whole project.

[simonhauck]

We also noticed, that the formatting can now take minutes and consumes a lot of RAM. Sometimes around 10GB

[cortinico]

Is this happening in 0.14.0 only? As if so this is most likely the culprit:

• #181 - move ktfmt to use Worker API for classloader isolation #182

If you confirm that this is not happening in 0.13, I'm leaning towards releasing 0.14.1 with the revert of #182 and get back to it in the future

[simonhauck]

Yes, from my testing this is only happening in 0.0.14.
I tried to investigate it further but was not able to narrow the issue down. I am also not so familiar with the isolation classloader feature.

I think the general implementation offers some nice advantages. Especially that it is now possible to set the version of the formatter manually by the plugin version.

Is it possible to keep this or do you think it is better to completly revert the commit?

Edit:
One last thing that came to my mind. Maybe its worth trying to use the 0.0.14 version with a different version of the formatter. So not 0.0.46 and instead take 0.0.45 or something similar. Just to rule out that it is not a ktfmt issue.

[cortinico]

@simonhauck one thing that would really be helpful would be to provide a reproducer.
Potentially by using this template https://github.com/cortinico/kotlin-android-template
If you could create a repo with say 10000 Kotlin files that showcase how ktfmt-gradle fails there I can take a look at it

[asapha]

one thing that would really be helpful would be to provide a reproducer.

Isn't the integration test above sufficient? It fails way before 10k files.

Some notes after poking around:

• The "Generic error during file processing" message hides an important error (see below)
• The ktfmt project warns of a memory leak in its Parser class. (YouTrack issue)
• (A Gradle issue about OOM when using classloader isolation.)
• Using process isolation instead makes the test pass

Generic error during file processing: java.lang.OutOfMemoryError: Java heap space
	at jdk.zipfs/jdk.nio.zipfs.ZipFileSystem.initCEN(ZipFileSystem.java:1559)
	at jdk.zipfs/jdk.nio.zipfs.ZipFileSystem.<init>(ZipFileSystem.java:179)
	at jdk.zipfs/jdk.nio.zipfs.ZipFileSystemProvider.getZipFileSystem(ZipFileSystemProvider.java:125)
	at jdk.zipfs/jdk.nio.zipfs.ZipFileSystemProvider.newFileSystem(ZipFileSystemProvider.java:120)
	at java.base/java.nio.file.FileSystems.newFileSystem(FileSystems.java:528)
	at java.base/java.nio.file.FileSystems.newFileSystem(FileSystems.java:400)
	at org.jetbrains.kotlin.com.intellij.ide.plugins.DescriptorLoadingContext.open(DescriptorLoadingContext.java:40)
	at org.jetbrains.kotlin.com.intellij.ide.plugins.PluginDescriptorLoader.loadDescriptorFromJar(PluginDescriptorLoader.java:84)
	at org.jetbrains.kotlin.com.intellij.ide.plugins.PluginManagerCore.registerExtensionPointAndExtensions(PluginManagerCore.java:1325)
	at org.jetbrains.kotlin.com.intellij.core.CoreApplicationEnvironment.registerExtensionPointAndExtensions(CoreApplicationEnvironment.java:287)
	at org.jetbrains.kotlin.cli.jvm.compiler.KotlinCoreEnvironment$Companion.registerApplicationExtensionPointsAndExtensionsFrom(KotlinCoreEnvironment.kt:625)
	at org.jetbrains.kotlin.cli.jvm.compiler.KotlinCoreEnvironment$Companion.createApplicationEnvironment(KotlinCoreEnvironment.kt:595)
	at org.jetbrains.kotlin.cli.jvm.compiler.KotlinCoreEnvironment$Companion.getOrCreateApplicationEnvironment(KotlinCoreEnvironment.kt:505)
	at org.jetbrains.kotlin.cli.jvm.compiler.KotlinCoreEnvironment$Companion.getOrCreateApplicationEnvironmentForProduction(KotlinCoreEnvironment.kt:492)
	at org.jetbrains.kotlin.cli.jvm.compiler.KotlinCoreEnvironment$Companion.createForProduction(KotlinCoreEnvironment.kt:440)
	at com.facebook.ktfmt.format.Parser.<clinit>(Parser.kt:57)
	at com.facebook.ktfmt.format.Formatter.sortedAndDistinctImports(Formatter.kt:143)
	at com.facebook.ktfmt.format.Formatter.format(Formatter.kt:90)
	at com.ncorti.ktfmt.gradle.tasks.worker.KtfmtFormatter.format(KtfmtFormatter.kt:41)
	at com.ncorti.ktfmt.gradle.tasks.worker.KtfmtWorkAction.processFile(KtfmtWorkAction.kt:61)
	at com.ncorti.ktfmt.gradle.tasks.worker.KtfmtWorkAction.execute(KtfmtWorkAction.kt:48)
	at org.gradle.workers.internal.DefaultWorkerServer.execute(DefaultWorkerServer.java:63)
	at org.gradle.workers.internal.AbstractClassLoaderWorker$1.create(AbstractClassLoaderWorker.java:54)
	at org.gradle.workers.internal.AbstractClassLoaderWorker$1.create(AbstractClassLoaderWorker.java:48)
	at org.gradle.internal.classloader.ClassLoaderUtils.executeInClassloader(ClassLoaderUtils.java:100)
	at org.gradle.workers.internal.AbstractClassLoaderWorker.executeInClassLoader(AbstractClassLoaderWorker.java:48)
	at org.gradle.workers.internal.IsolatedClassloaderWorker.run(IsolatedClassloaderWorker.java:49)
	at org.gradle.workers.internal.IsolatedClassloaderWorker.run(IsolatedClassloaderWorker.java:30)
	at org.gradle.workers.internal.IsolatedClassloaderWorkerFactory$1.lambda$execute$0(IsolatedClassloaderWorkerFactory.java:57)
	at org.gradle.workers.internal.IsolatedClassloaderWorkerFactory$1$$Lambda$1789/0x0000000800d5ba28.execute(Unknown Source)
	at org.gradle.workers.internal.AbstractWorker$1.call(AbstractWorker.java:44)
	at org.gradle.workers.internal.AbstractWorker$1.call(AbstractWorker.java:41)

[simonhauck]

From my point of view, the above test would be sufficient. It triggers the same exception when I tested it. But @cortinico if you would still prefer an example with a real project I can still do it.

I read through the resources of @asapha and in this issue is a similar bug linked, that was solved with process isolation. The pull request is here.

I forked this repository and did some testing that you can find here and the relevant commit here

I tested all 3 isolation strategies and timed their execution time:

1. Class loader isolation crashes for me with 30 files and is in my opinion not a viable solution
2. Process isolation works. The test took 6605ms for 1000 files. With this option it is still possible to set the class loader. And from my understanding this was the goal of #181 - move ktfmt to use Worker API for classloader isolation #182
3. No isolation works also: The test took 2413 ms for 1000 files. With this option the classpath can not be set.

With these results, one of these 3 options seem the most suitable to me:

1. Use no isolation and live with potential class path conflicts
2. Use process isolation and accept comparatively slow performance
3. Add an option in the plugin configuration to set the desired strategy. This scenario could lead to higher maintenance effort in the future, since both versions and their different classpaths must be tested.

What is your opinion on that?

[cortinico]

@simonhauck thank you very much for you thorough investigation and testing!

I believe we should offer the Gradle property you're exposing in e74a8a0

I would default to NO_ISOLATION as you implemented as that covers the majority of use cases for the users, so feel free to send over a PR, we'll get it merged and make a new release with it.