Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add Resource-based simple constructors #1015

Open
wants to merge 7 commits into
base: series/0.9
Choose a base branch
from
Open

Add Resource-based simple constructors #1015

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
33ca4c9
Add Resource-based simple constructors
ybasket Feb 3, 2024
0384cb9
Fix problem with unusedCompileDependenciesFilter
ybasket Feb 3, 2024
2b71c70
Merge pull request #1121 from http4s/update/series/0.9/http4s-core-0.…
hamnis Nov 7, 2024
c5f098c
Merge branch 'series/0.9' into feature/#1011-client-as-resource
hamnis Nov 7, 2024
0f8e981
Merge branch 'series/0.9' into feature/#1011-client-as-resource
ybasket Nov 8, 2024
4cd2f14
Only offer Resource-based constructors
ybasket Nov 12, 2024
3d2262d
Bump tlBaseVersion
ybasket Nov 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 57 additions & 1 deletion .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,12 +30,16 @@ jobs:
matrix:
os: [ubuntu-latest]
scala: [2.12, 2.13, 3]
java: [temurin@11, temurin@17]
java: [temurin@11, temurin@17, temurin@21]
exclude:
- scala: 2.12
java: temurin@17
- scala: 2.12
java: temurin@21
- scala: 3
java: temurin@17
- scala: 3
java: temurin@21
runs-on: ${{ matrix.os }}
timeout-minutes: 60
steps:
Expand Down Expand Up @@ -73,6 +77,19 @@ jobs:
if: matrix.java == 'temurin@17' && steps.setup-java-temurin-17.outputs.cache-hit == 'false'
run: sbt +update

- name: Setup Java (temurin@21)
id: setup-java-temurin-21
if: matrix.java == 'temurin@21'
uses: actions/setup-java@v4
with:
distribution: temurin
java-version: 21
cache: sbt

- name: sbt update
if: matrix.java == 'temurin@21' && steps.setup-java-temurin-21.outputs.cache-hit == 'false'
run: sbt +update

- name: Check that workflows are up to date
run: sbt githubWorkflowCheck

Expand Down Expand Up @@ -158,6 +175,19 @@ jobs:
if: matrix.java == 'temurin@17' && steps.setup-java-temurin-17.outputs.cache-hit == 'false'
run: sbt +update

- name: Setup Java (temurin@21)
id: setup-java-temurin-21
if: matrix.java == 'temurin@21'
uses: actions/setup-java@v4
with:
distribution: temurin
java-version: 21
cache: sbt

- name: sbt update
if: matrix.java == 'temurin@21' && steps.setup-java-temurin-21.outputs.cache-hit == 'false'
run: sbt +update

- name: Download target directories (2.12)
uses: actions/download-artifact@v4
with:
Expand Down Expand Up @@ -255,6 +285,19 @@ jobs:
if: matrix.java == 'temurin@17' && steps.setup-java-temurin-17.outputs.cache-hit == 'false'
run: sbt +update

- name: Setup Java (temurin@21)
id: setup-java-temurin-21
if: matrix.java == 'temurin@21'
uses: actions/setup-java@v4
with:
distribution: temurin
java-version: 21
cache: sbt

- name: sbt update
if: matrix.java == 'temurin@21' && steps.setup-java-temurin-21.outputs.cache-hit == 'false'
run: sbt +update

- name: Submit Dependencies
uses: scalacenter/sbt-dependency-submission@v2
with:
Expand Down Expand Up @@ -303,6 +346,19 @@ jobs:
if: matrix.java == 'temurin@17' && steps.setup-java-temurin-17.outputs.cache-hit == 'false'
run: sbt +update

- name: Setup Java (temurin@21)
id: setup-java-temurin-21
if: matrix.java == 'temurin@21'
uses: actions/setup-java@v4
with:
distribution: temurin
java-version: 21
cache: sbt

- name: sbt update
if: matrix.java == 'temurin@21' && steps.setup-java-temurin-21.outputs.cache-hit == 'false'
run: sbt +update

- name: Generate site
run: sbt docs/tlSite

Expand Down
1 change: 1 addition & 0 deletions .mergify.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ pull_request_rules:
- status-success=Build and Test (ubuntu-latest, 2.12, temurin@11)
- status-success=Build and Test (ubuntu-latest, 2.13, temurin@11)
- status-success=Build and Test (ubuntu-latest, 2.13, temurin@17)
- status-success=Build and Test (ubuntu-latest, 2.13, temurin@21)
- status-success=Build and Test (ubuntu-latest, 3, temurin@11)
- status-success=Generate Site (ubuntu-latest, temurin@11)
actions:
Expand Down
5 changes: 3 additions & 2 deletions build.sbt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import com.typesafe.tools.mima.core._
import explicitdeps.ExplicitDepsPlugin.autoImport.moduleFilterRemoveValue
hamnis marked this conversation as resolved.
Show resolved Hide resolved

lazy val root = project
.in(file("."))
Expand Down Expand Up @@ -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"
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

