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

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()
     }
 
-    /**
+    internal fun createDefaultExecutor(): ExecutorService {
-     * The default executor when running the doc generator.
+      try {
-     *
+        val method = Executors::class.java.getMethod("newVirtualThreadPerTaskExecutor")
-     * Uses [Executors.newVirtualThreadPerTaskExecutor] if available (JDK 21). Otherwise, uses
+        return method.invoke(null) as ExecutorService
-     * [Executors.newFixedThreadPool] with 64 threads, or the number of available processors
+      } catch (e: Throwable) {
-     * available to the JVM (whichever is higher).
+        when (e) {
-     */
+          is NoSuchMethodException,
-    internal val executor: Executor
+          is IllegalAccessException,
-      get() {
+          is InvocationTargetException ->
-        try {
+            return Executors.newFixedThreadPool(
-          val method = Executors::class.java.getMethod("newVirtualThreadPerTaskExecutor")
+              64.coerceAtLeast(Runtime.getRuntime().availableProcessors())
-          return method.invoke(null) as Executor
+            )
-        } catch (e: Throwable) {
+
-          when (e) {
+          else -> throw 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)

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")
+  @EnabledForJreRange(min = JRE.JAVA_21)
-  fun `uses virtual thread executor on JDK 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.createDefaultExecutor().javaClass.canonicalName)
-    assertThat(DocGenerator.executor.javaClass.canonicalName)
       .isEqualTo("java.util.concurrent.ThreadPerTaskExecutor")
   }
 }