Bind PackageServer to ephemeral port to avoid port conflicts (#227)

This is a comprehensive solution to the "flaky PackageServer tests" problem. It rules out port conflicts and imposes no limits on test parallelism. The same solution can be used for other test servers in the future. Major changes: - Turn `PackageServer` from a singleton into a class that is instantiated per test class or test method. - Start the server the first time its `port` property is read. Bind the server to an ephemeral port instead of port 12110. - For every test that uses `PackageServer`, pass the server port to `--test-port`, `HttpClient.Builder.setTestPort`, the `CliBaseOptions` or `ExecutorOptions` constructor, or the Gradle plugin's `testPort` property. Wire all of these to `RequestRewritingClient`'s `testPort` constructor parameter. - Enhance `RequestRewritingClient` to replace port 12110 with `testPort` in request URIs unless `testPort` is -1 (its default). - Introduce `ExecutorOptions.Builder`. This makes executor options more comfortable to create and allows to hide options such as `testPort`. - Deprecate the `ExecutorOptions` constructor to steer users towards the builder. - Get rid of `ExecutorOptions2`, which is no longer needed. - Clean up `EmbeddedExecutorTest` with the help of the builder.
2026-04-25 09:48:41 +02:00 · 2024-03-13 10:40:55 -07:00
parent 1e608b2aae
commit 014b3a8816
26 changed files with 756 additions and 581 deletions
--- a/pkl-core/src/main/java/org/pkl/core/http/HttpClient.java
+++ b/pkl-core/src/main/java/org/pkl/core/http/HttpClient.java
@@ -108,6 +108,14 @@ public interface HttpClient extends AutoCloseable {
     */
    Builder addBuiltInCertificates();

+    /**
+     * Sets a test server's listening port.
+     *
+     * <p>If set, requests that specify port 12110 will be modified to use the given port. This is
+     * an internal test option.
+     */
+    Builder setTestPort(int port);
+
    /**
     * Creates a new {@code HttpClient} from the current state of this builder.
     *
--- a/pkl-core/src/main/java/org/pkl/core/http/HttpClientBuilder.java
+++ b/pkl-core/src/main/java/org/pkl/core/http/HttpClientBuilder.java
@@ -35,6 +35,7 @@ final class HttpClientBuilder implements HttpClient.Builder {
  private final Path caCertsDir;
  private final List<Path> certificateFiles = new ArrayList<>();
  private final List<URI> certificateUris = new ArrayList<>();
+  private int testPort = -1;

  HttpClientBuilder() {
    this(IoUtils.getPklHomeDir().resolve("cacerts"));
@@ -102,6 +103,12 @@ final class HttpClientBuilder implements HttpClient.Builder {
    return this;
  }

+  @Override
+  public HttpClient.Builder setTestPort(int port) {
+    testPort = port;
+    return this;
+  }
+
  @Override
  public HttpClient build() {
    return doBuild().get();
@@ -118,7 +125,7 @@ final class HttpClientBuilder implements HttpClient.Builder {
    var certificateUris = List.copyOf(this.certificateUris);
    return () -> {
      var jdkClient = new JdkHttpClient(certificateFiles, certificateUris, connectTimeout);
-      return new RequestRewritingClient(userAgent, requestTimeout, jdkClient);
+      return new RequestRewritingClient(userAgent, requestTimeout, testPort, jdkClient);
    };
  }

--- a/pkl-core/src/main/java/org/pkl/core/http/RequestRewritingClient.java
+++ b/pkl-core/src/main/java/org/pkl/core/http/RequestRewritingClient.java
@@ -16,12 +16,14 @@
 package org.pkl.core.http;

 import java.io.IOException;
+import java.net.URI;
 import java.net.http.HttpRequest;
 import java.net.http.HttpResponse;
 import java.net.http.HttpResponse.BodyHandler;
 import java.time.Duration;
 import java.util.concurrent.atomic.AtomicBoolean;
 import javax.annotation.concurrent.ThreadSafe;
+import org.pkl.core.util.HttpUtils;

 /**
 * An {@code HttpClient} decorator that
@@ -40,13 +42,16 @@ final class RequestRewritingClient implements HttpClient {
  // non-private for testing
  final String userAgent;
  final Duration requestTimeout;
+  final int testPort;
  final HttpClient delegate;

  private final AtomicBoolean closed = new AtomicBoolean();

-  RequestRewritingClient(String userAgent, Duration requestTimeout, HttpClient delegate) {
+  RequestRewritingClient(
+      String userAgent, Duration requestTimeout, int testPort, HttpClient delegate) {
    this.userAgent = userAgent;
    this.requestTimeout = requestTimeout;
+    this.testPort = testPort;
    this.delegate = delegate;
  }

@@ -67,7 +72,7 @@ final class RequestRewritingClient implements HttpClient {
    HttpRequest.Builder builder = HttpRequest.newBuilder();

    builder
-        .uri(original.uri())
+        .uri(rewriteUri(original.uri()))
        .expectContinue(original.expectContinue())
        .timeout(original.timeout().orElse(requestTimeout))
        .version(original.version().orElse(java.net.http.HttpClient.Version.HTTP_2));
@@ -99,6 +104,15 @@ final class RequestRewritingClient implements HttpClient {
    return builder.build();
  }

+  private URI rewriteUri(URI uri) {
+    // Would be nice to use port 0 instead of 12110,
+    // but this is best done in a separate commit.
+    if (testPort != -1 && uri.getPort() == 12110) {
+      return HttpUtils.setPort(uri, testPort);
+    }
+    return uri;
+  }
+
  private void checkNotClosed(HttpRequest request) {
    if (closed.get()) {
      throw new IllegalStateException(
--- a/pkl-core/src/main/java/org/pkl/core/service/ExecutorSpiImpl.java
+++ b/pkl-core/src/main/java/org/pkl/core/service/ExecutorSpiImpl.java
@@ -132,22 +132,26 @@ public class ExecutorSpiImpl implements ExecutorSpi {
  private HttpClient getOrCreateHttpClient(ExecutorSpiOptions options) {
    List<Path> certificateFiles;
    List<URI> certificateUris;
+    int testPort;
    try {
      if (options instanceof ExecutorSpiOptions2) {
        var options2 = (ExecutorSpiOptions2) options;
        certificateFiles = options2.getCertificateFiles();
        certificateUris = options2.getCertificateUris();
+        testPort = options2.getTestPort();
      } else {
        certificateFiles = List.of();
        certificateUris = List.of();
+        testPort = -1;
      }
      // host pkl-executor does not have class ExecutorOptions2 defined.
      // this will happen if the pkl-executor distribution is too old.
    } catch (NoClassDefFoundError e) {
      certificateFiles = List.of();
      certificateUris = List.of();
+      testPort = -1;
    }
-    var clientKey = new HttpClientKey(certificateFiles, certificateUris);
+    var clientKey = new HttpClientKey(certificateFiles, certificateUris, testPort);
    return httpClients.computeIfAbsent(
        clientKey,
        (key) -> {
@@ -158,6 +162,7 @@ public class ExecutorSpiImpl implements ExecutorSpi {
          for (var uri : key.certificateUris) {
            builder.addCertificates(uri);
          }
+          builder.setTestPort(key.testPort);
          // If the above didn't add any certificates,
          // builder will use the JVM's default SSL context.
          return builder.buildLazily();
@@ -167,11 +172,13 @@ public class ExecutorSpiImpl implements ExecutorSpi {
  private static final class HttpClientKey {
    final Set<Path> certificateFiles;
    final Set<URI> certificateUris;
+    final int testPort;

-    HttpClientKey(List<Path> certificateFiles, List<URI> certificateUris) {
+    HttpClientKey(List<Path> certificateFiles, List<URI> certificateUris, int testPort) {
      // also serve as defensive copies
      this.certificateFiles = Set.copyOf(certificateFiles);
      this.certificateUris = Set.copyOf(certificateUris);
+      this.testPort = testPort;
    }

    @Override
@@ -184,12 +191,13 @@ public class ExecutorSpiImpl implements ExecutorSpi {
      }
      HttpClientKey that = (HttpClientKey) obj;
      return certificateFiles.equals(that.certificateFiles)
-          && certificateUris.equals(that.certificateUris);
+          && certificateUris.equals(that.certificateUris)
+          && testPort == that.testPort;
    }

    @Override
    public int hashCode() {
-      return Objects.hash(certificateFiles, certificateUris);
+      return Objects.hash(certificateFiles, certificateUris, testPort);
    }
  }
 }
--- a/pkl-core/src/main/java/org/pkl/core/util/HttpUtils.java
+++ b/pkl-core/src/main/java/org/pkl/core/util/HttpUtils.java
@@ -17,8 +17,10 @@ package org.pkl.core.util;

 import java.io.IOException;
 import java.net.URI;
+import java.net.URISyntaxException;
 import java.net.URL;
 import java.net.http.HttpResponse;
+import org.pkl.core.PklBugException;

 public final class HttpUtils {
  private HttpUtils() {}
@@ -47,4 +49,22 @@ public final class HttpUtils {
    throw new IOException(
        ErrorMessages.create("badHttpStatusCode", response.statusCode(), response.uri()));
  }
+
+  public static URI setPort(URI uri, int port) {
+    if (port < 0 || port > 65535) {
+      throw new IllegalArgumentException(String.valueOf(port));
+    }
+    try {
+      return new URI(
+          uri.getScheme(),
+          uri.getUserInfo(),
+          uri.getHost(),
+          port,
+          uri.getPath(),
+          uri.getQuery(),
+          uri.getFragment());
+    } catch (URISyntaxException e) {
+      throw PklBugException.unreachableCode(); // only port changed
+    }
+  }
 }
--- a/pkl-core/src/test/kotlin/org/pkl/core/StackFrameTransformersTest.kt
+++ b/pkl-core/src/test/kotlin/org/pkl/core/StackFrameTransformersTest.kt
@@ -4,25 +4,30 @@ import org.pkl.commons.test.PackageServer
 import org.assertj.core.api.Assertions.assertThat
 import org.junit.jupiter.api.Disabled
 import org.junit.jupiter.api.Test
+import org.pkl.core.http.HttpClient

 class StackFrameTransformersTest {
  // TODO figure out how to test this; right now this fails because there is no VM context.
  @Test
  @Disabled
  fun replacePackageUriWithSourceCodeUrl() {
-    PackageServer.ensureStarted()
-    EvaluatorBuilder.preconfigured().build().use {
-      val frame = StackFrame(
-        "package://localhost:12110/birds@0.5.0#/Bird.pkl",
-        null,
-        listOf(),
-        1,
-        1,
-        2,
-        2)
-      val transformed =
-        StackFrameTransformers.replacePackageUriWithSourceCodeUrl.apply(frame)
-      assertThat(transformed.moduleUri).isEqualTo("https://example.com/birds/v0.5.0/blob/Bird.pkl#L1-L2")
+    PackageServer().use { server ->
+      val httpClient = HttpClient.builder().setTestPort(server.port).build()
+      EvaluatorBuilder.preconfigured()
+        .setHttpClient(httpClient)
+        .build().use {
+          val frame = StackFrame(
+            "package://localhost:12110/birds@0.5.0#/Bird.pkl",
+            null,
+            listOf(),
+            1,
+            1,
+            2,
+            2)
+          val transformed =
+            StackFrameTransformers.replacePackageUriWithSourceCodeUrl.apply(frame)
+          assertThat(transformed.moduleUri).isEqualTo("https://example.com/birds/v0.5.0/blob/Bird.pkl#L1-L2")
+        }
    }
  }
 }
--- a/pkl-core/src/test/kotlin/org/pkl/core/http/RequestRewritingClientTest.kt
+++ b/pkl-core/src/test/kotlin/org/pkl/core/http/RequestRewritingClientTest.kt
@@ -6,14 +6,13 @@ import org.junit.jupiter.api.Test
 import java.net.URI
 import java.net.http.HttpRequest
 import java.net.http.HttpRequest.BodyPublishers
-import java.net.http.HttpResponse
 import java.net.http.HttpResponse.BodyHandlers
 import java.time.Duration
 import java.net.http.HttpClient as JdkHttpClient

 class RequestRewritingClientTest {
  private val captured = RequestCapturingClient()
-  private val client = RequestRewritingClient("Pkl", Duration.ofSeconds(42), captured)
+  private val client = RequestRewritingClient("Pkl", Duration.ofSeconds(42), -1, captured)
  private val exampleUri = URI("https://example.com/foo/bar.html")
  private val exampleRequest = HttpRequest.newBuilder(exampleUri).build()

@@ -103,6 +102,26 @@ class RequestRewritingClientTest {
    
    assertThat(captured.request.bodyPublisher().get()).isSameAs(publisher)
  }
+  
+  @Test
+  fun `rewrites port 12110 if test port is set`() {
+    val captured = RequestCapturingClient()
+    val client = RequestRewritingClient("Pkl", Duration.ofSeconds(42), 5000, captured)
+    val request = HttpRequest.newBuilder(URI("https://example.com:12110")).build()
+
+    client.send(request, BodyHandlers.discarding())
+
+    assertThat(captured.request.uri().port).isEqualTo(5000)
+  }
+  
+  @Test
+  fun `leaves port 12110 intact if no test port is set`() {
+    val request = HttpRequest.newBuilder(URI("https://example.com:12110")).build()
+
+    client.send(request, BodyHandlers.discarding())
+    
+    assertThat(captured.request.uri().port).isEqualTo(12110)
+  }
 }


--- a/pkl-core/src/test/kotlin/org/pkl/core/packages/PackageResolversTest.kt
+++ b/pkl-core/src/test/kotlin/org/pkl/core/packages/PackageResolversTest.kt
@@ -6,8 +6,6 @@ import org.junit.jupiter.api.AfterAll
 import org.junit.jupiter.api.BeforeAll
 import org.junit.jupiter.api.Test
 import org.junit.jupiter.api.assertThrows
-import org.junit.jupiter.api.parallel.Execution
-import org.junit.jupiter.api.parallel.ExecutionMode
 import org.pkl.commons.deleteRecursively
 import org.pkl.commons.readString
 import org.pkl.commons.test.FileTestUtils
@@ -23,7 +21,6 @@ import kotlin.io.path.exists
 import kotlin.io.path.readBytes

 class PackageResolversTest {
-  @Execution(ExecutionMode.SAME_THREAD)
  abstract class AbstractPackageResolverTest {

    abstract val resolver: PackageResolver
@@ -31,15 +28,20 @@ class PackageResolversTest {
    private val packageRoot = FileTestUtils.rootProjectDir.resolve("pkl-commons-test/src/main/files/packages")

    companion object {
+      private val packageServer = PackageServer()
+      
      @JvmStatic
-      @BeforeAll
-      fun beforeAll() {
-        PackageServer.ensureStarted()
+      @AfterAll
+      fun afterAll() {
+        packageServer.close()
      }
      
-      val httpClient: HttpClient = HttpClient.builder()
-        .addCertificates(FileTestUtils.selfSignedCertificate)
-        .build()
+      val httpClient: HttpClient by lazy {
+        HttpClient.builder()
+          .addCertificates(FileTestUtils.selfSignedCertificate)
+          .setTestPort(packageServer.port)
+          .build()
+      }
    }

    @Test
@@ -199,7 +201,6 @@ class PackageResolversTest {
      @BeforeAll
      @JvmStatic
      fun beforeAll() {
-        PackageServer.ensureStarted()
        cacheDir.deleteRecursively()
      }
    }
--- a/pkl-core/src/test/kotlin/org/pkl/core/project/ProjectDependenciesResolverTest.kt
+++ b/pkl-core/src/test/kotlin/org/pkl/core/project/ProjectDependenciesResolverTest.kt
@@ -1,7 +1,7 @@
 package org.pkl.core.project

 import org.assertj.core.api.Assertions.assertThat
-import org.junit.jupiter.api.BeforeAll
+import org.junit.jupiter.api.AfterAll
 import org.junit.jupiter.api.Test
 import org.junit.jupiter.api.assertThrows
 import org.pkl.commons.test.FileTestUtils
@@ -16,15 +16,20 @@ import java.nio.file.Path

 class ProjectDependenciesResolverTest {
  companion object {
+    private val packageServer = PackageServer()
+    
    @JvmStatic
-    @BeforeAll
-    fun beforeAll() {
-      PackageServer.ensureStarted()
+    @AfterAll
+    fun afterAll() {
+      packageServer.close()
    }
    
-    val httpClient: HttpClient = HttpClient.builder()
-      .addCertificates(FileTestUtils.selfSignedCertificate)
-      .build()
+    val httpClient: HttpClient by lazy {
+      HttpClient.builder()
+        .addCertificates(FileTestUtils.selfSignedCertificate)
+        .setTestPort(packageServer.port)
+        .build()
+    }
  }

  @Test
--- a/pkl-core/src/test/kotlin/org/pkl/core/project/ProjectTest.kt
+++ b/pkl-core/src/test/kotlin/org/pkl/core/project/ProjectTest.kt
@@ -136,19 +136,20 @@ class ProjectTest {

  @Test
  fun `evaluate project module -- invalid checksum`() {
-    PackageServer.ensureStarted()
-    val projectDir = Path.of(javaClass.getResource("badProjectChecksum2/")!!.path)
-    val project = Project.loadFromPath(projectDir.resolve("PklProject"))
-    val httpClient = HttpClient.builder()
-      .addCertificates(FileTestUtils.selfSignedCertificate)
-      .build()
-    val evaluator = EvaluatorBuilder.preconfigured()
-      .applyFromProject(project)
-      .setModuleCacheDir(null)
-      .setHttpClient(httpClient)
-      .build()
-    assertThatCode { evaluator.evaluate(ModuleSource.path(projectDir.resolve("bug.pkl"))) }
-      .hasMessageStartingWith("""
+    PackageServer().use { server ->
+      val projectDir = Path.of(javaClass.getResource("badProjectChecksum2/")!!.path)
+      val project = Project.loadFromPath(projectDir.resolve("PklProject"))
+      val httpClient = HttpClient.builder()
+        .addCertificates(FileTestUtils.selfSignedCertificate)
+        .setTestPort(server.port)
+        .build()
+      val evaluator = EvaluatorBuilder.preconfigured()
+        .applyFromProject(project)
+        .setModuleCacheDir(null)
+        .setHttpClient(httpClient)
+        .build()
+      assertThatCode { evaluator.evaluate(ModuleSource.path(projectDir.resolve("bug.pkl"))) }
+        .hasMessageStartingWith("""
        –– Pkl Error ––
        Cannot download package `package://localhost:12110/fruit@1.0.5` because the computed checksum for package metadata does not match the expected checksum.
        
@@ -159,5 +160,6 @@ class ProjectTest {
        1 | import "@fruit/Fruit.pkl"
            ^^^^^^^^^^^^^^^^^^^^^^^^^
      """.trimIndent())
+    }
  }
 }
--- a/pkl-core/src/test/kotlin/org/pkl/core/util/HttpUtilsTest.kt
+++ b/pkl-core/src/test/kotlin/org/pkl/core/util/HttpUtilsTest.kt
@@ -39,4 +39,22 @@ class HttpUtilsTest {
      HttpUtils.checkHasStatusCode200(response2)
    }
  }
+  
+  @Test
+  fun setPort() {
+    assertThrows<IllegalArgumentException> {
+      HttpUtils.setPort(URI("https://example.com"), -1)
+    }
+    assertThrows<IllegalArgumentException> {
+      HttpUtils.setPort(URI("https://example.com"), 65536)
+    }
+    assertThat(HttpUtils.setPort(URI("http://example.com"), 123))
+      .isEqualTo(URI("http://example.com:123"))
+    assertThat(HttpUtils.setPort(URI("http://example.com:456"), 123))
+      .isEqualTo(URI("http://example.com:123"))
+    assertThat(HttpUtils.setPort(URI("https://example.com/foo/bar.baz?query=1#fragment"), 123))
+      .isEqualTo(URI("https://example.com:123/foo/bar.baz?query=1#fragment"))
+    assertThat(HttpUtils.setPort(URI("https://example.com:456/foo/bar.baz?query=1#fragment"), 123))
+      .isEqualTo(URI("https://example.com:123/foo/bar.baz?query=1#fragment"))
+  }
 }