cats-effect 3.1.x for Rerunnable -> Sync + Clock + MonadCancel

[felixbr]

Note: This is only up for discussion not really to be merged as of right now. I just use a draft PR for this, so it's easy to see code changes.

So cats-effect 3.0.0-M2 was released and I wanted to see if the claims were true and implementing the typeclasses is easier now.

For reference, the release notes have lots of useful information (the typeclass diagram in 3.0.0-M1 in particular):
https://github.com/typelevel/cats-effect/releases/tag/v3.0.0-M1
https://github.com/typelevel/cats-effect/releases/tag/v3.0.0-M2

Implementing the "right" half of the typeclass diagram (namely Clock and Sync) was exceptionally easy. Another good thing is that ContextShift and Timer are gone entirely and Bracket is no longer needed according to the documentation (it still exists for Resource for some reason).

The base typeclass MonadCancel already poses the same problems as Concurrent in cats-effect 2.x. At the moment Rerunnable is mostly a lazy wrapper around Future and Promise and therefore delegates almost all runtime concerns to those implementations. To implement cancelation as required by MonadCancel we'd have to introduce state somewhere to keep track of finalizers, cancelations, cancelation-masks, and possibly more. IO keeps this state in its Fiber abstraction and its runloop. I tried to put state into Rerunnable itself but it got messy very quickly. We'd probably have to rethink the whole architecture and how it relates to Future and future pools.

Another issue is that cats-effect expects Rerunnable to depend on cats-effect-kernel but we currently have a distinction between Rerunnable with only cats (in the util module) and cats-effect typeclasses for Rerunnable (in the effect module). This makes the implementation even more complicated.

You can see the IO implementation here: https://github.com/typelevel/cats-effect/blob/series/3.x/core/shared/src/main/scala/cats/effect/IOFiber.scala
As far as I understand this is a fairly low-level state-machine. I don't think we can just copy this easily, as modifying the Future runloop is not feasible.

The question for me is: Do we really need all that functionality? What are the use-cases for it?

One fairly cool thing would be to use fs2 and derivatives directly in applications built on Finagle and Future. But if we have to convert between Future and Rerunnable anyway, maybe it's not too bad to convert directly to IOinstead. The latter seems to work already via functions in io.catbird.util.effect.

Another use-case I've seen is to use Rerunnable in finch applications, i.e. Endpoint[Rerunnable]. I've looked through the code there and most stuff in Endpoint[F[_]] requires Sync[F] or Effect[F]. Sync already works and Effect no longer exists. As far as I can tell Effect was basically used to convert Rerunnable to IO inside finch, so maybe there's a solution possible since io.catbird.util.effect.rerunnableToIO exists.

If we still need to reimplement a runloop which supports all of cats-effect we might be better off starting from scratch. Fabio Labella wrote

yes, the runloop itself is not really aware of js, it just sees an ec to submit to. The only JVM specific thing in there (in CE3) is selecting the work stealing pool

Maybe we could do something like that but instead of submitting to an ExecutionContext we submit to a FuturePool.

Anyway, I hope this wasn't too much rambling. If anyone has any comments, I'd love to hear them. 🙂

Cheers
~ Felix

[felixbr]

I rebased the branch onto master and did some further build tweaks, so everything is in a decent state to move from M2 to M5.

[felixbr]

I asked in the cats-effect gitter channel and it seems we can try and implement MonadCancel using an uncancelable root scope similar to how SyncIO does it. This at least gives us back the utilities we had with Bracket.

As for the target scope, I want to try and support three things:

1. Safe and easy conversion between Future and IO, which is currently possible with something like IO.fromEffect(Rerunnable.fromFuture(someFutureCall)) even without cancelation this is really useful if you want to use finagle-thrift or finagle-http clients from a cats-effect application.
2. The flip-side is calling into IO or monix.eval.Task from a finagle application, which uses Future as the main effect. I hope this will be fairly simple.
3. Support whatever is required to have Endpoint[Rerunnable, A] in finch. Initially finch was tightly coupled to finagle, so this use-case should be well-supported if possible by sticking to Rerunnable as runtime for Endpoint. Last time I looked it only used Async and Effect but no cancelation, so maybe we can get there with cats-effect 3.x as well.

[codecov-io]

Codecov Report

Merging #267 (59ed6ff) into master (65d12f6) will decrease coverage by 3.35%.
The diff coverage is 80.00%.

❗ Current head 59ed6ff differs from pull request most recent head b50a7de. Consider uploading reports for the commit b50a7de to get more accurate results

@@            Coverage Diff             @@
##           master     #267      +/-   ##
==========================================
- Coverage   91.35%   88.00%   -3.36%     
==========================================
  Files           9        6       -3     
  Lines         162      125      -37     
  Branches        3        1       -2     
==========================================
- Hits          148      110      -38     
- Misses         14       15       +1     

Impacted Files	Coverage Δ
...rc/main/scala/io/catbird/util/effect/package.scala	70.00% <40.00%> (-18.89%)	⬇️
...scala/io/catbird/util/effect/FutureInstances.scala	33.33% <100.00%> (-66.67%)	⬇️
...a/io/catbird/util/effect/RerunnableInstances.scala	100.00% <100.00%> (ø)
...il/src/main/scala/io/catbird/util/Rerunnable.scala	87.80% <0.00%> (-2.56%)	⬇️

---

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 65d12f6...b50a7de. Read the comment docs.

[felixbr]

Since 3.0.0-RC1 dropped the strict cancelation requirements for MonadCancel we can actually implement it for both Rerunnable and even Future, which is really nice.

Without the cancelation requirement we might even be able to support more of the typeclass hierarchy than in 2.x.

I will also take a look into the drop in coverage, of course.

[felixbr]

I spoke a bit too soon. Implementing Spawn and further typeclasses requires actual cancelation, which makes sense, as racePair would leak resources if the losing Future cannot be canceled reliably.

[felixbr]

@travisbrown Hi, so since cats-effect 3 is stable, we probably should figure out how to release catbird for that. I still have to put in some work in this PR (in particular why codecov is so unhappy) but for the most part the code should work.

The big question for me is if we need to have a cross-released version for say Finagle 21.X.0 and both cats-effect 2 and 3 or if we just release catbird with Finagle 21.X.0 with cats-effect 2 and Finagle 21.(X+1).0 with cats-effect 3.
The latter is way less work but if there happens to be an incompatibility in Finagle between those versions, it might be ugly.

Maybe we can get away with only cross-publishing if someone explicitly asks for it. 🤔

What are your thoughts on that?

[travisbrown]

@felixbr I'm happy to take on the medium-term maintenance burden of cross-publishing releases (especially if it's for some fixed period of time), but unfortunately I don't think I'll have time any time soon to help in detail with the update or build setup.

[felixbr]

Ok, so instead of merging this into master (once it's done, ofc) we'd put it on a branch and publish the versions separately.

For example:
"catbird-effect" % "21.5.0" -> cats-effect 2
"catbird-effect-3" % "21.5.0" -> cats-effect 3

After we've done this for 21.5.0 and maybe 21.6.0 we'd continue with cats-effect 3.x only.

I'm sadly also quite busy but I'll try to get everything in a clean state for this in the next weeks.

*21.5.0 is just an example, there's no rush imo.

[travisbrown]

I think if possible I'd prefer to have separate source trees (possibly with a shared part) in a single branch, similar to what we do in circe-jackson to cross-publish for different Jackson versions. I don't have a strong opinion, though, and am happy to go either way.

[felixbr]

So two separate sub-projects in sbt for catbird-effect and catbird-effect-3? Yeah, that's probably a better idea. I'll see what I can do 👍

[travisbrown]

Great, thanks! The circe-jackson model may not be optimal but it more or less works, so you might consider it as a starting point.

[catostrophe]

@felixbr do you need any help to get this merged?

[felixbr]

@catostrophe This is far from ready to be merged but the limiting factor is mostly a lack of free time to work on it.

I hope you aren't blocked by this?

[catostrophe]

@felixbr actually I am, transitively. catbird is needed for the integration of tapir and finatra. And I need a CE3-compatible release of tapir

[felixbr]

@catostrophe Well, that's unfortunate. If you want to help, there are tasks which you could already do to make this PR easier to fix:

• We should drop Scala 2.11. Keeping it around is annoying because other dependencies also dropped it a while ago. (issue)
• We should upgrade cats. 2.0.0 is fairly old and the latest cats-effect also uses a recent version. Besides that it's also beneficial once we aim for Scala 3.0 cross-builds.

If you want to work on one of those things, you can open PRs for the master branch as usual. 🙂

[brbrown25]

@felixbr I created #320 which removes 2.11 and also upgrades cats and cats-effect to newer 2.x releases

[felixbr]

Thanks, I will take a look in the next days

[codecov-commenter]

Codecov Report

Merging #267 (c428a72) into master (0310001) will decrease coverage by 1.58%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #267      +/-   ##
==========================================
- Coverage   91.82%   90.24%   -1.58%     
==========================================
  Files           9        6       -3     
  Lines         159      123      -36     
  Branches        3        1       -2     
==========================================
- Hits          146      111      -35     
+ Misses         13       12       -1     

Impacted Files	Coverage Δ
...a/io/catbird/util/effect/RerunnableInstances.scala
...scala/io/catbird/util/effect/FutureInstances.scala
...rc/main/scala/io/catbird/util/effect/package.scala

---

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 0310001...c428a72. Read the comment docs.

[felixbr]

GitHub Actions seem to have downtime right now: actions/runner-images#2179 (comment)

[felixbr]

The last codecov report is weird. The 2 files that have a significant drop in coverage are ones I haven't touched at all (as after the last force-push I'm only adding new files in catbird-effect3, so other modules should stay the same).

