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

Introduce ContextShift for Rerunnable #216

Merged
merged 1 commit into from
May 6, 2020

Conversation

felixbr
Copy link
Contributor

@felixbr felixbr commented Mar 17, 2020

This relates to issue #157.

Neither Future nor IO provide a way to get a handle on the underlying thread-pool, so the only way to construct a ContextShift instance is to pass a handle to an ExecutionContext explicitly.

@ben-healthforge mentioned in the issue that this "seems wrong", but I currently don't see a way around it. Feel free to comment if you have any ideas :)

monix.eval.Task is worth mentioning which provides Task.deferAction { implicit scheduler => ... } to get a handle on the underlying Scheduler and I've read this will probably make it into cats-effect 3.0.
For the moment I'd go with this implementation, which is closer to the one for IO.

In the tests I've tried to check correct context-switching for common scenarios like calling into a library like fs2 or http4s from a Finagle/Finatra application (which is based on Future). Hope this makes sense.

@travisbrown travisbrown self-requested a review May 5, 2020 09:38
Copy link
Contributor

@travisbrown travisbrown left a comment

Choose a reason for hiding this comment

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

This looks reasonable to me. Do you think it's ready to merge, @felixbr?

@felixbr
Copy link
Contributor Author

felixbr commented May 6, 2020

From my side it's ready. I just waited for feedback from you or @ben-healthforge who initially opened the related issue 🙂

@felixbr felixbr merged commit eb441c4 into typelevel:master May 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants