Fix threadpool shifting for futureToAsync

[felixbr]

Don't merge this yet, I don't consider this done 🙂

Hi, I was using catbird-effect to wrap finagle-http for usage in projects that mainly use cats.effect.IO or monix.eval.Task.

When I tried a few things locally I noticed in the log output that my logs (wrapped in IO) where coming from the finagle/netty threadpool and not the main one even though the actual http-call was already done.

After some testing and research I came to the conclusion that io.catbird.util.effect.futureToAsync is flawed and does only convert Future to IO but does not shift execution back to the threadpool initially used for IO.

I wrote two tests for that (one negative and one positive) so you can easily see the difference and how a solution would look like.

The PR right now adds an additional function futureToAsyncAndShift instead of changing the behavior of the existing one.
However, I strongly think that this is a bad solution as the default should be safe and the current behavior is almost never what users want in practice.

I also looked into how IO.fromFuture is implemented for Scala futures and indeed it shifts back to the IO threadpool after the Future is resolved. (https://github.com/typelevel/cats-effect/blob/master/core/shared/src/main/scala/cats/effect/IO.scala#L1350-L1355)

Imo this fix would warrant a breaking change (requiring a ContextShift[F] in addition to Async[F]) but I don't know what compatibility guarantees you want to provide. I'll rework the PR as needed in any case 🙂

Cheers
~ Felix

edit: I also found cases where the execution did continue on the IO pool even without my fix. This makes the current implementation even more wacky as the user has no deterministic control over what pool is being used. So even if we keep futureToAsync in its current form, we should make sure that it has deterministic and documented behavior regarding threadpools.

[codecov-io]

Codecov Report

Merging #209 into master will decrease coverage by 0.65%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master     #209      +/-   ##
==========================================
- Coverage   93.98%   93.33%   -0.66%     
==========================================
  Files           5        5              
  Lines         133      135       +2     
  Branches        3        3              
==========================================
+ Hits          125      126       +1     
- Misses          8        9       +1

Impacted Files	Coverage Δ
...rc/main/scala/io/catbird/util/effect/package.scala	85.71% <66.66%> (-14.29%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a7092a4...99b6ebe. Read the comment docs.

[travisbrown]

@felixbr Thanks for taking this on! This library has been needing to do some catch-up with cats-effect for a long time, and I'm not using Finagle these days, so it's hard for me to prioritize. Any chance you'd want to join as a maintainer?

[felixbr]

I'd be happy to join as a maintainer 🙂

[felixbr]

After thinking about it a bit more, I think we should go the same route as cats.effect.IO went and deprecate the existing futureToAsync (and rerunnableToIO, I guess) and add similar functions that need ContextShift[F] to do the correct shifting.

I really hope downstream users will not ignore the deprecation, though 😶

[travisbrown]

@felixbr That sounds reasonable to me.

[felixbr]

I dug deeper and it seems that we cannot really fix this properly without putting a ContextShift[Rerunnable] requirement on the instance of Effect[Rerunnable]. Indeed this is what the docs of Effect say ("Building this implicit value might depend on having an implicit
s.c.ExecutionContext in scope, a Scheduler, a ContextShift[F]
or some equivalent type.") and what the instance for monix.eval.Task does (You need a Scheduler in scope if you want to use Effect[Task])

I also looked at finch as I suspected it would be using it downstream but to my surprise it has it's own implementation which seems to have the same problem: https://github.com/finagle/finch/blob/master/core/src/main/scala/io/finch/internal/ToAsync.scala

So, what does this mean now? I'm keeping the additional function as I'm currently not sure what to do about the Effect[Rerunnable] instance which uses the current version of rerunnableToIO.

I'm thinking about tackling the implementation of ContextShift[Rerunnable] and Timer[Rerunnable] next as I'd really like to be able to have the possibility of using fs2 libraries in Finagle-based applications. I have hope that I will get a better understanding of this whole thread pool shifting business while implementing ContextShift[Rerunnable] (fingers crossed)

[felixbr]

The codecov tool seems to bug out again. It claims I haven't tested futureToAsyncAndShift but I have (I double-checked again). Maybe it only finds things that run on the Scalatest threadpool?

Anyway, I'd like to merge this soon, as I have implementations for ContextShift[Rerunnable] and Timer[Rerunnable] ready, but I'd like to share some test-code between them, so it would be easier to merge this first. 🙂

[travisbrown]

It looks like coverage is complaining about rerunnableToIOAndShift, so it's at least not wrong, although given the implementation I don't think the missing coverage is a big deal.

My general approach to coverage is to have pretty low thresholds for "failing" checks but to merge anyway if I'm aware of what I'm doing. I'm happy to adjust the thresholds instead if you'd prefer.

In any case, I'll merge this today if that's okay with you!

[felixbr]

I guess I'm not used to reading the coverage reports yet. In the "Files" windows it shows it differently, but the "Diff" is much clearer. 🙂

I'd be fine with merging as is.

[travisbrown]

Great, thanks again!