Add Resource-based simple constructors #1015
Conversation
As Java 21+ allows explicitly closing clients, this adds new constructors that return the clients wrapped in a Resource. On older Java versions, the release is a no-op, on newer it enforces the cleanup of resources. Also ensures the websocket client closes the input channel on release so the connection gets properly closed. Closes http4s#1011
| // If the input side is still open (no close received from server), the JDK will not clean up the connection. | ||
| // This also implies the client can't be shutdown on Java 21+ as it waits for all open connections | ||
| // to be be closed. As we don't expect/handle anything coming on the input anymore | ||
| // at this point, we can safely abort. | ||
| _ <- F.delay(webSocket.abort()) |
There was a problem hiding this comment.
Without this, the test suite hangs when using simpleResource as the connections aren't closed. I'm no expert in web sockets, so this may have negative side effects. #853 tackles the same problem with a similar, but more elaborated approach, so it might be better to merge that one first.
| F: Async[F] | ||
| ): Resource[F, HttpClient] = | ||
| Resource.make[F, HttpClient](defaultHttpClient[F]) { | ||
| case c: AutoCloseable => Sync[F].blocking(c.close()) |
There was a problem hiding this comment.
This is the most relevant bit, actively closing if HttpClient <: AutoCloseable which is the case on Java 21+.
| * properly shut down on JVM 21 and higher. On lower Java versions, closing the resource does | ||
| * nothing (garbage collection will eventually clean up the client). | ||
| */ | ||
| def simpleResource[F[_]](implicit F: Async[F]): Resource[F, Client[F]] = |
There was a problem hiding this comment.
Name is up for discussion, I don't particular like simpleResource, but at least it's rather clear.
There was a problem hiding this comment.
As already mentioned in #1011 (comment), we could just change the signature of simple and cut a 0.10 release; it should be rather simple for users to migrate.
There was a problem hiding this comment.
I think we should rename simple and simpleResource to something like unsafeDefault and default. We should encourage users to do the right thing (using the resource variant)
We can do that in a 0.10 release, or deprecate simple in this and create new new constructors.
There was a problem hiding this comment.
@hamnis I can adjust the names, no problem, just a bit unsure about the unsafe prefix. On everything below Java 21, it's actually not unsafe in the sense CE usually uses this, but the only thing you can have (the Resource variant won't close anything there either).
So maybe a 0.10 with simple (returning a Resource, by re-using the name users get nudged towards this) and a def forEffect[F[_]: Async]: F[Client[F]] (or any better name) with docs explaining the caveats on Java 21+?
There was a problem hiding this comment.
do we really need the forEffect constructor? Do we lose anything with just having the simple | default | insertyournamehere resource constructor ?
There was a problem hiding this comment.
@hamnis It may cause some (unnecessary) trouble for users who now need to bubble up Resource usage in case they hadn't so far. But as allocated is safe on Java below 21, I changed the PR to only offer def simple[F[_]: Async]: Resource[F, …] and mentioned allocated in the docs, so people have a hint on a workaround. Hope that's a fair compromise!
There was a problem hiding this comment.
Obviously, that means MiMa is unhappy and we need to release it as 0.10.
…ore-0.23.29 Update http4s-client, ... to 0.23.29 in series/0.9
rossabaker
force-pushed
the
series/0.9
branch
from
9fca89c
to
d5cf705
Compare
| @@ -71,7 +72,7 @@ val coreDeps = Seq( | |||
| val scala213 = "2.13.15" | |||
| ThisBuild / crossScalaVersions := Seq("2.12.20", scala213, "3.3.4") | |||
| ThisBuild / scalaVersion := scala213 | |||
| ThisBuild / tlBaseVersion := "0.9" | |||
| ThisBuild / tlBaseVersion := "0.10" | |||
There was a problem hiding this comment.
There are more references to 0.9 and series/0.9, so whoever does the next release, please ensure to update accordingly before (I don't think those fit in this PR's scope).
As Java 21+ allows explicitly closing clients, this adds new constructors that return the clients wrapped in a
Resource. On older Java versions, the release is a no-op, on newer it enforces the cleanup of resources.Also ensures the websocket client closes the input channel on release so the connection gets properly closed.
Closes #1011