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.

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

Parameterize `Timeout` middleware #1899

Merged
merged 3 commits into from May 31, 2018

Conversation

@tomwadeson
Copy link
Contributor

@tomwadeson tomwadeson commented May 30, 2018

Relates to: #1890

I've only generalised the non-deprecated, public API so far. Should I continue?


import scala.concurrent.ExecutionContext.Implicits.global
val T = Timer.derive[F]

This comment has been minimized.

@tomwadeson

tomwadeson May 30, 2018
Author Contributor

I was seeing the following error:

[error] /Users/tom/Projects/http4s/examples/blaze/src/main/scala/com/example/http4s/blaze/demo/server/Module.scala:48:22: Cannot find implicit value for Timer[[A]cats.data.OptionT[F,A]].
[error] Note that [A]cats.data.OptionT[F,A] needs to be a cats.effect.Async data type. You might also
[error] need a scala.concurrent.ExecutionContext in scope, or equivalent, try to
[error] import scala.concurrent.ExecutionContext.Implicits.global
[error]     Timeout(1.second)(timeoutHttpEndpoint)

We already require Concurrent[F], which implies LiftIO[F], so we're able to Timer.derive[F] where we need one.

If we're happy to do this, is it OK to use the global ExecutionContext here, or should we require a Strategy/ExecutionStrategy be passed in?

This comment has been minimized.

@jmcardon

jmcardon May 30, 2018
Member

I would prefer you simply take a Timer instead of calling derive since it opens feeding in a different one if need be.

This comment has been minimized.

@tomwadeson

tomwadeson May 30, 2018
Author Contributor

Right! And on the issue of there not being an instance for OptionT[F, ?] - do you have any suggestions on how best to provide one, or would we leave this to users?

This comment has been minimized.

@jmcardon

jmcardon May 30, 2018
Member

that's in userland/another PR I think.

This comment has been minimized.

@tomwadeson

tomwadeson May 30, 2018
Author Contributor

I get you. I'm happy to defer to your judgement on this, but would say that:

  1. I think I'd argue that it's an implementation detail of the Timeout middleware and it's not immediately obvious to me why I might want to switch one Timer out for another
  2. I think the ergonomics are better this way: it's cleaned up Module and Server in this PR and I think I prefer this API as a user

If that's not convincing, that's fine, I'll change it 😄

This comment has been minimized.

@jmcardon

jmcardon May 30, 2018
Member

Counter args:

  1. the global ec is often used as the general client pool in something like blaze. Where a user chooses to execute stuff is not an implementation detail in my eyes. It impacts the app, even if it's true that a good chunk of the times people do use global. Whenever you Timer.sleep, you force the continuation of that call onto the other pool provided by the timer. For advanced users that do configure thread pools, this is a net loss.
  2. What does it clean up aside from removing a single implicit?

I'm in general 👎 on explicitly less powerful configuration. It's a slight win for beginner users by limiting advanced users by forcing them onto global. Moreover, I don't particularly like considering application configuration (which timer propagates, essentially since it does carry a thread pool reference no matter how much anyone wants to pretend it doesn't) "an implementation detail".

This is a democratic decision though, so other maintainer opinions would be nice.

This comment has been minimized.

@tomwadeson

tomwadeson May 30, 2018
Author Contributor

I don't disagree with any of that! To be clear, I'm only arguing for the Timer to be defined locally. The ExecutionContext can absolutely be passed in.

I'd also welcome other POVs. I'm acutely aware that I'm arguing from a position of ignorance on this!

This comment has been minimized.

@jmcardon

jmcardon May 30, 2018
Member

but that's the thing though: in the ecosystem we're trying to move away from passing ExecutonContexts and passing Timer[F]s. Hence my quip.

This comment has been minimized.

@tomwadeson

tomwadeson May 30, 2018
Author Contributor

I see! I'll change it.

tomwadeson added 2 commits May 30, 2018
class Module[F[_]](client: Client[F])(
implicit F: ConcurrentEffect[F],
S: Scheduler,
T: Timer[OptionT[F, ?]]) {

This comment has been minimized.

@jmcardon

jmcardon May 31, 2018
Member

Sorry, is there a reason this is necessary in particular to be OptionT? I'm missing something not immediately clear upon inspection.

Also, Timer is there to replace fs2 scheduler eventually. Maybe in another PR we can proceed to clean up examples, but when you have one, the other is no longer needed

This comment has been minimized.

@tomwadeson

tomwadeson May 31, 2018
Author Contributor

This is Timeout pre-generalisation:

  def apply[F[_]](timeout: FiniteDuration, timeoutResponse: F[Response[F]])(
      @deprecatedName('service) routes: HttpRoutes[F])(
      implicit F: Concurrent[F],
      T: Timer[F]): HttpRoutes[F] =???

F is the non-OptionT-adorned F that parameterises HttpRoutes[F] (which expands to Http[OptionT[F, ?], F]), so it doesn't matter that there's no Timer[OptionT[F, ?]] because we don't require evidence for one.

But by having now generalised it like so:

  def apply[F[_], G[_], A](timeout: FiniteDuration, timeoutResponse: F[Response[G]])(
      @deprecatedName('service) http: Kleisli[F, A, Response[G]])(
      implicit F: Concurrent[F],
      T: Timer[F]): Kleisli[F, A, Response[G]] = ???

... then when F is instantiated within Module (and I guess most places Timeout is used in userland too), it becomes OptionT[F, ?], and we now require evidence of Timer[OptionT[F, ?]]:

  private val timeoutHttpEndpoint: HttpRoutes[F] =
    new TimeoutHttpEndpoint[F].service

  private val timeoutEndpoints: HttpRoutes[F] =
    Timeout(1.second)(timeoutHttpEndpoint)

This is quite gnarly though (and what prompted me to Timer.derive[F] at the use-site previously. ) Is there a simpler solution that I'm not seeing? I guess the real fix would be to have transformer instances for Timer defined upstream in cats.

This comment has been minimized.

@jmcardon

jmcardon May 31, 2018
Member

100% my bad I misread this. I just thought Timeout wasn't being used at all and/or it was being passed a Http[F, F]. You're right 👍

Sorry about that. More a mistake due to improperly reading Module[F]

This comment has been minimized.

@tomwadeson

tomwadeson May 31, 2018
Author Contributor

Cool. Happy to tackle the clean up of examples in a separate PR, as you suggest 😄

@rossabaker rossabaker merged commit 51c728f into http4s:master May 31, 2018
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.