Use JDK 11 HttpClient

[nafg]

Fix #150

• Switches from HttpURLConnection to JDK 11's HttpClient
• statusMessage no longer provides access to the "reason phrase" sent by the server. Instead a hardcoded Map is used.

[lihaoyi]

@nafg you'll need to bump the Java 8 minimum version to Java 11 I think

Could you also write up a PR description summarizing the major changes/decisions/etc.

[nafg]

@nafg you'll need to bump the Java 8 minimum version to Java 11 I think

Could you also write up a PR description summarizing the major changes/decisions/etc.

Sure. I need to clean up the code and commits too.

[nafg]

Also, I haven't implemented the async API yet

[nafg]

Am I supposed to update something for the bincompat check? Or do you just ignore it when you want to do a breaking release?

[lihaoyi]

@nafg let's ignore it for now, once things are settled we can see how easy it is to maintain bincompat or whether we should just release a new breaking version

[nafg]

@nafg let's ignore it for now, once things are settled we can see how easy it is to maintain bincompat or whether we should just release a new breaking version

I think the code is settled enough to decide this. Here are the breaking changes as I recall:

1. statusMessage is removed from Response and StreamHeaders. We could preserve it by having a fixed Map of status codes to messages, but there's no way to get it from the HttpClient API. I guess we could mark it deprecated. It's your call whether we care about breaking the code of the (hopefully few) people that actually use requestMessage. But I suspect you prefer a failed compilation to a potentially silent breakage, for instance if someone is actually using a server that puts meaningful text in the reason phrase that they have to handle. In such a case they have to know that upgrading will necessitate revisiting that architecture.
2. RequestBlob now has a contentLength: Option[Long] instead of putting it directly in the headers, because HttpClient doesn't allow you to.

Actually, at this point, headers is only used for Content-Type, which means that in terms of interface it's equivalent to Writable. So maybe we should drop RequestBlob and just use geny.Writable.

If we want to be backwards compatible we could leave RequestBlob as is, introspect headers, and remove Content-Length if it's there. But I don't think that's a good API to have.

If we mainly care about source compatibility we could leave RequestBlob as is, change data to take a geny.Writable instead of a RequestBlob, and have an implicit conversion so that users can continue to pass in RequestBlobs if they were doing so explicitly.

If we don't want to define implicit conversions to Writable instead of RequestBlob, or we can't because of implicit scope rules, we could have a new data type with some other name that literally just wraps a Writable, includes the same implicit conversions as RequestBlob plus a conversion from RequestBlob, and then the data parameter will be of that type.

It would still be a binary breaking change because we've changed the type of data, but it would be at least almost 100% source compatible, I think.

[nafg]

I should update the commit messages and the README, and add the async APIs, but aside for the discussion questions I think I'm happy with the code (although I can look it over again).

[lihaoyi]

At a first glance, it looks reasonable. I did a cursory pass on the code, but if you could write up the PR description about the major things a reviewer should know of the approach that would help a lot

Binary compatibility is more important than source compatible (due to potentially-unsolvable diamond dependency problems), so if it's not too hard to preserve I'd say we go with that.

I think a fixed map of statusMessages, defaulting to Unknown for those outside the standard list, is the way to go. This solves the 99% scenario of users using standard HTTP response codes and statusMessages allowing them to upgrade smoothly, and limits the breakage to the tiny minority of people putting custom stuff in the statusMessage

I think we should leave RequestBlob as is to minimize breakage. Even if it requires a bit of internal gymnastics, it's probably worth it for the smooth upgrades. Trying to change the API or swap over to geny.Writable all will cause bincompat breakage and inconvenience; we can leave a TODO and do it later if necessary

If it's not too onerous, then getting check-binary-compatibility green would be a great indication of whether maintaining bincompat is possible, and how much contortions we need to go through to do so. Let's do that first, and if it proves difficult/impossible we can reconsider

[lihaoyi]

@nafg bump on this :) just making sure you didn't miss my response

[nafg]

Is there a reason why blobHeaders is passed separately from data?

[nafg]

Any why in one case it's passed in as data.headers but in another place as Seq.empty?

[nafg]

Oops, I defaulted the statusMessage to "" instead of "Unknown", is that ok?

[nafg]

I'm getting a lot of

WARNING: Certificate may have expired and needs to be updated. Please check: https://github.com/lihaoyi/requests-scala/blob/master/requests/test/resources/badssl.com-client.md and/or file issue

[nafg]

Note that I deprecated statusMessage with a "since" of "0.9.0"; I assumed that was the plan.

[nafg]

I updated the PR description. Is that enough detail or should I go into the internal changes?

P.S. the status messages were generated with ChatGPT.

[nafg]

Note: I still didn't implement the async API

[lihaoyi]

Is there a reason why blobHeaders is passed separately from data?

blobHeaders come from #26. It appears to be meant to handle the headers that come from the RequestBlob object

Any why in one case it's passed in as data.headers but in another place as Seq.empty?

I suspect the place where it's passed as Seq.empty is a bug? Just that def stream(r: Request) probably isn't called much so probably nobody noticed

Oops, I defaulted the statusMessage to "" instead of "Unknown", is that ok?

I think that's fine

Note that I deprecated statusMessage with a "since" of "0.9.0"; I assumed that was the plan.

Yes that's fine

I updated the PR description. Is that enough detail or should I go into the internal changes?

Do go into the internal changes as well; the PR description is for future maintainers too, not just for downstream library users

P.S. the status messages were generated with ChatGPT.

Is it possible to get this list from a more authoritative source, like some RFC? We should add a comment with a link to the source so people know where it came from

Note: I still didn't implement the async API

Is it easy to implement? From the sound of it it's mostly the same as the sync API, just that it needs a ExecutionContext or similar to call .write for us right?

[lihaoyi]

If async proves tricky, I think I would be fine dropping that as a requirement for this ticket and bounty. Just the synchronous API should be enough

[nafg]

Is it easy to implement?

Originally I thought it was basically a matter of adapting the Java Future to a Scala Future, which is a bit harder for Scala 2.11 IIRC. But since we are still using RequestBlob / Writable which is why we are writing on the caller thread, I don't see how we are any more in a position to have an async API than before.

From the sound of it it's mostly the same as the sync API, just that it needs a ExecutionContext or similar to call .write for us right?

I mean how is that different than just having the caller wrap the call in Future { ... }? The library is still completely blocking.

I wonder if you'd be interested in a call to discuss some of the design decisions and tradeoffs?

[lihaoyi]

Let's drop async for now.

Maybe we can pick it up in a follow up bounty, but right now it's orthogonal to the core requirement of getting onto java HttpClient and will require more invasive hanges to the API that will take more time to consider

[lihaoyi]

@nafg looks like CI is green, if you update the PR description and I give it another round of review then we can merge it and close out the bounty

[nafg]

I don't really understand what else to write

[lihaoyi]

@nafg that's fine, if you're done with this I can merge it and close this out

[nafg]

I think so...

I would suggest that because of the nature of the change it might be worth having an RC period, even though I don't think most of your libraries usually do that. What if there's some implementation difference between HttpURLConnection and JDK http client that affects users that we aren't aware of, and fixing it really does require a breaking change?