Maybe codecov got confused because I rewrote the whole feature-branch.

Anyway. I'm not claiming that catbird-effect3 necessarily supports exactly the same things as the 2.x version but the easiest way to find out would be a release (e.g. maybe even just a RC or nightly).

I've incorporated the review feedback from @catostrophe but If there is anything else missing let me know. 🙂

[dangerousben]

FYI, I have been experimenting with an idea to implement fibers and cancellation for CE3. Not yet sure whether it will pan out, but it would be good to have this as a base to build on so I'd like to help out however I can. I'm on gitter if anyone would like to chat about it.

I agree with the separate-source-tree approach, I think this should be considered a clean slate rather a rework of the CE2 version.

I also have some catbird-native testing infrastructure that could be integrated into this (though not at the expense of getting it merged, it can always be added later).

[travisbrown]

@felixbr @dangerousben Just a heads up that this repo has now moved from my individual account to the Typelevel organization. In my experience GitHub's forwarding in these situations is solid and works indefinitely (?), so it shouldn't affect anything here, but it'd probably be a good idea to update your references locally at some point.

[sebastianvoss]

Thanks a lot for this PR! Is there a rough timeline for this to get merged? Also, if help is needed I'm happy to contribute where I can.

[travisbrown]

@sebastianvoss I'll defer to @felixbr and @dangerousben on that question. I'm happy to publish a release whenever they say it's ready.

[dangerousben]

@sebastianvoss I'll defer to @felixbr and @dangerousben on that question. I'm happy to publish a release whenever they say it's ready.

I don't know what's holding it back personally, the codecov issue?

There is a rebased version at https://github.com/dangerousben/catbird/tree/topic/cats-effect-3.0.0 if it helps.

[codecov-commenter]

Codecov Report

Merging #267 (8403f80) into master (a49d9d9) will decrease coverage by 1.73%.
The diff coverage is 74.07%.

@@            Coverage Diff             @@
##           master     #267      +/-   ##
==========================================
- Coverage   92.85%   91.12%   -1.74%     
==========================================
  Files          14       17       +3     
  Lines         266      293      +27     
  Branches        3        1       -2     
==========================================
+ Hits          247      267      +20     
- Misses         19       26       +7     

Impacted Files	Coverage Δ
...rc/main/scala/io/catbird/util/effect/package.scala	54.54% <54.54%> (ø)
...scala/io/catbird/util/effect/FutureInstances.scala	60.00% <60.00%> (ø)
...a/io/catbird/util/effect/RerunnableInstances.scala	100.00% <100.00%> (ø)

---

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 a49d9d9...8403f80. Read the comment docs.

[catostrophe]

Great! Waiting for a release!

[sebastianvoss]

Thanks a lot for the merge. Sorry for being annoying. @travisbrown, are you planning to push a release?

[travisbrown]

Should we wait for #347?

[travisbrown]

@sebastianvoss I'm publishing catbird-effect3 under the 21.5.0 release now, since it's a new module and the other dependencies are unchanged since 21.5.0 (apart from Scala patch versions).

[sebastianvoss]

@travisbrown Cool, thanks a lot for your effort. Highly appreciated.