Move uncancelable down to Bracket. Add laws specifying interaction. #241

oleg-py · 2018-05-23T19:38:07Z

A little cherry-pick from discussions in #192: since we require uninterruptible acquisition and cleanup, uncancelable can be described in terms of Bracket.

The rationale for pushing it to typeclass is two-fold:

it can be overriden for concrete implementations with a more optimal version
effectful code willing to run something uninterruptibly is able to express that, given only Sync - no need to ask for Concurrent, which might be a problem to reliably do after Require Timer and shift automatically in concurrent IO operations #232

Additionally, I added ensuring as a helper method I've found myself reinventing few times, and IIRC @alexandru mentioned it should probably be added :)

I had to remove the uncancelable on Concurrent (causes VerifyErrors for IO instance), so I moved the doc to the class level.

There's also an oddity for Kleisli: looks like we assumed the function could be side-effecting, so we suspended the uncancelable. Not sure what to do with that :)

johnynek · 2018-05-23T20:59:54Z

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

+   * }}}
+   */
+  def uncancelable[A](task: F[A]): F[A] =
+    bracket(task)(pure)(_ => unit)


due to this law:
https://github.com/typelevel/cats-effect/pull/241/files#diff-321005aff53c79253fc4a54b188dc3b4R30hh

And the monad law f.flatMap(pure(_)) == f, isn't this the same as identity?

What am I missing?

Clicking on the link gets me nowhere :)
You're missing the interaction with data types that support cancellation, as they need to ensure that acquire and release cannot be cancelled

N.B. this is in the model that supports auto-cancelable flatMaps.

Basically if you have auto-cancelable flatMaps, there's no way to implement bracket without an acquire that's not uncancelable. It wouldn't work.

Consider that when acquiring a resource, you might do multiple flatMaps, but then the user cancels the tasks before bracket has a chance to install the finalizers. It's harder to end up in such a situation without auto-cancelable binds because as a user you can reason about how acquire behaves.

@alexandru are auto-cancelable binds required to trigger odd behavior? I thought it would be enough if acquire part contains a cancel boundary.

Yes, auto-cancelable binds are required. Example:

IO(new FileReader(file)).bracket { f => IO(f.read()) } { f => IO(f.close()) }

You can reason about this code and you don't need to ensure that acquire is uncancelable because it already is. You can see that it is.

And in case acquire is cancelable, it's usually something that's safe to cancel. Stupid sample, but you can picture:

val acquire = IO.cancelable { cb => val fh = new FileReader(file) ec.execute(() => cb(Right(f))) IO(fh.close()) } // And then we don't care about the cancelability of acquire: acquire.bracket(use)(release)

This mental model completely breaks down with auto-cancelable binds. Which is what I was ranting about in #192.

It starts from the implementation actually. It's not only desirable for acquire to be uncancelable. Rather, it's impossible to implement otherwise 😉

rossabaker · 2018-05-23T19:52:08Z

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

+   *   F.uncancelable { F.bracketCase(F.unit)(_ => fa) { case Cancelled(_) => action; case _ => action2 } } <-> F.ensuring(fa)(action2)
+   * }}}
+   */
+  def uncancelable[A](task: F[A]): F[A] =


Nit: I think we usually call these parameters fa. Makes them a bit less concrete.

rossabaker · 2018-05-23T19:53:18Z

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

+   * A special case of [[bracket]], which is not meant for resource acquisition
+   */
+  def ensuring[A](task: F[A])(finalizer: F[Unit]): F[A] =
+    bracket(unit)(_ => task)(_ => finalizer)


👍 I've written this a couple times.

rossabaker · 2018-05-23T22:37:54Z

laws/shared/src/main/scala/cats/effect/laws/BracketLaws.scala

+    }) <-> F.uncancelable(F.ensuring(fa)(onFinish))
+
+  def acquireAndReleaseAreUncancelable[A, B](fa: F[A], use: A => F[B], release: A => F[Unit]) =
+    F.bracket(F.uncancelable(fa))(use)(a => F.uncancelable(release(a))) <-> F.bracket(fa)(use)(release)


Should we have canceled F[A] in our arbitraries to make this a more interesting law?

Canceled F[A]s cannot be made uncancelable :) We need F[A]s that might get concurrently cancelled or not, which is a plenty of work that probably should be done later.

