Use java.net.http.HttpClient instead of java.net.Http(s)URLConnection #217

translatenix · 2024-02-20T21:25:19Z

Moving to java.net.http.HttpClient brings many benefits, including HTTP/2 support and the ability to make asynchronous requests.

Major additions and changes:

Introduce a lightweight org.pkl.core.http.HttpClient API. This keeps some flexibility and allows to enforce behavior such as setting the User-Agent header.
Provide an implementation that delegates to java.net.http.HttpClient.
Use HttpClient for all HTTP(s) requests across the codebase. This required adding an HttpClient parameter to constructors and factory methods of multiple classes, some of which are public APIs.
Manage CA certificates per HTTP client instead of per JVM. This makes it unnecessary to set JVM-wide system/security properties and default SSLSocketFactory's.

Each HTTP client maintains its own connection pool and SSLContext. For efficiency reasons, I've tried to reuse clients whenever feasible. To avoid memory leaks, clients are not stored in static fields.

HTTP clients are expensive to create. For this reason, EvaluatorBuilder defaults to a "lazy" client that creates the underlying java.net.http.HttpClient on the first send (which may never happen).

Fixes #157.

Note: This PR allows to fix the "flaky PackageServer tests" problem in a principled way. Because all HTTP requests are now routed through HttpClient, PackageServer can bind to a dynamic port, and HttpClient can rewrite requests to use that port in test mode. I've tested this approach on a local branch, and it's the first time I can get "gw clean build" to pass without "connection refused" errors.

bioball · 2024-02-21T01:02:13Z

Thanks! I haven't reviewed this yet, but this is erroring in our CI build, for test org.pkl.executor.EmbeddedExecutorTest:

org.pkl.executor.ExecutorException: 
–– Pkl Error ––
Exception when making request `GET https://localhost:12110/birds@0.5.0`:
PKIX path building failed: sun.security.provider.certpath.SunCertPathBuilderException: unable to find valid certification path to requested target

4 | import "package://localhost:12110/birds@0.5.0#/Bird.pkl"
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
at MyModule#Bird (file:///tmp/junit15463112887516922814/test.pkl)

6 | chirpy = new Bird { name = "Chirpy"; favoriteFruit { name = "Orange" } }
                 ^^^^
at MyModule#chirpy (file:///tmp/junit15463112887516922814/test.pkl)

106 | text = renderer.renderDocument(value)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
at pkl.base#Module.output.text (https://github.com/apple/pkl/blob/7a26325/stdlib/base.pkl#L106)

translatenix · 2024-02-21T01:38:26Z

Unfortunately I can't reproduce locally. Is it possible to trigger PR builds automatically for contributors? Otherwise the feedback loop is too long.

bioball · 2024-02-21T04:55:16Z

Re: CI: yeah, I realize it's kind of painful. We'll need to think through how to make this experience better for ya'll.

translatenix · 2024-02-21T05:32:48Z

I found and fixed the problem. It wasn't showing locally because the test was using cache dir ~/.pkl/cache.

The fix makes breaking changes to spi.v1, which probably isn't a good idea. Let me know if you'd like me to do spi.v2 instead.

holzensp

This looks really, really good... As to the API change; I believe the incompatible changes you're making are all additional fields in constructors and such (did I miss it?). Maybe it can stay v1 if you add additional constructors that retain the old behaviour and just fill in reasonable defaults for the HttpClient in those cases.

Thoughts, @bioball?

holzensp · 2024-02-21T19:04:20Z

pkl-cli/pkl-cli.gradle.kts

@@ -156,7 +156,7 @@ fun Exec.configureExecutable(isEnabled: Boolean, outputFile: File, extraArgs: Li
        ,"--no-fallback"
        ,"-H:IncludeResources=org/pkl/core/stdlib/.*\\.pkl"
        ,"-H:IncludeResources=org/jline/utils/.*"
-        ,"-H:IncludeResources=org/pkl/commons/cli/commands/IncludedCARoots.pem"
+        ,"-H:IncludeResources=org/pkl/core/http/IncludedCARoots.pem"


This move (or, rather, copy?), I don't quite understand? Why can't it stay in commons? cc @bioball

The resource is now exclusively used by org.pkl.core.http.HttpClientBuilder, so moving it was natural. I believe it is also necessary because core doesn't depend on commons-cli.

We intentionally kept this separate from pkl-core because on the Java side, we wanted our HTTP requests to defer to the host application's settings (e.g. trust the same CA roots).

holzensp · 2024-02-21T19:05:23Z

pkl-cli/src/test/kotlin/org/pkl/cli/CliEvaluatorTest.kt

@@ -1158,6 +1158,7 @@ result = someLib.x
  }

  @Test
+  @Disabled("flaky because CliEvaluator falls back to ~/.pkl/cacerts if no certs are given")


That should not make this flakey; "if no certs are given" seems a bug in the test, in this case.

Fixed in a later commit. I've noticed that several CLI tests use default settings and hence use ~/.pkl/cacerts and ~/.pkl/cache, which makes them non-deterministic.

holzensp · 2024-02-21T19:12:46Z

pkl-commons-cli/src/main/kotlin/org/pkl/commons/cli/CliBaseOptions.kt

+  /**
+   * The HTTP client shared between CLI commands created with this [CliBaseOptions] instance.
+   *
+   * To release the resources held by the HTTP client in a timely manner, call its `close()` method.


It's java.lang.AutoCloseable, so I think any recommendation should favour try-with-resources patterns, no?

I didn't change CliBaseOptions to implement AutoCloseable. Usually, an options object shouldn't need to be closed. Adding CliBaseOptions.httpClient felt a bit smelly, but it seemed like a price worth paying for being able to share HttpClient resources between commands. I couldn't find another way to achieve this without a major redesign.

I documented calling CliBaseOptions.httpClient.close() for the edge cases where it might matter. That's also why I made httpClient public.

Let me know if you want me to change CliBaseOptions to implement AutoCloseable. Kotlin doesn't have try-with-resources, but it does have a Closeable.use extension method.

I don't think CliBaseOptions implementing AutoCloseable is the right call.

We have a couple places where our builders accept Closeable, but we also try to avoid it whenever we can.

Probably makes more sense to either accept the options on HttpClient.Builder directly, or accept HttpClient.Builder as an option as a whole.

Same comment for the settings on EvaluatorBuilder.

Probably makes more sense to either accept the options on HttpClient.Builder directly, or accept HttpClient.Builder as an option as a whole.

Sorry I don't understand what you're proposing. Can you elaborate?
I considered passing HttpClient to CliCommand, but then CliBaseOptions.caCertificates is no longer meaningful. (It's HttpClient that needs to be configured with the certificates.)

Probably makes more sense to either accept the options on HttpClient.Builder directly, or accept HttpClient.Builder as an option as a whole.

Sorry but I don't follow. Can you elaborate on what you're proposing?

One option I considered was to pass HttpClient to constructors of CliCommand subclasses. But this doesn't really work because it makes CliBaseOptions.caCertificates irrelevant. (It's the HttpClient that needs to be configured with certificates.)

Sorry, so, one suggestion is to add the builder methods on HttpClient.Builder to EvaluatorBuilder:

class EvaluatorBuilder { public EvaluatorBuilder addCertificates(Path file) { /* etc */ } public EvaluatorBuilder addCertificates(URI file) { /* etc */ }

