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.

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

TickWheelExecutor is a resource, not an allocation #2189

Merged
merged 3 commits into from Oct 20, 2018

Conversation

Projects
None yet
2 participants
@rossabaker
Member

rossabaker commented Oct 19, 2018

Composing two F[A, F[Unit]]'s together defeats the purpose of the Resource AST: any intermediate cancellations can leak a resource. The only safe way to combine resources is through the resource interpreter. This makes the BlazeClientBuilder.resource the primitive constructor, and expresses allocate in terms of it.

@rossabaker rossabaker added this to the 0.19.1 milestone Oct 19, 2018

@@ -117,4 +119,47 @@ package object internal {
}
}
}
/* Derived from https://github.com/typelevel/cats-effect/blob/c62c6c76b3066c500fca2f9e0897605bc12eb2a0/core/shared/src/main/scala/cats/effect/Resource.scala */
private[http4s] def interpretResource[F[_], A](resource: Resource[F, A])(

This comment has been minimized.

@rossabaker

rossabaker Oct 19, 2018

Member

This is a copy of Client.toHttpApp, which should be expressed in terms of this. But let's agree on whether it should return ExitCase[Throwable] => F[Unit] or just F[Unit] first.

)
}
}
def allocate(implicit F: ConcurrentEffect[F]): F[(Client[F], ExitCase[Throwable] => F[Unit])] =

This comment has been minimized.

@rossabaker

rossabaker Oct 19, 2018

Member

I think I saw discussion of this return type fly by on Gitter recently, but I can't find it now. Do we want the ExitCase here or nah?

This comment has been minimized.

@ChristopherDavenport

ChristopherDavenport Oct 19, 2018

Member

Do we care about ExitCase ever?

@rossabaker

This comment has been minimized.

Member

rossabaker commented Oct 19, 2018

Build failures are #2191, #2192.

@@ -117,4 +119,14 @@ package object internal {
}
}
}
private[http4s] def allocated[F[_], A](resource: Resource[F, A])(

This comment has been minimized.

@ChristopherDavenport

ChristopherDavenport Oct 20, 2018

Member

This is too useful, whats the catch? The stronger constraint?

This comment has been minimized.

@rossabaker

rossabaker Oct 20, 2018

Member

That's the only downside I can see.

@rossabaker rossabaker merged commit 721d3eb into http4s:master Oct 20, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@rossabaker rossabaker deleted the rossabaker:tick-wheel-resource branch Oct 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment