Add Resource-based simple constructors #1015
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -131,6 +131,11 @@ object JdkWSClient { | |
| }) | ||
| } yield () | ||
| } | ||
| // 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()) | ||
|
Comment on lines
+134
to
+138
There was a problem hiding this comment. Without this, the test suite hangs when using |
||
| } yield () | ||
| } | ||
| .map { case (webSocket, queue, closedDef, sendSem) => | ||
|
|
@@ -164,7 +169,17 @@ object JdkWSClient { | |
| * [[cats.effect.kernel.Async.executor executor]], sets the | ||
| * [[org.http4s.client.defaults.ConnectTimeout default http4s connect timeout]], and disables | ||
| * [[https://github.com/http4s/http4s-jdk-http-client/issues/200 TLS 1.3 on JDK 11]]. | ||
| * | ||
| * * On Java 21 and higher, prefer [[simpleResource]] as it actively closes the underlying | ||
| * client, releasing its resources early. | ||
| */ | ||
| def simple[F[_]](implicit F: Async[F]): F[WSClient[F]] = | ||
| JdkHttpClient.defaultHttpClient[F].map(apply(_)) | ||
|
|
||
| /** Like [[simple]], but wraps the client in a [[cats.effect.Resource Resource]] to ensure it's | ||
| * 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, WSClient[F]] = | ||
| JdkHttpClient.defaultHttpClientResource[F].map(apply(_)) | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As already mentioned in #1011 (comment), we could just change the signature of
simpleand cut a 0.10 release; it should be rather simple for users to migrate.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should rename
simpleandsimpleResourceto something likeunsafeDefaultanddefault. 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hamnis I can adjust the names, no problem, just a bit unsure about the
unsafeprefix. On everything below Java 21, it's actually not unsafe in the sense CE usually uses this, but the only thing you can have (theResourcevariant won't close anything there either).So maybe a 0.10 with
simple(returning aResource, by re-using the name users get nudged towards this) and adef 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we really need the forEffect constructor? Do we lose anything with just having the
simple|default|insertyournamehereresource constructor ?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hamnis It may cause some (unnecessary) trouble for users who now need to bubble up
Resourceusage in case they hadn't so far. But asallocatedis safe on Java below 21, I changed the PR to only offerdef simple[F[_]: Async]: Resource[F, …]and mentionedallocatedin the docs, so people have a hint on a workaround. Hope that's a fair compromise!There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Obviously, that means MiMa is unhappy and we need to release it as
0.10.