Properly propogate Writer and State effects in bracketCase #374

LukaJCB · 2018-09-24T13:57:13Z

Should fix #371.

With this change:

scala> WriterT(IO(1 -> "asd"))
res0: cats.data.WriterT[cats.effect.IO,Int,String] = WriterT(IO$2102705681)

scala> Sync[WriterT[IO, Int, ?]].bracket(res0)(s => WriterT(IO(2 -> "2")))(s => WriterT(IO( (7, ()) )))
res1: cats.data.WriterT[cats.effect.IO,Int,String] = WriterT(IO$1115955315)

scala> res1.run
res2: cats.effect.IO[(Int, String)] = IO$1115955315

scala> res2.unsafeRunSync
res2: (Int, String) = (10,2)

Without this change:

scala> WriterT(IO(1 -> "asd"))
res0: cats.data.WriterT[cats.effect.IO,Int,String] = WriterT(IO$62720203982)

scala> Sync[WriterT[IO, Int, ?]].bracket(res0)(s => WriterT(IO(2 -> "2")))(s => WriterT(IO( (7, ()) )))
res1: cats.data.WriterT[cats.effect.IO,Int,String] = WriterT(IO$31215753453)

scala> res1.run
res2: cats.effect.IO[(Int, String)] = IO$297120624

scala> res2.unsafeRunSync
res2: (Int, String) = (3,2)

codecov-io · 2018-09-24T14:07:05Z

Codecov Report

Merging #374 into master will increase coverage by 0.91%.
The diff coverage is 97.87%.

@@            Coverage Diff             @@
##           master     #374      +/-   ##
==========================================
+ Coverage   88.56%   89.48%   +0.91%     
==========================================
  Files          69       69              
  Lines        1802     1845      +43     
  Branches      197      192       -5     
==========================================
+ Hits         1596     1651      +55     
+ Misses        206      194      -12

catostrophe · 2018-09-24T23:42:15Z