Another option is to accept an HttpClient.Builder as an option in EvaluatorBuilder:

class EvaluatorBuilder { public HttpClient.Builder setHttpClientBuilder(HttpClient.Builder builder) { /* etc */ } }

Currently we have EvaluatorBuilder.setHttpClient(). Your proposals might make EvaluatorBuilder more convenient to use if the default HTTP client isn't good enough. But I don't see how any of this relates to the CliBaseOptions.httpClient discussion, and it's not clear to me if and what change you want there.

I'm proposing that both here, and in evaluator builder, we provide knobs for addCertificates, and possibly any other options on HttpClient.Builder.

After playing around with this locally, though, I don't feel strongly about this, and am okay with how you are doing it.

holzensp · 2024-02-21T19:13:29Z

pkl-commons-cli/src/main/kotlin/org/pkl/commons/cli/CliBaseOptions.kt

+    // Lazy building significantly reduces execution time of commands that do minimal work.
+    // However, it means that HTTP client initialization errors won't surface until an HTTP request
+    // is
+    // made. A middleground would be to only build lazily if built-in certificates are used.


nit, comment formatting...

This is a little too contemplative / open-ended for my liking. Do you have strong preferences, @bioball?

The formatting is due to ktfmt, which seems to produce worse result than the Java formatter.

I'm okay with the contents of this comment; it provides some context if we want to revisit this decision.

Also, 👍 on lazy. Pkl is a lazy language anyway, and business logic should be deferred to when it is actually needed.

holzensp · 2024-02-21T19:15:01Z

pkl-commons-cli/src/main/kotlin/org/pkl/commons/cli/commands/BaseOptions.kt

@@ -142,28 +130,11 @@ class BaseOptions : OptionGroup() {
    option(
        names = arrayOf("--ca-certificates"),
        metavar = "<path>",
-        help = "Replaces the built-in CA certificates with the provided certificate file."
+        help = "Trust CA certificates from the provided file."


The replace wording was quite careful and should likely be retained. It should be clear this isn't additive.

I changed the wording because this option actually replaces ~/.pkl/cacerts, which in turn replaces the built-in certificates. This felt too long to explain here.

For now I changed this to: "Only trust CA certificates from the provided file(s)." (The option is repeatable.) Let me know if this works for you.

New wording LGTM

holzensp · 2024-02-22T15:54:54Z

pkl-core/src/main/java/org/pkl/core/http/RequestRewritingClient.java

+    if (!closed.getAndSet(true)) delegate.close();
+  }
+
+  // Based on JDK 17's implementation of HttpRequest.newBuilder(HttpRequest, filter).


Does that mean it's a drop-in replacement when we purge JDK11 dependencies?

It means that the implementation can be simplified once JDK 17 is available.

I can't tell; what is this rewriting?

It rewrites (modifies) the HttpRequest. From the Javadoc of HttpClient.send:

If the request does not specify a timeout, the client's
request timeout is used. If the request does not specify
a preferred HTTP version, HTTP/2 is used. The request's
User-Agent header is set to the client's User-Agent
header.

My package-server PR also rewrites the port if necessary.

Ah, I see. Thanks!

holzensp · 2024-02-22T15:57:44Z

pkl-core/src/main/java/org/pkl/core/module/ModuleKeys.java

@@ -494,7 +509,7 @@ public ResolvedModuleKey resolve(SecurityManager securityManager)
        }
        securityManager.checkResolveModule(redirected);
        var text = IoUtils.readString(stream);
-        return ResolvedModuleKeys.virtual(this, uri, text, true);
+        return ResolvedModuleKeys.virtual(this, redirected, text, true);


To use redirected here will have some confusing error messages when SecurityManager::checkImportModule fails.

I pondered on this for a while because I had to make the same decision a few lines above. I came to the conclusion that the use of uri here is a bug. It means that ModuleCache.modulesByResolvedUri will cache the module under its unresolved URI, which is exactly what ModuleCache.modulesByOriginalUri does. So modulesByResolvedUri no longer serves any purpose.

"resolve" here means resolving the in-language URI to the actual URI used for fetching a module's contents.

For example, an in-language URI might be projectpackage://pkg.pkl-lang.com/foo@1.0.0#/foo.kl, whereas the resolved URI might be file:///path/to/foo.pkl. It's useful here because the same actual file can be imported using two different URIs in language, but actually are both the same file and we don't need to load it twice.

It means that ModuleCache.modulesByResolvedUri will cache the module under its unresolved URI, which is exactly what ModuleCache.modulesByOriginalUri does. So modulesByResolvedUri no longer serves any purpose

There's plenty of module keys that have the same entries in both modulesByResolvedUri and modulesByOriginalUri (e.g. file: modules). It's expected for things like this.

holzensp · 2024-02-22T15:58:03Z

pkl-core/src/main/java/org/pkl/core/module/ResolvedModuleKeys.java

+      if (HttpUtils.isHttpUrl(url)) {
+        var httpClient = VmContext.get(null).getHttpClient();
+        var request = HttpRequest.newBuilder(uri).build();
+        var response = httpClient.send(request, BodyHandlers.ofString());
+        HttpUtils.checkHasStatusCode200(response);
+        return response.body();
+      }


Seems worth a helper; repeated.

There are too many differences to share with ResourceReaders, and I can't spot any other repetitions.

holzensp · 2024-02-22T15:58:38Z

pkl-core/src/main/java/org/pkl/core/resource/ResourceReaders.java

