From 1b6e89c971d2be28cca344ceb84d628f694c76ac Mon Sep 17 00:00:00 2001
From: odenix <self@odenix.org>
Date: Thu, 14 May 2026 20:02:23 +0200
Subject: [PATCH] pkl-doc: Fix/improve Executor handling in DocGenerator
 (#1590)

run() now creates and closes a default Executor per call. This is fine
because there is no good reason to call this method multiple times.

run(Executor) now lets callers provide their own Executor, which is
customary for a well-behaved library.

Also: Fix IntelliJ warning by calling toSet()

Closes #1583
---
 .../main/kotlin/org/pkl/doc/DocGenerator.kt   | 68 ++++++++++++-------
 .../kotlin/org/pkl/doc/DocGeneratorTest.kt    | 17 ++---
 2 files changed, 48 insertions(+), 37 deletions(-)

diff --git a/pkl-doc/src/main/kotlin/org/pkl/doc/DocGenerator.kt b/pkl-doc/src/main/kotlin/org/pkl/doc/DocGenerator.kt
index 2aa6065d..f6e677f4 100644
--- a/pkl-doc/src/main/kotlin/org/pkl/doc/DocGenerator.kt
+++ b/pkl-doc/src/main/kotlin/org/pkl/doc/DocGenerator.kt
@@ -22,7 +22,9 @@ import java.net.URI
 import java.nio.file.Files
 import java.nio.file.Path
 import java.util.concurrent.Executor
+import java.util.concurrent.ExecutorService
 import java.util.concurrent.Executors
+import java.util.concurrent.TimeUnit
 import kotlin.io.path.*
 import kotlinx.coroutines.*
 import org.pkl.commons.copyRecursively
@@ -107,30 +109,23 @@ class DocGenerator(
         .toList()
     }
 
-    /**
-     * The default executor when running the doc generator.
-     *
-     * Uses [Executors.newVirtualThreadPerTaskExecutor] if available (JDK 21). Otherwise, uses
-     * [Executors.newFixedThreadPool] with 64 threads, or the number of available processors
-     * available to the JVM (whichever is higher).
-     */
-    internal val executor: Executor
-      get() {
-        try {
-          val method = Executors::class.java.getMethod("newVirtualThreadPerTaskExecutor")
-          return method.invoke(null) as Executor
-        } catch (e: Throwable) {
-          when (e) {
-            is NoSuchMethodException,
-            is IllegalAccessException,
-            is InvocationTargetException ->
-              return Executors.newFixedThreadPool(
-                64.coerceAtLeast(Runtime.getRuntime().availableProcessors())
-              )
-            else -> throw e
-          }
+    internal fun createDefaultExecutor(): ExecutorService {
+      try {
+        val method = Executors::class.java.getMethod("newVirtualThreadPerTaskExecutor")
+        return method.invoke(null) as ExecutorService
+      } catch (e: Throwable) {
+        when (e) {
+          is NoSuchMethodException,
+          is IllegalAccessException,
+          is InvocationTargetException ->
+            return Executors.newFixedThreadPool(
+              64.coerceAtLeast(Runtime.getRuntime().availableProcessors())
+            )
+
+          else -> throw e
         }
       }
+    }
   }
 
   private val descendingVersionComparator: Comparator<String> = versionComparator.reversed()
@@ -163,8 +158,31 @@ class DocGenerator(
     siteSearchIndex.map { async { tryLoadPackageData(it) } }.awaitAll().filterNotNull()
   }
 
-  /** Runs this documentation generator. */
-  fun run() =
+  /**
+   * Runs this documentation generator with a default executor created and closed during the run.
+   *
+   * On JDK 21+, the default executor is [Executors.newVirtualThreadPerTaskExecutor]. On earlier JDK
+   * versions, it is [Executors.newFixedThreadPool] with either 64 threads or the number of
+   * processors available to the JVM, whichever is greater.
+   */
+  fun run() {
+    val executor = createDefaultExecutor()
+    try {
+      run(executor)
+    } finally {
+      // can't use close() because we compile with --release 17
+      executor.shutdown()
+      try {
+        executor.awaitTermination(Long.MAX_VALUE, TimeUnit.NANOSECONDS)
+      } catch (e: InterruptedException) {
+        executor.shutdownNow()
+        throw e
+      }
+    }
+  }
+
+  /** Runs this documentation generator with the given executor. */
+  fun run(executor: Executor) =
     runBlocking(executor.asCoroutineDispatcher()) {
       try {
         if (!docMigrator.isUpToDate) {
@@ -230,7 +248,7 @@ class DocGenerator(
     currentPackages: List<PackageData>,
     existingCurrentPackages: List<PackageData>,
   ) {
-    val packagesToCreate = currentPackages - existingCurrentPackages
+    val packagesToCreate = currentPackages - existingCurrentPackages.toSet()
     for (packageData in packagesToCreate) {
       val basePath = outputDir.resolve(packageData.ref.pkg.pathEncoded)
       val src = basePath.resolve(packageData.ref.version)
diff --git a/pkl-doc/src/test/kotlin/org/pkl/doc/DocGeneratorTest.kt b/pkl-doc/src/test/kotlin/org/pkl/doc/DocGeneratorTest.kt
index a6511749..b2aa8d8f 100644
--- a/pkl-doc/src/test/kotlin/org/pkl/doc/DocGeneratorTest.kt
+++ b/pkl-doc/src/test/kotlin/org/pkl/doc/DocGeneratorTest.kt
@@ -17,22 +17,15 @@ package org.pkl.doc
 
 import org.assertj.core.api.Assertions.assertThat
 import org.junit.jupiter.api.Test
-import org.junit.jupiter.api.condition.EnabledIf
+import org.junit.jupiter.api.condition.EnabledForJreRange
+import org.junit.jupiter.api.condition.JRE
 
 class DocGeneratorTest {
-  companion object {
-    @JvmStatic
-    fun isJdk21OrLater(): Boolean {
-      return Runtime.version().feature() >= 21
-    }
-  }
-
   @Test
-  @EnabledIf("isJdk21OrLater")
-  fun `uses virtual thread executor on JDK 21`() {
+  @EnabledForJreRange(min = JRE.JAVA_21)
+  fun `uses virtual thread executor on JDK 21+`() {
     // On older JDKs, we get a ThreadPoolExecutor.
-    // not sure if there's a better assertion to make here.
-    assertThat(DocGenerator.executor.javaClass.canonicalName)
+    assertThat(DocGenerator.createDefaultExecutor().javaClass.canonicalName)
       .isEqualTo("java.util.concurrent.ThreadPerTaskExecutor")
   }
 }