The same technique can be applied to the other transformers: EitherT, OptionT and IorT (#365).

catostrophe · 2018-09-24T23:43:30Z

Async.asyncF has the same issue.

alexandru · 2018-09-25T07:53:06Z

core/shared/src/main/scala/cats/effect/Sync.scala

-            release(a, res).value
+      (release: (A, ExitCase[Throwable]) => WriterT[F, L, Unit]): WriterT[F, L, B] =
+        WriterT.liftF(Ref.of[F, Option[L]](None)).flatMap { ref =>
+          uncancelable(acquire).flatMap { a =>


This construct is not correct: uncancelable(acquire).flatMap

The gist is this: a task that allocates a resource, like acquire, always has to be in the context of a bracket. In this case it is not and that flatMap can be cancelled via the auto-cancelation mechanisms. The problem is that it can't be reproduced with cats.effect.IO, only because cats.effect.IO does not auto-fork.

If we cannot fix it somehow, then this instance needs to be scheduled for deletion in version 2.0 because it is broken.

@oleg-py can you also take a look when you've got some time?

Also, worse case scenario we add a @deprecated tag to it warning people that it is broken.

Good catch, this is certainly broken.

I think we could make release idempotent here and use bracket second time instead of uncancelable, if we want to keep this instance.

alexandru · 2018-09-26T14:46:00Z

As a manifestation of what I was saying, the problem with uncancelable(acquire).flatMap is now reproducible in PR #376

I had to deactivate WriterT's instance tests because WriterT is now unlawful.

LukaJCB · 2018-09-26T14:50:21Z

Shall we merge #376 first and then fix the instance with this PR?

alexandru · 2018-09-26T15:11:47Z

Yes, that order sounds good as now we can really see the problem.

vpavkin · 2018-10-05T14:09:50Z

core/shared/src/main/scala/cats/effect/Sync.scala

-          F.bracketCase(F.pure(a))(use.andThen(_.run)){ (a, res) =>
-            release(a, res).value
+      (release: (A, ExitCase[Throwable]) => WriterT[F, L, Unit]): WriterT[F, L, B] =
+        WriterT.liftF(Ref.of[F, Option[L]](None)).flatMap { ref =>


Since we have a Monoid, can't we just use Ref.of[F, L](L.empty) and save on Option ?

Actually I don't think this will work, given the fact that we couldn't check if an L is empty, like we can with Option (atleast without requiring an extra Eq[L]).

You mean that getOrElse in the end?
I see. Didn't notice that, thanks for clearing this up for me 👍

alexandru · 2018-10-07T07:11:54Z

I have merged #376 in master and would like to do a release soon.

Any progress on this?

catostrophe · 2018-10-07T08:04:55Z

Have we come to a consensus on the transformer effect propagation in bracket's release and Async.asyncF? Why does this PR fix only WriterT and StateT instances and only for Sync? For any transformer, its own effect can be temporarily stored into a Ref and added (flatTapped) to the result later.

I'm working on #365 and it's not clear for me what strategy for IorT effect propagation should be chosen in bracket's release and asyncF. Current PR just ignores it as in other instances. But I prepared a draft (not pushed yet) where I take it into account.

Can we clarify sematics?

LukaJCB · 2018-10-08T10:13:14Z

@catostrophe
I haven't looked at asyncF yet, but we need Sync in order to use Ref. I think we could probably use Ref for EitherT as well, but I haven't tried yet. I'll try to work on it soon :)

LukaJCB · 2018-10-08T11:11:50Z

Added OptionT and EitherT:

scala> OptionT(IO(Option(1))) ^
res0: cats.data.OptionT[cats.effect.IO,Int] = OptionT(IO$1900219430)

scala> Sync[OptionT[IO, ?]].bracket(res0)(_ => OptionT(IO(Option(2))))(_ => OptionT.none[IO, Unit])
res1: cats.data.OptionT[cats.effect.IO,Int] = OptionT(IO$1768249421)

scala> res1.value.unsafeRunSync
res2: Option[Int] = None

scala> EitherT(IO(Either.right[String, Int](1)))
res0: cats.data.EitherT[cats.effect.IO,String,Int] = EitherT(IO$983210249)

scala> Sync[EitherT[IO, String, ?]].bracket(res10)(_ => EitherT(IO(Either.right[String, Int](2))))(_ => EitherT(IO(Either.left[String, Unit]("Hi"))))
res1: cats.data.EitherT[cats.effect.IO,String,Int] = EitherT(IO$111852734)

scala> res1.value.unsafeRunSync
res2: Either[String,Int] = Left(Hi)

catostrophe · 2018-10-08T19:37:37Z

core/shared/src/main/scala/cats/effect/Sync.scala

-            F.unit // nothing to release
-        }
-      })
+      EitherT.liftF(Ref.of[F, Option[L]](None)).flatMap { ref =>


catostrophe · 2018-10-08T19:39:41Z

core/shared/src/main/scala/cats/effect/Sync.scala

-          F.unit
-      })
+      //Boolean represents if release returned None
+      OptionT.liftF(Ref.of[F, Boolean](false)).flatMap { ref =>


catostrophe · 2018-10-08T19:40:20Z

core/shared/src/main/scala/cats/effect/Sync.scala

-        } { case ((s, a), br) =>
-          release(a, br).run(s).void
+      (release: (A, ExitCase[Throwable]) => StateT[F, S, Unit]): StateT[F, S, B] =
+        StateT.liftF(Ref.of[F, Option[S]](None)).flatMap { ref =>


catostrophe · 2018-10-08T19:42:13Z

core/shared/src/main/scala/cats/effect/Sync.scala

+      (release: (A, ExitCase[Throwable]) => WriterT[F, L, Unit]): WriterT[F, L, B] =
+        WriterT.liftF(Ref.of[F, Option[L]](None)).flatMap { ref =>
+          acquire.flatMap { a =>
+            WriterT(


alexandru · 2018-11-09T12:26:45Z

Guys, no progress on these instances?

I my PR I introduced this:

@deprecated("WARNING: currently the Async[WriterT[F, L, ?]] instance is broken!", "1.1.0")

For a 1.1.0 release we need to make a decision — do we fix these instances, or do we deprecate them?

Can we fix them by any means? Can you make progress on it?

catostrophe · 2018-11-09T12:33:38Z

This issues seems to be fixed. We only need to finish the review.

LukaJCB · 2018-11-09T12:37:07Z

I'm not sure its fixed unfortunately, the issue with propogation in release is fixed, but the underlying issue with making acquire uncancelable is still there as can be seen by the failing tests in CI :(

catostrophe · 2018-11-19T06:44:18Z

This should fix the failing tests:

import cats.instances.tuple._

def bracketCase[A, B](acquire: WriterT[F, L, A])
  (use: A => WriterT[F, L, B])
  (release: (A, ExitCase[Throwable]) => WriterT[F, L, Unit]): WriterT[F, L, B] =
  WriterT(
    Ref[F].of(none[L]).flatMap { ref =>
      F.bracketCase(acquire.run) { la =>
        WriterT(la.pure[F]).flatMap(use).run
      } { case ((_, a), ec) =>
        val r = release(a, ec).run
        if (ec == ExitCase.Completed)
          r flatMap { case (l, _) => ref.set(l.some) }
        else
          r.void
      }.flatMap { lb =>
        ref.get.map(_.fold(lb)(l => lb.leftMap(_ |+| l)))
      }
    }
  )

catostrophe · 2018-11-19T07:44:09Z

Oh, and Option[L] can be replaced with L here, as @vpavkin noticed. I don't compare it with empty.

Something like:

import cats.instances.tuple._

def bracketCase[A, B](acquire: WriterT[F, L, A])
  (use: A => WriterT[F, L, B])
  (release: (A, ExitCase[Throwable]) => WriterT[F, L, Unit]): WriterT[F, L, B] =
  WriterT(
    Ref[F].of(L.empty).flatMap { ref =>
      F.bracketCase(acquire.run) { la =>
        WriterT(la.pure[F]).flatMap(use).run
      } { case ((_, a), ec) =>
        val r = release(a, ec).run
        if (ec == ExitCase.Completed)
          r flatMap { case (l, _) => ref.set(l) }
        else
          r.void
      }.flatMap { lb =>
        ref.get.map(l => lb.leftMap(_ |+| l))
      }
    }
  )

LukaJCB · 2018-11-19T09:56:20Z

CI seems to be happy and the release part still propogates, thanks so much @catostrophe!

alexandru · 2018-11-19T11:12:18Z

🎉

Properly propogate Writer and State effects in bracketCase

865b157

alexandru reviewed Sep 25, 2018

View reviewed changes

alexandru mentioned this pull request Sep 25, 2018

Cats Effect 3.0 Design Discussion #321

Closed

vpavkin reviewed Oct 5, 2018

View reviewed changes

Merge branch 'umaster' into concurrent-state

30c668f

LukaJCB force-pushed the concurrent-state branch from e74153c to 30c668f Compare October 8, 2018 10:27

Update OptionT and EitherT instances

7ca92e1

Undeprecate WriterT instances and reenable tests

64ecd44

catostrophe reviewed Oct 8, 2018

View reviewed changes

catostrophe mentioned this pull request Oct 13, 2018

Bracket implementations for all transformers ignore aux effects in release #371

Closed

Luka Jacobowitz added 2 commits November 9, 2018 13:32

Merge branch 'umaster' into concurrent-state

23951ef

Merge branch 'umaster' into concurrent-state

3325b42

Fix WriterT

d1a5a40

alexandru merged commit 9b7dba1 into typelevel:master Nov 22, 2018

LukaJCB deleted the concurrent-state branch November 22, 2018 08:45

This was referenced Nov 25, 2018

Undeprecate WriterT instance #427

Merged

Add IorT instances #365

Merged

SystemFw mentioned this pull request Mar 25, 2019

Remove unnecessary code from Sync[OptionT] #500

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Properly propogate Writer and State effects in bracketCase #374

Properly propogate Writer and State effects in bracketCase #374

LukaJCB commented Sep 24, 2018

codecov-io commented Sep 24, 2018 •

edited

Loading

catostrophe commented Sep 24, 2018

catostrophe commented Sep 24, 2018

alexandru Sep 25, 2018

alexandru Sep 25, 2018 •

edited

Loading

oleg-py Sep 25, 2018

alexandru commented Sep 26, 2018 •

edited

Loading

LukaJCB commented Sep 26, 2018

alexandru commented Sep 26, 2018

vpavkin Oct 5, 2018

LukaJCB Oct 8, 2018

vpavkin Oct 8, 2018

alexandru commented Oct 7, 2018

catostrophe commented Oct 7, 2018

LukaJCB commented Oct 8, 2018

LukaJCB commented Oct 8, 2018 •

edited

Loading

catostrophe Oct 8, 2018

catostrophe Oct 8, 2018

catostrophe Oct 8, 2018

catostrophe Oct 8, 2018

alexandru commented Nov 9, 2018

catostrophe commented Nov 9, 2018

LukaJCB commented Nov 9, 2018

catostrophe commented Nov 19, 2018

catostrophe commented Nov 19, 2018

LukaJCB commented Nov 19, 2018

alexandru commented Nov 19, 2018

Properly propogate Writer and State effects in bracketCase #374

Properly propogate Writer and State effects in bracketCase #374

Conversation

LukaJCB commented Sep 24, 2018

codecov-io commented Sep 24, 2018 • edited Loading

Codecov Report

catostrophe commented Sep 24, 2018

catostrophe commented Sep 24, 2018

Choose a reason for hiding this comment

alexandru Sep 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexandru commented Sep 26, 2018 • edited Loading

LukaJCB commented Sep 26, 2018

alexandru commented Sep 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexandru commented Oct 7, 2018

catostrophe commented Oct 7, 2018

LukaJCB commented Oct 8, 2018

LukaJCB commented Oct 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexandru commented Nov 9, 2018

catostrophe commented Nov 9, 2018

LukaJCB commented Nov 9, 2018

catostrophe commented Nov 19, 2018

catostrophe commented Nov 19, 2018

LukaJCB commented Nov 19, 2018

alexandru commented Nov 19, 2018

codecov-io commented Sep 24, 2018 •

edited

Loading

alexandru Sep 25, 2018 •

edited

Loading

alexandru commented Sep 26, 2018 •

edited

Loading

LukaJCB commented Oct 8, 2018 •

edited

Loading