Replace `java.net.HTTPUrlConnection` with `java.net.http.HttpClient` (500USD Bounty)

[lihaoyi]

This would fix #123, and put us in a better state going forward.

HttpClient was standardized on Java 11, which is probably old enough we can rely on it already

HttpClient provides an async API, and we should expose it as requests.get.async, requests.post.async, etc.. Apart from that, the rest of the API and test suite should be unchanged. All existing tests should pass, with any unavoidable failures or necessary changes in the tests called out in the ticket

This should also resolve #123

To incentivize contribution, I'm putting a 500USD bounty on resolving this ticket. This is payable via bank transfer, and at my discretion in case of ambiguity.

[nafg]

I'll take a stab at this now

[nafg]

HTTPUrlConnection has you write the request body by exposing an OutputStream, and RequestBlob is designed around this.

OTOH with HttpClient you pass a BodyPublisher, which is an abstraction analogous to RequestBlob other than the headers, that has helpers given you have a byte array or InputStream or other common cases. Otherwise it's based on java.util.concurrent.Flow.Publisher so you can wrap one of those or extend it.

Can I change the RequestBlob API, or do we have to work with it? In particular I'm not sure how to adapt it to a BodyPublisher without either loading it all into memory or starting another thread.

TBH I'm not sure why it has to take something as powerful as Publisher which can have multiple subscribers.

[lihaoyi]

@nafg does HttpResponse.BodyHandlers.ofInputStream do what we want? At at glance it seems to do the right thing - streaming the data and making it available to whoever reads the stream as it downloads.

[nafg]

@nafg does HttpResponse.BodyHandlers.ofInputStream do what we want? At at glance it seems to do the right thing - streaming the data and making it available to whoever reads the stream as it downloads.

I guess you mean BodyPublisher (BodyHandler would be for the response body). It does "the right thing" as you say, but I don't know how to adapt a RequestBlob to it (without reading it into memory or starting another thread).

[nafg]

As I said, the cleanest implementation would be to completely break the RequestBlob API. (Of course we would still support implicitly adapting the same types of values.)

[nafg]

Maybe I could just create a connected PipedOutputStream and PipedInputStream and pass the former to data.write and the latter to BodyPublisher.ofInputStream. But where would write be called?

[lihaoyi]

Sorry, I mixed up BodyHandler and BodyPublisher.

If I understand the problem right, it is that RequestBlob exposes a push-based def write(out: java.io.OutputStream): Unit for people to implement, whereas HttpRequest.BodyPublisher.ofInputStream requires a pull-based Supplier[? extends InputStream], and the pull-based approach is fundamentally harder to implement that the original push-based approach and thus incompatible.

Making RequestBlob pull-based rather than push-based is a no-go. Pull-based APIs are much harder to implement than pull-based APIs, and it would mean the Writables coming from e.g. uPickle or Scalatags or other libraries would not be compatible with this API. Making uPickle, Scalatags, etc. support pull-based serialization would be a full rewrite and is unfeasible

Spawning a thread is an option, but an expensive one. What if we instead use the Java HTTPClient's sendAsync, and then use the calling thread to call RequestBlob#write and block on it? The calling thread isn't doing anything anyway when using sendAsync, so it costs us nothing to use it to call .write. That way we don't need to pay the cost of a new thread while the calling thread sits idle

I'm actually not sure exactly how the async concurrency story works with java.net.http.HttpClient. Does it need an Executor, ExecutionContext, or Threadpool of some sort? We need to sort that out as part of this ticket so we can understand what the consequences of this replacement will be

[nafg]

I'm actually not sure exactly how the async concurrency story works with java.net.http.HttpClient. Does it need an Executor, ExecutionContext, or Threadpool of some sort? We need to sort that out as part of this ticket so we can understand what the consequences of this replacement will be

Yeah, the builder takes an Executor IIRC

[nafg]

Spawning a thread is an option, but an expensive one. What if we instead use the Java HTTPClient's sendAsync, and then use the calling thread to call RequestBlob#write and block on it? The calling thread isn't doing anything anyway when using sendAsync, so it costs us nothing to use it to call .write. That way we don't need to pay the cost of a new thread while the calling thread sits idle

Does that mean that we give it an InputStream that will block until we write to the OutputStream? Not sure I follow.

[lihaoyi]

I'm not totally sure myself TBH. But if you said you can solve the problem by spawning a thread, it seems like you should be able to solve the problem by using sendAsync and taking advantage of the calling thread.

[nafg]

Do I need to handle chunkedUpload==false

[nafg]

I don't see an equivalent of connection.getResponseMessage

[lihaoyi]

@nafg You'll have to dig through the code and tell me what the tradeoffs and necessary changes are

[nafg]

Re response message (reason phrase) see https://stackoverflow.com/a/63576759/333643

Some points from there: HTTP 2 and some HTTP 1 servers don't provide it, HttpClient has no API to get it, and it doesn't really have any purpose.

Two options:

1. Preserve compatibility using a table of strings per status code
2. Remove the field from case class Response