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

Should ConcurrentEffect[Task] take implicit Task.Options? #625

Closed
oleg-py opened this Issue Mar 28, 2018 · 5 comments

Comments

Projects
None yet
2 participants
@oleg-py
Collaborator

oleg-py commented Mar 28, 2018

When using abstractions built on top of ConcurrentEffect, the user is no longer in charge of running the task. It makes it quite hard to configure both cancelability and local context propagation, requiring manual insertion of executeWithOptions in every possibly affected returned Task.

I think it makes sense if ConcurrentEffect required not only Scheduler, but also Task.Options.

For example, http4s now cancels running effect. It might be an option to not resume execution of task (by enabling autoCancelableRunLoops) if I have only DB transactions (which require localContextPropagation).

While I can build instance of Effect[Task] myself with all desired properties, a user might not be aware that he needs to, as it is not a compile-time error, but might lead to incorrect behavior at runtime, possibly even without even a runtime exception thrown.

/cc @leandrob13

@alexandru

This comment has been minimized.

Member

alexandru commented Mar 29, 2018

We probably need Task.Options in there, yes, but with a default value.

I'm not 100% sure though. It complicates the API.
Are you sure it eases your interaction with Http4s?

@oleg-py

This comment has been minimized.

Collaborator

oleg-py commented Mar 30, 2018

@alexandru the thing is:

  • I am using TaskApp to start http4s server
  • I am redefining def options to Coeval(Task.Options(true, true))
  • When I make a request, http4s uses Task.Options(false, false) (the default) to run my Tasks

Issues:

  1. Caught me completely by surprise. So it might be error-prone.
  2. There is no easy way provide the Effect instance I need. I have to extend CatsEffectForTask (luckily it's not private) and override runAsync
@alexandru

This comment has been minimized.

Member

alexandru commented Apr 1, 2018

@oleg-py can you do a PR? It needs a default value though, set to Task.defaultOptions.

@oleg-py

This comment has been minimized.

Collaborator

oleg-py commented Apr 1, 2018

@alexandru Sure. But I don't see why it is a good idea to provide default value. It would still compile without any warning if you don't have an implicit instance available (which, when using TaskApp, you don't).

@alexandru

This comment has been minimized.

Member

alexandru commented Apr 1, 2018

Well, Task.defaultOptions is not implicit and I don't want people to be forced to provide their own Options for Effect, but I want to force people to provide their own Options when they do runAsyncOpt.

We probably need another change as well. Usage of TaskLocal is error prone without that option being enabled and I think we should enable it ourselves on bind for the resulting task.

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