notxcain · 2018-05-24T03:09:55Z

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

+ *     _ <- fiber.cancel
+ *     _ <- fiber.join
+ *   } yield {
+ *     println("Tick!")


Maybe it should be _ <- F.delay(println("Tick!"))? Just to avoid bad practices in docs ;)

alexandru · 2018-05-24T07:52:43Z

laws/shared/src/main/scala/cats/effect/laws/BracketLaws.scala

@@ -38,6 +38,21 @@ trait BracketLaws[F[_], E] extends MonadErrorLaws[F, E] {

  def bracketIsDerivedFromBracketCase[A, B](fa: F[A], use: A => F[B], release: A => F[Unit]) =
    F.bracket(fa)(use)(release) <-> F.bracketCase(fa)(use)((a, _) => release(a))
+
+  def uncancelableIsDerivedFromBracket[A, B](fa: F[A]) =
+    F.uncancelable(fa) <-> F.bracket(fa)(F.pure)(_ => F.unit)


This law is useful for us to read, but it's not useful in terms of tests via ScalaCheck.

We need something stronger in ConcurrentLaws. Don't we have a law for uncancelable in ConcurrentLaws? We need one for bracket's acquire and maybe release too.

But make sure it works with Monix 😛

* renamed `task` to `fa` in signature * Stronger law on Concurrent * Made doc more pure :)

codecov-io · 2018-05-24T18:26:55Z

Codecov Report

Merging #241 into master will increase coverage by 0.33%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #241      +/-   ##
==========================================
+ Coverage   88.73%   89.07%   +0.33%     
==========================================
  Files          58       58              
  Lines        1492     1520      +28     
  Branches      145      157      +12     
==========================================
+ Hits         1324     1354      +30     
+ Misses        168      166       -2

johnynek · 2018-05-24T18:34:27Z

some current laws:

// law 0
F.bracket(fa)(use)(release) <-> F.bracketCase(fa)(use)((a, _) => release(a))
// law 1
F.bracketCase(fa)(f)((_, _) => F.unit) <-> F.flatMap(fa)(f)
// law 2
F.uncancelable(fa) <-> F.bracket(fa)(F.pure)(_ => F.unit)

recall the monad law: F.flatMap(fa)(F.pure(_)) <-> fa

So putting this together:

// apply law 0 to the left hand side of this equation
F.bracket(fa)(F.pure)(_ => F.unit) <-> F.bracketCase(fa)(F.pure)((_, _) => F.unit)
// apply law1 to the right hand side of above.
F.bracket(fa)(F.pure)(_ => F.unit) <-> F.flatMap(fa)(F.pure)
// apply the monad bind identity, to right hand side of above.
F.bracket(fa)(F.pure)(_ => F.unit) <-> fa
// apply law2 to the left hand side of above.
F.uncancelable(fa) <-> fa

Did I make a mistake? All lawful instances have to be identity according to these laws if I didn't.

oleg-py · 2018-05-24T18:40:22Z

Law 1 doesn't look right to me. In presence of cancelable binds and concurrent cancellation - which we don't do a great job of testing against - it can't hold.

johnynek · 2018-05-24T18:43:56Z

law 1 is here:
https://github.com/oleg-py/cats-effect/blob/82d9eb38c348ef0ac23e954bbb4281b3a0991bb2/laws/shared/src/main/scala/cats/effect/laws/BracketLaws.scala#L31

oleg-py · 2018-05-25T07:24:12Z

@johnynek yeah, it needs to be restated, since now acquire has additional semantics. Thanks for noticing 👍

alexandru · 2018-05-25T09:06:53Z

@oleg-py ensuring might be a bad name, because we already have ensure on MonadError, see: https://github.com/typelevel/cats/blob/master/core/src/main/scala/cats/MonadError.scala#L13

What a waste 🙂 release would be an alternative. It mirrors the naming on bracket. finally would have been great, but it's a reserved word. Or we could keep ensuring and ignore ensure on MonadError.

That said, can you think of any sample for which ensuring is useful?

Right now all I can think of is that on usage of ensuring you're tying the source's termination to a finalizer that releases an outer resource and given my new explanation for bracket, I can't help but think that many use-cases are invalid, as you can't really rely on an IO task to release resources that were not acquired in the same bracket call.