+      if (HttpUtils.isHttpUrl(uri)) {
+        var httpClient = VmContext.get(null).getHttpClient();
+        var request = HttpRequest.newBuilder(uri).build();
+        var response = httpClient.send(request, BodyHandlers.ofByteArray());
+        if (response.statusCode() == 404) return Optional.empty();
+        HttpUtils.checkHasStatusCode200(response);
+        return Optional.of(new Resource(uri, response.body()));
+      }


There are too many differences to share with ResolvedModuleKeys, and I can't spot any other repetitions.

holzensp · 2024-02-22T17:51:18Z

pkl-core/src/test/kotlin/org/pkl/core/http/HttpClientTest.kt

+
+  @Test
+  fun `can load certificates from default location`(@TempDir userHome: Path) {
+    val certFile = userHome.resolve(".pkl")


I see the intent here, but I feel userHome is misleading naming in this test

Changed to tempDir.

bioball

Amazing work! This is looking really great.

Some things that need changes (details are in my comments)

Move built-in CA certs back to pkl-commons
Let's not touch the executor API
EvaluatorBuilder.preconfigured() should trust the JVM's certs

Also: what was the thinking around introducing org.pkl.core.http.HttpClient?

I see the benefits that it brings (DummyHttpClient is quite nice). But, it also puts the burden on us to expose all the settings someone might want, that are already in java.net.http.HttpClient.Builder. Also, maybe users already have their own java.net.HttpClient that they want to use and share with the rest of their application?

bioball · 2024-02-22T22:13:35Z

pkl-commons-cli/src/main/kotlin/org/pkl/commons/cli/CliBaseOptions.kt

+    // Lazy building significantly reduces execution time of commands that do minimal work.
+    // However, it means that HTTP client initialization errors won't surface until an HTTP request
+    // is
+    // made. A middleground would be to only build lazily if built-in certificates are used.


I'm okay with the contents of this comment; it provides some context if we want to revisit this decision.

Also, 👍 on lazy. Pkl is a lazy language anyway, and business logic should be deferred to when it is actually needed.

bioball · 2024-02-22T22:14:46Z

pkl-commons-cli/src/main/kotlin/org/pkl/commons/cli/CliBaseOptions.kt

+    val builder = HttpClient.builder()
+    if (normalizedCaCertificates.isEmpty()) {
+      builder.addDefaultCliCertificates()
+    } else {
+      for (file in normalizedCaCertificates) builder.addCertificates(file)
+    }
+    // Lazy building significantly reduces execution time of commands that do minimal work.
+    // However, it means that HTTP client initialization errors won't surface until an HTTP request
+    // is
+    // made. A middleground would be to only build lazily if built-in certificates are used.
+    builder.buildLazily()


[nit] apply is a nice way to reduce a bit of boilerplate.

Suggested change

val builder = HttpClient.builder()

if (normalizedCaCertificates.isEmpty()) {

builder.addDefaultCliCertificates()

} else {

for (file in normalizedCaCertificates) builder.addCertificates(file)

}

// Lazy building significantly reduces execution time of commands that do minimal work.

// However, it means that HTTP client initialization errors won't surface until an HTTP request

// is

// made. A middleground would be to only build lazily if built-in certificates are used.

builder.buildLazily()

HttpClient.builder().apply {

if (normalizedCaCertificates.isEmpty()) {

addDefaultCliCertificates()

} else {

for (file in normalizedCaCertificates) addCertificates(file)

}

}

// Lazy building significantly reduces execution time of commands that do minimal work.

// However, it means that HTTP client initialization errors won't surface until an HTTP request

// is

// made. A middleground would be to only build lazily if built-in certificates are used.

.buildLazily

Move built-in CA certs back to pkl-commons

They never were in pkl-commons. Details elsewhere.

Let's not touch the executor API

Further discussed elsewhere.

EvaluatorBuilder.preconfigured() should trust the JVM's certs

I think the best I can do is to use SSLContext.getDefault(), which is more or less what you're asking for.

Also: what was the thinking around introducing org.pkl.core.http.HttpClient?

At first I was using java.net.http.HttpClient directly. But baking this interface into Pkl APIs didn't feel right. Having our own abstraction has clear benefits:

It puts us in control: We can remove what is irrelevant to Pkl, and add or change what is beneficial.

A smaller interface is easier to implement and doesn't require throwing UnsupportedOperationException for all the methods that a particular implementation may not be able or willing to support. We already have 4 implementations, and chances are there will be more.

For the same reason, it will be easier to support alternative implementations (Apache, Netty, etc.) if the need arises. While java.net.http.HttpClient is a clear step up from HttpURLConnection, it will always remain less capable than third-party libraries. For example, Netty supports a safer TLS implementation.

But, it also puts the burden on us to expose all the settings someone might want, that are already in java.net.http.HttpClient.Builder.
Also, maybe users already have their own java.net.HttpClient that they want to use and share with the rest of their application?

We could easily support this with something like an HttpClient from(java.net.http.HttpClient) factory method.

I didn't want to go so far as to have our own HttpRequest(Builder), HttpResponse(Builder), etc. Having "just" our own HttpClient(Builder) felt like the sweet spot.

apply is a nice way to reduce a bit of boilerplate.

Changed to with(HttpClient.builder()) {...}.

I think the best I can do is to use SSLContext.getDefault(), which is more or less what you're asking for.

Yeah, this sounds like the right thing to do here.

We could easily support this with something like an HttpClient from(java.net.http.HttpClient) factory method.

Ah, that's a neat idea.

pkl-commons-cli/src/main/kotlin/org/pkl/commons/cli/CliBaseOptions.kt

bioball · 2024-02-23T00:15:44Z

pkl-cli/pkl-cli.gradle.kts

@@ -156,7 +156,7 @@ fun Exec.configureExecutable(isEnabled: Boolean, outputFile: File, extraArgs: Li
        ,"--no-fallback"
        ,"-H:IncludeResources=org/pkl/core/stdlib/.*\\.pkl"
        ,"-H:IncludeResources=org/jline/utils/.*"
-        ,"-H:IncludeResources=org/pkl/commons/cli/commands/IncludedCARoots.pem"
+        ,"-H:IncludeResources=org/pkl/core/http/IncludedCARoots.pem"


We intentionally kept this separate from pkl-core because on the Java side, we wanted our HTTP requests to defer to the host application's settings (e.g. trust the same CA roots).

pkl-core/src/main/java/org/pkl/core/http/HttpClient.java

bioball · 2024-02-23T02:44:46Z

pkl-core/src/main/java/org/pkl/core/http/LazyHttpClient.java

+import javax.annotation.concurrent.ThreadSafe;
+
+@ThreadSafe
+class LazyHttpClient implements HttpClient {


Ditto; docs would be helpful for maintainability.

bioball · 2024-02-23T02:46:39Z

pkl-core/src/main/java/org/pkl/core/http/RequestRewritingClient.java

+    if (!closed.getAndSet(true)) delegate.close();
+  }
+
+  // Based on JDK 17's implementation of HttpRequest.newBuilder(HttpRequest, filter).


I can't tell; what is this rewriting?

bioball · 2024-02-23T03:03:02Z

pkl-core/src/main/java/org/pkl/core/module/ModuleKeys.java

@@ -494,7 +509,7 @@ public ResolvedModuleKey resolve(SecurityManager securityManager)
        }
        securityManager.checkResolveModule(redirected);
        var text = IoUtils.readString(stream);
-        return ResolvedModuleKeys.virtual(this, uri, text, true);
+        return ResolvedModuleKeys.virtual(this, redirected, text, true);


"resolve" here means resolving the in-language URI to the actual URI used for fetching a module's contents.

For example, an in-language URI might be projectpackage://pkg.pkl-lang.com/foo@1.0.0#/foo.kl, whereas the resolved URI might be file:///path/to/foo.pkl. It's useful here because the same actual file can be imported using two different URIs in language, but actually are both the same file and we don't need to load it twice.

It means that ModuleCache.modulesByResolvedUri will cache the module under its unresolved URI, which is exactly what ModuleCache.modulesByOriginalUri does. So modulesByResolvedUri no longer serves any purpose

There's plenty of module keys that have the same entries in both modulesByResolvedUri and modulesByOriginalUri (e.g. file: modules). It's expected for things like this.

pkl-core/src/main/java/org/pkl/core/service/ExecutorSpiImpl.java

bioball · 2024-02-23T03:18:30Z

pkl-executor/src/main/java/org/pkl/executor/spi/v1/ExecutorSpiOptions.java

+  public List<URL> getCertificateUrls() {
+    return certificateUrls;
+  }
+


I think I'd prefer to not change this.

If we add this, we'll need to introduce v2. And, for the executor use-case, you likely want Pkl to trust the same certs that the JVM already trusts anyways. It doesn't seem compelling enough to add a new knob here just yet.

I think let's have ExecutorSpiImpl use an http client that uses the default SSLContext. And in EmbeddedExecutorTest, we can use the good ol' CertificateUtils.setupAllX509CertificatesGlobally to get those tests to pass. Except move that class into pkl-commons-test.

I don't mind not supporting certificates in Executor. But I really feel that CertificateUtils.setupAllX509CertificatesGlobally needs to go. It's a hack that has caused, and will cause, trouble (might get used elsewhere, might interfere with other tests, code duplication with JdkHttpClient, etc.).

PS: My package-server PR also breaks spi.v1. Perhaps it's time for spi.v2 anyways?

By moving CertificateUtils.setupAllX509CertificatesGlobally into pkl-commons-test, we ensure that it's only used for testing purposes. And likely, the embedded executor test will be the only place that uses it, so we can even just inline its contents into that test itself.

PS: My package-server PR also breaks spi.v1. Perhaps it's time for spi.v2 anyways?

Hm, I haven't reviewed that one yet. I'll need to take a look. Yeah if we do introduce spi.v2, might as well keep these options.

By moving CertificateUtils.setupAllX509CertificatesGlobally into pkl-commons-test, we ensure that it's only used for testing purposes.

I think it's only a matter of time until this will once again cause flaky tests. Mutating global state is best avoided. I'd be more than happy to implement spi.v2 if that's what it takes.

translatenix · 2024-02-23T20:56:27Z

@holzensp @bioball I think I've addressed all feedback (see recent commits) except for the following open questions:

pkl-certs details (see that thread)
org.pkl.executor.Executor details (see that thread)
CliBaseOptions.httpClient details (see that thread)

translatenix · 2024-02-26T19:31:55Z

@holzensp @bioball I'm blocked on your feedback.
I'd love to land this PR and #227 ASAP to stabilize the build/tests. It will make everything easier for everyone.

I think I've addressed all feedback (see recent commits) except for the following open questions:

pkl-certs details (see that thread)

org.pkl.executor.Executor details (see that thread)

CliBaseOptions.httpClient details (see that thread)

bioball · 2024-02-27T01:00:14Z

@translatenix: ack; I'll get back to you by end of day tomorrow. Thanks again for your work here, and bear with us as we work through all the PRs (including our own).

translatenix · 2024-02-27T20:02:11Z

I've now implemented spi.v2 to support certificate files (and testPort in #227 ) without introducing breaking changes.
I've also made sure that pkl-certs gets published.

This is ready to be merged from my side.
I'd prefer to defer non-essential improvements to this PR until the build has been stabilized (#227 ).
Stabilizing the build should be the highest priority.

Moving to java.net.http.HttpClient brings many benefits, including HTTP/2 support and the ability to make asynchronous requests. Major additions and changes: - Introduce a lightweight org.pkl.core.http.HttpClient API. This keeps some flexibility and allows to enforce behavior such as setting the User-Agent header. - Provide an implementation that delegates to java.net.http.HttpClient. - Use HttpClient for all HTTP(s) requests across the codebase. This required adding an HttpClient parameter to constructors and factory methods of multiple classes, some of which are public APIs. - Manage CA certificates per HTTP client instead of per JVM. This makes it unnecessary to set JVM-wide system/security properties and default SSLSocketFactory's. Each HTTP client maintains its own connection pool and SSLContext. For efficiency reasons, I've tried to reuse clients whenever feasible. To avoid memory leaks, clients are not stored in static fields. HTTP clients are expensive to create. For this reason, EvaluatorBuilder defaults to a "lazy" client that creates the underlying java.net.http.HttpClient on the first send (which may never happen).

- fix test that was using cache dir ~/.pkl/cache - verify in one existing test that cache is populated

- throw HttpClientInitException with user-friendly error message - handle both eagerly and lazily initialized clients

- use uri instead of response.uri()/redirected when constructing ResolvedModuleKey - add docs for HttpClient implementations

- default to SSLContext.getDefault() instead of built-in certificates in HttpClient - simplify Kotlin code using `with()` - add missing semicolon in documented User-Agent header - shorten comment (topic was discussed in PR, stops silly ktfmt formatting) - move built-in certificate file to new pkl-certs subproject - use java.net.URI instead of java.net.URL for certificate files (safer) - only allow jar: and file: certificate URIs

This allows to support certificate files without introducing breaking changes. - undo breaking changes to spi.v1 - play it safe and keep spi.v2 totally independent of spi.v1 (no `ExecutorSpiOptions2 extends ExecutorSpiOptions` etc.) - run executor tests against both SPI versions - switch to `getPlatformClassLoader()` as indicated in "once we move to JDK9+" comments

bioball

No major issues. Thanks again for your contribution!

One blocking issue is to simplify the SPI v2 options (see comment below)

bioball · 2024-02-27T00:49:39Z

pkl-core/src/main/java/org/pkl/core/http/HttpClientBuilder.java

+  }
+
+  public HttpClient.Builder addDefaultCliCertificates() {
+    var directory = userHome.resolve(".pkl").resolve("cacerts");


Suggested change

var directory = userHome.resolve(".pkl").resolve("cacerts");

var directory = IoUtils.getPklHomeDir().resolve("cacerts");

can't use IoUtils.getPklHomeDir() here because userHome is configurable to enable testing

Ah, I see. How about:

HttpClientBuilder() { this(IoUtils.getPklHomeDir()); } HttpClientBuilder(Path pklHomeDir) { this.pklHomeDir = pklHomeDir }

bioball · 2024-02-27T22:34:50Z

pkl-commons-cli/src/main/kotlin/org/pkl/commons/cli/CliCommand.kt

@@ -158,6 +152,10 @@ abstract class CliCommand(protected val cliOptions: CliBaseOptions) {
    )
  }

