pkl
pkl copied to clipboard
Use java.net.http.HttpClient instead of java.net.Http(s)URLConnection
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.
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/[email protected]`:
PKIX path building failed: sun.security.provider.certpath.SunCertPathBuilderException: unable to find valid certification path to requested target
4 | import "package://localhost:12110/[email protected]#/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)
Unfortunately I can't reproduce locally. Is it possible to trigger PR builds automatically for contributors? Otherwise the feedback loop is too long.
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.
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 @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)
@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)
@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).
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.
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).
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
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.
@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.
Cleanup PR here: https://github.com/apple/pkl/pull/295