alexandru · 2018-05-25T09:09:46Z

Or in other words, whenever there's a call like:

F.bracket(F.unit)(_ => task)(_ => release)

I'm getting the feeling that all such calls are either not valid under auto-cancelable binds or very error prone (e.g. requiring idempotency on evaluating release).

oleg-py · 2018-05-25T09:56:54Z

Good point about the name. I'll change it to guarantee then, as was attempted to be done previously 😄

Besides, stdlib has its own ensuring which throws assertion errors, added by implicit conversion.

My use-case for it is not for resource handling, but for doing effecty stuff like logging or sending a message once the task has successfully completed. Previously I did .attempt in for-comprehension followed by rethrow, but that didn't work well with transformers for obvious reasons.

That said, it's rare enough that I can live without, but it's a convenience I'd like to have baked in (the whole thing that attracted me in FP in the first place was having tons of such generic conveniences)

alexandru · 2018-05-25T10:22:52Z

It’s fine, we can have it 👍 and guarantee sounds good.

mpilquist · 2018-05-25T11:59:36Z

Note we call this `onFinalize` in fs2.

…

On May 25, 2018, at 6:22 AM, Alexandru Nedelcu ***@***.***> wrote: It’s fine, we can have it 👍 and guarantee sounds good. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

alexandru · 2018-05-25T14:35:24Z

laws/shared/src/main/scala/cats/effect/laws/ConcurrentLaws.scala