+  // share HTTP client with other commands with the same cliOptions


This is true for the rest of the protected members of this class.

Suggested change

// share HTTP client with other commands with the same cliOptions

This is true for the rest of the protected members of this class.

I don't see why. It's true for httpClient because it delegates to cliOptions.httpClient.

What I mean is that all of the protected members exist to be shared with other commands. It's a tad unncessary that this one is called out in particular as being shared.

It’s the same instance that’s shared, which is especially relevant for HttpClient and not generally the case for other protected members.

bioball · 2024-02-27T23:05:34Z

pkl-commons-cli/src/main/kotlin/org/pkl/commons/cli/CliBaseOptions.kt

+  /**
+   * The HTTP client shared between CLI commands created with this [CliBaseOptions] instance.
+   *
+   * To release the resources held by the HTTP client in a timely manner, call its `close()` method.


I'm proposing that both here, and in evaluator builder, we provide knobs for addCertificates, and possibly any other options on HttpClient.Builder.

After playing around with this locally, though, I don't feel strongly about this, and am okay with how you are doing it.

bioball · 2024-02-27T23:12:35Z

pkl-core/src/main/java/org/pkl/core/http/HttpClient.java

+ *
+ * <p>To create a new HTTP client, use a {@linkplain #builder() builder}. To send {@linkplain
+ * HttpRequest requests} and retrieve their {@linkplain HttpResponse responses}, use {@link #send}.
+ * To release resources held by the client in a timely manner, use {@link #close}.


[nit] the wording is slightly vague

Suggested change

* To release resources held by the client in a timely manner, use {@link #close}.

* To release resources held by the client, use {@link #close}.

I'm proposing that both here, and in evaluator builder, we provide knobs for addCertificates, and possibly any other options on HttpClient.Builder.

There's already a CliBaseOptions.caCertificates constructor parameter. There is currently no builder for CliBaseOptions.
Adding convenience methods to EvaluatorBuilder is possible, but let's defer this. I could do this in #197 , which already adds a couple of convenience methods to EvaluatorBuilder.

the wording is slightly vague

It's because the underlying JDK client only offers weak guarantees, and at least connections will eventually expire even if close() isn't called. (Before JDK 21 there isn't even a close() method.) But I'll remove this nevertheless.

bioball · 2024-02-27T23:20:32Z

pkl-core/src/main/java/org/pkl/core/http/JdkHttpClient.java

+    var methodType = MethodType.methodType(void.class, java.net.http.HttpClient.class);
+    MethodHandle result;
+    try {
+      //noinspection JavaLangInvokeHandleSignature
+      result =
+          MethodHandles.publicLookup()
+              .findVirtual(java.net.http.HttpClient.class, "close", methodType);
+    } catch (NoSuchMethodException | IllegalAccessException e) {
+      // use no-op close method
+      result = MethodHandles.empty(methodType);
+    }


Ah, missed that!

bioball · 2024-02-27T23:25:59Z

pkl-core/src/main/java/org/pkl/core/http/LazyHttpClient.java

+import javax.annotation.concurrent.ThreadSafe;
+
+@ThreadSafe
+class LazyHttpClient implements HttpClient {


We need to detail this somewhere, but--PackageResolver isn't a public API. But, that's no excuse; there should be doc comments there too!

bioball · 2024-02-27T23:26:16Z

pkl-core/src/main/java/org/pkl/core/http/RequestRewritingClient.java

+    if (!closed.getAndSet(true)) delegate.close();
+  }
+
+  // Based on JDK 17's implementation of HttpRequest.newBuilder(HttpRequest, filter).


Ah, I see. Thanks!

pkl-core/src/main/java/org/pkl/core/service/ExecutorSpiImpl.java

bioball · 2024-02-28T00:21:58Z

pkl-executor/src/main/java/org/pkl/executor/spi/v2/ExecutorSpiOptions2.java

+import java.util.List;
+import java.util.Map;
+
+public class ExecutorSpiOptions2 {


For just adding more options, let's make this part of the v1 SPI, but just as an additional class that extends ExecutorSpiOptions. This simplifies the SPI stuff quite a bit.

I've prepared a diff that demonstrates the changes that I'm talking about here (not fully tested):

https://gist.github.com/bioball/c1399aabc99e44103408085f0d03133a

I don’t think this works. Classes in the spi package are loaded by the class loader that loads pkl-executor classes (see PklDistributionClassloader.loadClass). If you have an old pkl-executor that only has v1.ExecutorSpiOptions, and a newer pkl-core that tries to instantiate v1.ExecutorSpiOptions2, you’ll get a NoClassDefFoundError.

SPI versioning is tricky business. I think it’s best to keep everything strictly separated even if it means some code duplication. I tried making v2.ExecutorSpiOptions2 extend v1.ExecutorSpiOptions, which I think should work, but it didn’t help all that much, and I didn’t feel it was worth the risk and version entanglement.

Note that going from 2 to 3 versions will be easier than going from 1 to 2 because I more or less added the infrastructure needed to support n versions. The code duplication is somewhat ugly, but the duplicated code is pretty dumb.

We need to detail this somewhere, but--PackageResolver isn't a public API.

Currently it is, by definition:

EvaluatorBuilder is a public API. It has a public method setProjectDependencies(org.pkl.core.project.DeclaredDependencies) -> org.pkl.core.project is a public API.

org.pkl.core.project.ProjectDependenciesResolver has a public constructor that takes org.pkl.core.packages.PackageResolver -> org.pkl.core.packages is a public API.

PackageResolver.getDependencyMetadataAndComputeChecksum returns org.pkl.core.util.Pair -> org.pkl.core.util is a public API.

Formalizing the public API by adding support for JPMS (#42) will be a good exercise. It will also likely require breaking API changes. It's something I'd be interested to work on.

I don’t think this works. Classes in the spi package are loaded by the class loader that loads pkl-executor classes (see PklDistributionClassloader.loadClass). If you have an old pkl-executor that only has v1.ExecutorSpiOptions, and a newer pkl-core that tries to instantiate v1.ExecutorSpiOptions2, you’ll get a NoClassDefFoundError.

Right; you won't be able to use a newer Pkl version with this approach. But, it's backwards compatible with older Pkl distributions. But, we actually have this problem regardless with your approach too?

Tested via:

In branch http-client: ./gradlew :pkl-config-java:build

In branch main: Add this test and run it:

diff --git a/pkl-executor/src/test/kotlin/org/pkl/executor/EmbeddedExecutorTest.kt b/pkl-executor/src/test/kotlin/org/pkl/executor/EmbeddedExecutorTest.kt index c9495631b..907df7bf1 100644 --- a/pkl-executor/src/test/kotlin/org/pkl/executor/EmbeddedExecutorTest.kt +++ b/pkl-executor/src/test/kotlin/org/pkl/executor/EmbeddedExecutorTest.kt @@ -13,6 +13,7 @@ import java.nio.file.Files import java.nio.file.Path import java.time.Duration import kotlin.io.path.createDirectories +import kotlin.io.path.writeText class EmbeddedExecutorTest { private val pklDistribution by lazy { @@ -39,6 +40,33 @@ class EmbeddedExecutorTest { } } + @Test + fun testStuff(@TempDir tempDir: Path) { + val executor = Executors.embedded( + listOf(FileTestUtils.rootProjectDir.resolve("pkl-config-java/build/libs/pkl-config-java-all-0.26.0-SNAPSHOT.jar")) + ) + val file = tempDir.resolve("test.pkl").also { + it.writeText(""" + @ModuleInfo { minPklVersion = "0.26.0" } + module foo + + foo = 1 + """.trimIndent()) + } + executor.evaluatePath(file, ExecutorOptions( + listOf("file:"), + listOf("prop:"), + mapOf(), + mapOf(), + listOf(), + tempDir, + null, + null, + null, + null + )) + } + @Test fun extractMinPklVersion() { assertThat(

Produces error: java.lang.NoClassDefFoundError: org/pkl/executor/spi/v2/ExecutorSpi2

FWIW: the approach that I outlined is how we managed new SPI versions prior to open sourcing Pkl. We got rid of all the different variations when we open sourced because, might as well start with a clean slate.

The way I know this from other projects is that either side should be able to update without breaking the other to avoid a lock-in. If there’s a case that doesn’t work, I’m happy to fix it (and add test coverage) after the build has been stabilized.

I'd be happy to merge this PR if it didn't include these SPI changes.

Can you omit the SPI changes from this PR for now? And let's address that in a follow-up PR.

In #227, feel free to add @Disabled to tests evaluate a module that loads a package and evaluate a project dependency so we can stabilize the build without needing to bring in SPI changes.

You mean to neither add spi.v2 nor change spi.v1 to pass certificates? Then I’ll also need to disable some tests in this PR.

Yeah; no SPI changes for this PR. And yeah, LGTM to disable those tests in this PR too.

How about accepting breaking spi.v1 changes for now. That requires far fewer changes, and this will need to be fixed anyway.

I know this is higher friction for you, but, I'd prefer to not merge something into main that isn't releasable. If it feels like too much churn for you, I can prepare a version of your PR that excludes the SPI changes (and same with #227)

pkl-certs/pkl-certs.gradle.kts

- fix pkl-certs description - build executor `HttpClient`s lazily - polish docs

translatenix · 2024-02-28T03:38:23Z

I've addressed everything I could in my latest commit (as per my comments). Once you're ready to merge, please squash before merging (or ask me to squash).

bioball · 2024-02-28T22:32:31Z

I've addressed everything I could in my latest commit (as per my comments). Once you're ready to merge, please squash before merging (or ask me to squash).

We will definitely squash when merging

- Remove spi.v2 and switch to the implementation strategy proposed in the review: spi.v1.ExecutorSpiOptions2 now extends spi.v1.ExecutorSpiOptions1. - Add ExecutorOptions2 to avoid breaking changes in the pkl-executor API. - Tweak PklDistributionClassLoader to support using an older pkl-executor with a newer Pkl distribution if the Pkl distribution contains pkl-executor SPI classes. To de-risk this commit, distributions aren't changed to contain these classes. - Improve EmbeddedExecutorTest to run most tests against all combinations of older/newer pkl-executor and older/newer Pkl distribution. Testing of older pkl-executor and newer Pkl distribution can be enabled when a distribution containing SPI classes becomes available.

translatenix · 2024-02-29T15:02:43Z

Instead of backing out all SPI changes, I switched to your suggested implementation approach.
To gain some confidence in this approach, I massively improved EmbeddedExecutorTest.
Details in the commit message.

If you prefer to back out all SPI changes, I'll take you up on your offer to take over from here.

bioball

Some nits, but overall LGTM. Great work!

Passing over to @stackoverflow for review

bioball · 2024-02-29T21:47:57Z

pkl-executor/src/main/java/org/pkl/executor/EmbeddedExecutor.java

+              // so load it from the distribution.
+              // This can happen if the pkl-executor version is lower than the distribution version.
+              clazz = findClass(name);
+            }


Neat solution!

bioball · 2024-02-29T22:02:00Z

pkl-executor/pkl-executor.gradle.kts

+  src("https://repo1.maven.org/maven2/org/pkl-lang/pkl-config-java-all/0.25.0/pkl-config-java-all-0.25.0.jar")
+  dest("build/download/pkl-config-java-all-0.25.0.jar")
+  doFirst { file("build/download").mkdirs() }
+}


[nit] this is a little nicer:

val pklHistoricalDistributions: Configuration by configurations.creating dependencies { @Suppress("UnstableApiUsage") pklHistoricalDistributions(libs.pklConfigJavaAll025) } val prepareHistoricalDistributions by tasks.registering(Copy::class) { from(pklHistoricalDistributions) into(layout.buildDirectory.dir("pklDistributions")) } val prepareTest by tasks.registering { dependsOn(prepareHistoricalDistributions, pklDistribution) }

Ack. I went with a solution that doesn't copy. Switching to IntelliJ's Gradle test runner would simplify matters, but it's 10x slower than the already super slow built-in test runner on WSL.

Can we keep the copy solution?

This piece of code seems too brittle:

System.getProperty("user.home").toPath() .resolve(".gradle/caches/modules-2/files-2.1/org.pkl-lang/pkl-config-java-all/" + "0.25.0/e9451dda554f1659e49ff5bdd30accd26be7bf0f/pkl-config-java-all-0.25.0.jar")

It's supposed to be deterministic and is only used when running tests with IntelliJ's built-in test runner. The main problem with copying is that it requires gw prepTest after every gw clean somethingThatDoesntRunPrepTest, which is quite annoying.

Would be best to move to IntelliJ's Gradle test runner once developing on Windows is possible.

Using IJ's JUnit run configurations have always been the better experience for me, over the Gradle run configurations. I actually like gw prepTest as a way to bridge the gap for us here.

bioball · 2024-02-29T22:29:40Z

pkl-core/src/main/java/org/pkl/core/service/ExecutorSpiImpl.java

+    if (options instanceof ExecutorSpiOptions2) {
+      var options2 = (ExecutorSpiOptions2) options;
+      certificateFiles = options2.getCertificateFiles();
+      certificateUris = options2.getCertificateUris();
+    } else {


To handle if pkl-executor is older than this distribution:

Suggested change

if (options instanceof ExecutorSpiOptions2) {

var options2 = (ExecutorSpiOptions2) options;

certificateFiles = options2.getCertificateFiles();

certificateUris = options2.getCertificateUris();

} else {

try {

if (options instanceof ExecutorSpiOptions2) {

var options2 = (ExecutorSpiOptions2) options;

certificateFiles = options2.getCertificateFiles();

certificateUris = options2.getCertificateUris();

} else {

certificateFiles = List.of();

certificateUris = List.of();

}

} catch (NoClassDefFoundError e) {

And we can enable the test if the executor is older than the distribution version; no need to ship SPI classes with pkl-core.

This is really testing the limits, and I can't actually get it to work. I think it's far safer to ship pkl-executor with pkl-config-java-all or some other distribution.

There may be a solution that doesn’t require including pkl-executor in distributions. For now, let's leave the “older pkl-executor, newer distribution” test disabled.

This change is passing for me:

diff --git a/pkl-core/src/main/java/org/pkl/core/service/ExecutorSpiImpl.java b/pkl-core/src/main/java/org/pkl/core/service/ExecutorSpiImpl.java index 3af2df3eb..1c713cb76 100644 --- 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,11 +132,16 @@ public class ExecutorSpiImpl implements ExecutorSpi { private HttpClient getOrCreateHttpClient(ExecutorSpiOptions options) { List<Path> certificateFiles; List<URI> certificateUris; - if (options instanceof ExecutorSpiOptions2) { - var options2 = (ExecutorSpiOptions2) options; - certificateFiles = options2.getCertificateFiles(); - certificateUris = options2.getCertificateUris(); - } else { + try { + if (options instanceof ExecutorSpiOptions2) { + var options2 = (ExecutorSpiOptions2) options; + certificateFiles = options2.getCertificateFiles(); + certificateUris = options2.getCertificateUris(); + } else { + certificateFiles = List.of(); + certificateUris = List.of(); + } + } catch (NoClassDefFoundError e) { certificateFiles = List.of(); certificateUris = List.of(); } diff --git a/pkl-executor/src/test/kotlin/org/pkl/executor/EmbeddedExecutorTest.kt b/pkl-executor/src/test/kotlin/org/pkl/executor/EmbeddedExecutorTest.kt index 3658b3ead..46e014274 100644 --- a/pkl-executor/src/test/kotlin/org/pkl/executor/EmbeddedExecutorTest.kt +++ b/pkl-executor/src/test/kotlin/org/pkl/executor/EmbeddedExecutorTest.kt @@ -39,7 +39,7 @@ class EmbeddedExecutorTest { // This context has a pkl-executor version that is lower than the distribution version. // It can be enabled once there is a distribution that includes pkl-executor. - //ExecutionContext(executor1_2.value, ::convertToOptions1, "Options1, Executor1, Distribution2"), + ExecutionContext(executor1_2.value, ::convertToOptions1, "Options1, Executor1, Distribution2"), ExecutionContext(executor2_1.value, ::convertToOptions1, "Options1, Executor2, Distribution1"), ExecutionContext(executor2_1.value, ::convertToOptions2, "Options2, Executor2, Distribution1"),

Didn't work for me, but maybe IntelliJ didn't pick up my code change. (I've been struggling with this a lot, probably WSL related.) I don't know if NoClassDefFoundError is guaranteed to be only thrown here though. It's a fairly extreme solution.

- improve how tests get access to Pkl distributions

stackoverflow

Looks good. Left some comments.

stackoverflow · 2024-03-01T16:08:05Z

pkl-core/src/main/java/org/pkl/core/http/RequestRewritingClient.java

+                  builder.GET();
+                  break;
+                case "DELETE":
+                  builder.DELETE();


Any reason why GET and DELETE receive special treatment here?

When a builder and the getters of the object it builds aren't symmetric, copying becomes more difficult. Copied from JDK 17 code.

stackoverflow · 2024-03-01T16:32:15Z

pkl-core/src/main/java/org/pkl/core/util/Exceptions.java

+  public static Throwable getRootCause(Throwable t) {
+    var result = t;
+    var cause = result.getCause();
+    while (cause != null) {
+      result = cause;
+      cause = cause.getCause();
+    }
+    return result;
+  }


Suggested change

public static Throwable getRootCause(Throwable t) {

var result = t;

var cause = result.getCause();

while (cause != null) {

result = cause;

cause = cause.getCause();

}

return result;

}

public static Throwable getRootCause(Throwable t) {

var result = t;

while (result.getCause() != null) {

result = result.getCause();

}

return result;

}

stackoverflow · 2024-03-04T13:19:59Z

pkl-executor/src/test/kotlin/org/pkl/executor/EmbeddedExecutorTest.kt

+    }
+
+    // A pkl-executor library that supports ExecutorSpiOptions up to v2
+    // and a Pkl distribution that supports ExecutorSpiOptions up to v.


Suggested change

// and a Pkl distribution that supports ExecutorSpiOptions up to v.

// and a Pkl distribution that supports ExecutorSpiOptions up to v2.

bioball · 2024-03-05T22:52:43Z

@translatenix: are you planning on addressing the nits here? If so, I'll wait for them before merging. Otherwise, we can go ahead and merge, and we will submit a follow-up cleanup PR.

bioball · 2024-03-06T19:10:05Z

Cleanup PR here: #295

translatenix force-pushed the http-client branch from 41563d2 to 7a26325 Compare February 20, 2024 21:53

translatenix force-pushed the http-client branch 3 times, most recently from 1f8d8f0 to a01b108 Compare February 22, 2024 00:42

translatenix mentioned this pull request Feb 22, 2024

Bind PackageServer to ephemeral port to avoid port conflicts #227

Merged

translatenix force-pushed the http-client branch from 93b2dde to 47674e7 Compare February 22, 2024 15:44

holzensp reviewed Feb 22, 2024

View reviewed changes

bioball requested changes Feb 23, 2024

View reviewed changes

translatenix requested a review from bioball February 23, 2024 04:09

translatenix force-pushed the http-client branch 2 times, most recently from b26b91f to 96ad5eb Compare February 23, 2024 17:12

translatenix force-pushed the http-client branch 2 times, most recently from 17f95e0 to 41cb695 Compare February 23, 2024 23:33

translatenix added 9 commits February 27, 2024 12:30

Support CA certificates in Executor API

dc78962

- fix test that was using cache dir ~/.pkl/cache - verify in one existing test that cache is populated

Build CLI HTTP client lazily

903f141

Improve handling of HTTP client initialization errors

1ca9814

- throw HttpClientInitException with user-friendly error message - handle both eagerly and lazily initialized clients

Improve SSL related error messages

765b439

Rename class

988481f

Rename some internals

0022ea3

Remove assert that is specific to JDK implementation

0b3734d

Fix compile error on JDK 11

8f8ac86

translatenix added 8 commits February 27, 2024 12:30

Incorporate review feedback

b12db60

Incorporate review feedback (more to follow later)

a240d4c

- use uri instead of response.uri()/redirected when constructing ResolvedModuleKey - add docs for HttpClient implementations

Polish error message

ed1f6ac

Make SSL error messages more robust

e16a8e0

Improve error message on JDK 11

9b696b8

Publish pkl-certs JAR

09be2f4

translatenix force-pushed the http-client branch from bfd58a3 to 8d78d28 Compare February 27, 2024 20:32

Reuse EmbeddedExecutor instances for all tests

fdf2026

bioball requested changes Feb 28, 2024

View reviewed changes

Incorporate review feedback

cc25583

- fix pkl-certs description - build executor `HttpClient`s lazily - polish docs

Polish EmbeddedExecutorTest

85103c6

bioball previously approved these changes Feb 29, 2024

View reviewed changes

bioball assigned stackoverflow and bioball Feb 29, 2024

Incorporate review feedback

06f94fd

- improve how tests get access to Pkl distributions

translatenix dismissed bioball’s stale review via 06f94fd March 1, 2024 01:45

stackoverflow approved these changes Mar 4, 2024

View reviewed changes

bioball merged commit 3f3dfde into apple:main Mar 6, 2024
4 checks passed

	var directory = userHome.resolve(".pkl").resolve("cacerts");
	var directory = IoUtils.getPklHomeDir().resolve("cacerts");

	* To release resources held by the client in a timely manner, use {@link #close}.
	* To release resources held by the client, use {@link #close}.

	// and a Pkl distribution that supports ExecutorSpiOptions up to v.
	// and a Pkl distribution that supports ExecutorSpiOptions up to v2.

Use java.net.http.HttpClient instead of java.net.Http(s)URLConnection #217

Use java.net.http.HttpClient instead of java.net.Http(s)URLConnection #217

Conversation

translatenix commented Feb 20, 2024 • edited Loading

bioball commented Feb 21, 2024

translatenix commented Feb 21, 2024

bioball commented Feb 21, 2024

translatenix commented Feb 21, 2024

holzensp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

translatenix Feb 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

translatenix Feb 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bioball Feb 27, 2024 • edited Loading

Choose a reason for hiding this comment

translatenix Feb 27, 2024 • edited Loading

Choose a reason for hiding this comment

bioball Feb 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

translatenix Feb 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

translatenix Feb 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bioball left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

translatenix Feb 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

translatenix Feb 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

translatenix commented Feb 23, 2024

translatenix commented Feb 26, 2024

bioball commented Feb 27, 2024

translatenix commented Feb 27, 2024

bioball left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

translatenix Feb 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

translatenix commented Feb 20, 2024 •

edited

Loading

translatenix Feb 22, 2024 •

edited

Loading

translatenix Feb 22, 2024 •

edited

Loading

bioball Feb 27, 2024 •

edited

Loading

translatenix Feb 27, 2024 •

edited

Loading

bioball Feb 27, 2024 •

edited

Loading

translatenix Feb 22, 2024 •

edited

Loading

translatenix Feb 22, 2024 •

edited

Loading

translatenix Feb 23, 2024 •

edited

Loading

translatenix Feb 23, 2024 •

edited

Loading

translatenix Feb 28, 2024 •

edited

Loading

translatenix Feb 28, 2024 •

edited

Loading

bioball Feb 27, 2024 •

edited

Loading

translatenix Feb 28, 2024 •

edited

Loading