ThisBuild / startYear := Some(2019)
ThisBuild / developers := List(
tlGitHubDev("ChristopherDavenport", "Christopher Davenport"),
Expand All @@ -80,7 +81,7 @@ ThisBuild / developers := List(
)

ThisBuild / tlJdkRelease := Some(11)
ThisBuild / githubWorkflowJavaVersions := Seq("11", "17").map(JavaSpec.temurin(_))
ThisBuild / githubWorkflowJavaVersions := Seq("11", "17", "21").map(JavaSpec.temurin(_))
ThisBuild / tlCiReleaseBranches := Seq("series/0.9")
ThisBuild / tlSitePublishBranch := Some("series/0.9")

Expand Down
17 changes: 15 additions & 2 deletions core/src/main/scala/org/http4s/jdkhttpclient/JdkHttpClient.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,9 +243,22 @@ object JdkHttpClient {
* [[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, it actively closes the underlying client, releasing its resources
* early. On earlier Java versions, closing the underlying client is not possible, so the release
* is a no-op. On these Java versions (and there only), you can safely use
* [[cats.effect.Resource allocated]] to avoid dealing with resource management.
*/
def simple[F[_]](implicit F: Async[F]): F[Client[F]] =
defaultHttpClient[F].map(apply(_))
def simple[F[_]](implicit F: Async[F]): Resource[F, Client[F]] =
defaultHttpClientResource[F].map(apply(_))

private[jdkhttpclient] def defaultHttpClientResource[F[_]](implicit
F: Async[F]
): Resource[F, HttpClient] =
Resource.make[F, HttpClient](defaultHttpClient[F]) {
case c: AutoCloseable => Sync[F].blocking(c.close())
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the most relevant bit, actively closing if HttpClient <: AutoCloseable which is the case on Java 21+.

case _ => Applicative[F].unit
}

private[jdkhttpclient] def defaultHttpClient[F[_]](implicit F: Async[F]): F[HttpClient] =
F.executor.flatMap { exec =>
Expand Down
14 changes: 12 additions & 2 deletions core/src/main/scala/org/http4s/jdkhttpclient/JdkWSClient.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that #853 isn't going anywhere soon, I think the fix in this PR is a good step for now. It avoids the test hang and improves the current situation without blocking even more elaborate solutions like #853.

} yield ()
}
.map { case (webSocket, queue, closedDef, sendSem) =>
Expand Down Expand Up @@ -164,7 +169,12 @@ 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, it actively closes the underlying client, releasing its resources
* early. On earlier Java versions, closing the underlying client is not possible, so the release
* is a no-op. On these Java versions (and there only), you can safely use
* [[cats.effect.Resource allocated]] to avoid dealing with resource management.
*/
def simple[F[_]](implicit F: Async[F]): F[WSClient[F]] =
JdkHttpClient.defaultHttpClient[F].map(apply(_))
def simple[F[_]](implicit F: Async[F]): Resource[F, WSClient[F]] =
JdkHttpClient.defaultHttpClientResource[F].map(apply(_))
}
2 changes: 1 addition & 1 deletion core/src/test/scala/org/http4s/jdkhttpclient/BodyLeakExample.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ object BodyLeakExample extends IOApp {
.withPort(port"8080")
.withHttpApp(app)
.build
.product(Resource.eval(JdkHttpClient.simple[IO]))
.product(JdkHttpClient.simple[IO])
.use { case (_, client) =>
for {
counter <- Ref.of[IO, Long](0L)
Expand Down
2 changes: 1 addition & 1 deletion core/src/test/scala/org/http4s/jdkhttpclient/DeadlockWorkaround.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ class DeadlockWorkaround extends CatsEffectSuite {
test("fail to connect via TLSv1.3 on Java 11") {
if (Runtime.version().feature() > 11) IO.pure(true)
else
(JdkHttpClient.simple[IO], JdkWSClient.simple[IO]).flatMapN { (http, ws) =>
(JdkHttpClient.simple[IO], JdkWSClient.simple[IO]).tupled.use { case (http, ws) =>
def testSSLFailure(r: IO[Unit]) = r.intercept[SSLHandshakeException]
testSSLFailure(http.expect[Unit](uri"https://tls13.1d.pw")) *>
testSSLFailure(ws.connectHighLevel(WSRequest(uri"wss://tls13.1d.pw")).use(_ => IO.unit))
Expand Down
2 changes: 1 addition & 1 deletion core/src/test/scala/org/http4s/jdkhttpclient/JdkHttpClientSpec.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ import org.typelevel.ci._
import scala.concurrent.duration._

class JdkHttpClientSpec extends ClientRouteTestBattery("JdkHttpClient") {
def clientResource: Resource[IO, Client[IO]] = Resource.eval(JdkHttpClient.simple[IO])
def clientResource: Resource[IO, Client[IO]] = JdkHttpClient.simple[IO]

// regression test for https://github.com/http4s/http4s-jdk-http-client/issues/395
test("Don't error with empty body and explicit Content-Length: 0") {
Expand Down
2 changes: 1 addition & 1 deletion core/src/test/scala/org/http4s/jdkhttpclient/JdkWSClientSpec.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ import scala.concurrent.duration._
class JdkWSClientSpec extends CatsEffectSuite {

val webSocket: IOFixture[WSClient[IO]] =
ResourceSuiteLocalFixture("webSocket", Resource.eval(JdkWSClient.simple[IO]))
ResourceSuiteLocalFixture("webSocket", JdkWSClient.simple[IO])
val echoServerUri: IOFixture[Uri] =
ResourceSuiteLocalFixture(
"echoServerUri",
Expand Down
Loading