+    val lh =
+      for {
+        mVar <- F.liftIO(MVar[IO].of(b1))
+        task = F.bracket(F.unit)(_ => F.unit)(_ => F.liftIO(mVar.put(b2)))


The use in this case should probably be F.never to prevent release from going off without a fiber.cancel.

alexandru

LGTM

alexandru

Ah, wait, so you did not change IO's implementation?

If the laws aren't failing our IO, we have a problem somewhere.

See implementation: https://github.com/typelevel/cats-effect/blob/master/core/shared/src/main/scala/cats/effect/internals/IOBracket.scala#L33

alexandru · 2018-05-27T03:38:49Z

laws/shared/src/main/scala/cats/effect/laws/ConcurrentLaws.scala

+  def acquireIsNotCancelable[A](a1: A, a2: A) = {
+    val lh =
+      for {
+        mVar <- F.liftIO(MVar[IO].of(a1))


Seeing this MVar[IO] used via F.liftIO makes me uncomfortable. Why are we doing that, instead of working with MVar[F] directly?

alexandru · 2018-05-27T03:40:10Z

laws/shared/src/main/scala/cats/effect/laws/ConcurrentLaws.scala

+        task = F.bracket(F.liftIO(mVar.put(a2)))(_ => F.never[A])(_ => F.unit)
+        fiber <- F.start(task)
+        _     <- fiber.cancel
+        _     <- F.liftIO(ioTimer.shift)


I haven't paid attention to the shift PR, I guess that's when the ioTimer happened, but why do we have an ioTimer in ConcurrentLaws?

If anything, we need a Timer[F]. If a Timer[IO] is needed to test F[_], that kind of smells badly.

We use Deferred[IO, ?] to test cancelable builder. Building that Deferred requires Concurrent[IO], which now requires Timer[IO]. And there's no way to get Timer[IO] from Timer[F] in Concurrent

Deferred works with just Async. Do we really need a cancelable Deferred in the cancelable builder?

Seeing that Timer[IO] in there seems odd given we are testing F[_].

oleg-py · 2018-05-27T15:00:40Z

@alexandru seems we're using Deferred[IO, ?] only as a latch to check that cancel was called, so cancelability of get is not required. So I made the change to Timer[F]

alexandru · 2018-05-27T15:30:50Z

Looking good 👍

Move uncancelable down to Bracket. Add laws specifying interaction.

4042696

johnynek reviewed May 23, 2018

View reviewed changes

rossabaker reviewed May 23, 2018

View reviewed changes

notxcain reviewed May 24, 2018

View reviewed changes

alexandru reviewed May 24, 2018

View reviewed changes

alexandru mentioned this pull request May 24, 2018

Restate ConcurrentLaws to support auto-cancelable run loops #237

Merged

oleg-py added 4 commits May 24, 2018 20:53

Improvements for uncancelable

b561d17

* renamed `task` to `fa` in signature * Stronger law on Concurrent * Made doc more pure :)

More MiMa exclusions

a948c82

Add missing cancel calls in bracket-related laws

d61733a

Fix doc link to cancelable

82d9eb3

alexandru mentioned this pull request May 25, 2018

Proposal: Continual (type class?) #242

Closed

Rename ensuring to guarantee; restate flatmap equivalence

edc1ff1

alexandru reviewed May 25, 2018

View reviewed changes

Use never in ConcurrentLaws

b652942

alexandru approved these changes May 25, 2018

View reviewed changes

alexandru suggested changes May 25, 2018

View reviewed changes

Stricten ConcurrentLaws / Fix IO.bracket and WriterT.bracket

8e863ad

oleg-py mentioned this pull request May 26, 2018

Proposal: demote start and cancelable from being primitives #247

Closed

alexandru reviewed May 27, 2018

View reviewed changes

Use MVar[F] instead of MVar[IO] in tests

8c35b62

alexandru approved these changes May 27, 2018

View reviewed changes

Change Timer[IO] to Timer[F] in Concurrent(Effect) tests

c89fc0b

alexandru added 🍄 enhancement breaking change labels May 28, 2018

alexandru added this to the 1.0.0-RC2 milestone May 28, 2018

rossabaker approved these changes May 28, 2018

View reviewed changes

alexandru mentioned this pull request May 28, 2018

Optimize async trampoline, add ContextSwitch #250

Merged

alexandru merged commit bb1283e into typelevel:master May 28, 2018

alexandru mentioned this pull request May 29, 2018

Fix #251: onCancelRaiseError, guaranteeCase, ExitCase #254

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move uncancelable down to Bracket. Add laws specifying interaction. #241

Move uncancelable down to Bracket. Add laws specifying interaction. #241

oleg-py commented May 23, 2018

johnynek May 23, 2018

oleg-py May 24, 2018

alexandru May 24, 2018

oleg-py May 24, 2018 •

edited

Loading

alexandru May 24, 2018 •

edited

Loading

rossabaker May 23, 2018

rossabaker May 23, 2018

rossabaker May 23, 2018

oleg-py May 24, 2018

notxcain May 24, 2018

alexandru May 24, 2018

codecov-io commented May 24, 2018 •

edited

Loading

johnynek commented May 24, 2018 •

edited

Loading

oleg-py commented May 24, 2018

johnynek commented May 24, 2018

oleg-py commented May 25, 2018

alexandru commented May 25, 2018

alexandru commented May 25, 2018

oleg-py commented May 25, 2018

alexandru commented May 25, 2018

mpilquist commented May 25, 2018 via email

alexandru May 25, 2018

alexandru left a comment

alexandru left a comment

alexandru May 27, 2018

alexandru May 27, 2018

oleg-py May 27, 2018 •

edited

Loading

alexandru May 27, 2018

oleg-py commented May 27, 2018

alexandru commented May 27, 2018

Move uncancelable down to Bracket. Add laws specifying interaction. #241

Move uncancelable down to Bracket. Add laws specifying interaction. #241

Conversation

oleg-py commented May 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleg-py May 24, 2018 • edited Loading

Choose a reason for hiding this comment

alexandru May 24, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented May 24, 2018 • edited Loading

Codecov Report

johnynek commented May 24, 2018 • edited Loading

oleg-py commented May 24, 2018

johnynek commented May 24, 2018

oleg-py commented May 25, 2018

alexandru commented May 25, 2018

alexandru commented May 25, 2018

oleg-py commented May 25, 2018

alexandru commented May 25, 2018

mpilquist commented May 25, 2018 via email

Choose a reason for hiding this comment

alexandru left a comment

Choose a reason for hiding this comment

alexandru left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleg-py May 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleg-py commented May 27, 2018

alexandru commented May 27, 2018

oleg-py May 24, 2018 •

edited

Loading

alexandru May 24, 2018 •

edited

Loading

codecov-io commented May 24, 2018 •

edited

Loading

johnynek commented May 24, 2018 •

edited

Loading

oleg-py May 27, 2018 •

edited

Loading