pkl icon indicating copy to clipboard operation
pkl copied to clipboard

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

Open translatenix opened this issue 1 year ago • 12 comments

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.

translatenix avatar Feb 20 '24 21:02 translatenix

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)

bioball avatar Feb 21 '24 01:02 bioball

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

translatenix avatar Feb 21 '24 01:02 translatenix

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.

bioball avatar Feb 21 '24 04:02 bioball

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.

translatenix avatar Feb 21 '24 05:02 translatenix

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

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

translatenix avatar Feb 23 '24 20:02 translatenix

@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:

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

translatenix avatar Feb 26 '24 19:02 translatenix

@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).

bioball avatar Feb 27 '24 01:02 bioball

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.

translatenix avatar Feb 27 '24 20:02 translatenix

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).

translatenix avatar Feb 28 '24 03:02 translatenix

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

bioball avatar Feb 28 '24 22:02 bioball

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 avatar Feb 29 '24 15:02 translatenix

@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 avatar Mar 05 '24 22:03 bioball

Cleanup PR here: https://github.com/apple/pkl/pull/295

bioball avatar Mar 06 '24 19:03 bioball