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

Add Bracket type class #113

Merged
merged 56 commits into from Apr 16, 2018

Conversation

@LukaJCB
Collaborator

LukaJCB commented Jan 10, 2018

Some WIP code I came up with, at some point the future, this might fix #88.

Luka Jacobowitz
@SystemFw

This comment has been minimized.

Collaborator

SystemFw commented Jan 10, 2018

Obviously this is just a WIP so mine it's a discussion item and not an actual code review, but this is not the model of bracket I have in mind.
Let's forget interruption and concurrency for a second, but even in the simple sync case, this initial model of bracket can only return IO[Unit], not IO[B].
Consider what happens with your current implementation:

def getFile = bracket(openFile)(file => file.pure[IO])((file,_) => IO(file.close))
def uhOh = getFile.flaMap(f => IO(f.readFirstLine))

by the time we try reading, the file is closed already.
Your type signature is indeed what I would want, but the implementation needs to track currently opened resources across IO operations, and close them appropriately.

P.S. Apologies if this is already perfectly clear to you, but I thought it would be good to specify it anyway for other readers/just in case

@LukaJCB

This comment has been minimized.

Collaborator

LukaJCB commented Jan 10, 2018

Ah you're right, that's a problem indeed, didn't think about that.

@SystemFw

This comment has been minimized.

Collaborator

SystemFw commented Jan 10, 2018

Another thing to keep in mind is that taking an A in the release action incorrectly assumes that acquire can't fail.

@alexandru

This comment has been minimized.

Member

alexandru commented Jan 10, 2018

@SystemFw wait, so let us be clear, is this something that we really want to work?

def getFile = bracket(openFile)(file => file.pure[IO])((file,_) => IO(file.close))
def uhOh = getFile.flaMap(f => IO(f.readFirstLine))

How would that even work? When is f released exactly?

Are you sure you're not thinking of FS2's Stream or Iterant, for which this version of bracket makes sense, due to working with streams?

@LukaJCB

This comment has been minimized.

Collaborator

LukaJCB commented Jan 10, 2018

@SystemFw My assumption on this, is that when acquire fails, it short circuits completely and none of the functions actually get called, no?

SoIO.raiseError(e).bracket(use)(release) wouldn't actually use the resource at all and therefore doesn't need to release it either.

@alexandru

This comment has been minimized.

Member

alexandru commented Jan 10, 2018

On an acquire that fails, there's nothing to release since the resource was not allocated. If the user does not want that, then it is trivial to add error handling on the acquire action.

@alexandru

This comment has been minimized.

Member

alexandru commented Jan 10, 2018

Just to be clear guys, in my mind bracket is the exact equivalent of this:

val resource = acquire()
try {
  use(resource)
} finally {
  release(resource)
}

If you have something else in mind, then we need to translate it in code like this.

@johnynek

This comment has been minimized.

johnynek commented Jan 10, 2018

I don't see how without either linear types or returning F[Unit] we can prevent the kind of error @SystemFw is talking about. We have no way to guarantee A has not been put into the return type of the bracket. It would be nice to have some compiler generated type class like DoesNotReach[B, A] which only has an instance if B has no path to any value of type A. Then we could return F[B] without the worry that it includes the thing that the bracket is protecting.

@SystemFw

This comment has been minimized.

Collaborator

SystemFw commented Jan 10, 2018

Ah, you're right! Well, good to have that clarified early on :)

@SystemFw

This comment has been minimized.

Collaborator

SystemFw commented Jan 10, 2018

@alexandru

Are you sure you're not thinking of FS2's Stream or Iterant, for which this version of bracket makes sense, due to working with streams?

Yep, got my wires crossed and was indeed thinking of that, I should probably change topic for today 😄

EDIT: fwiw, I want that bracket will run the finaliser if use is interrupted, not the behaviour described above which you can have with Stream

trait Bracket[F[_], E] extends MonadError[F, E] {
def bracket[A, B](acquire: F[A])(use: A => F[B])
(release: (A, Either[Throwable, B]) => F[Unit]): F[B]

This comment has been minimized.

@alexandru

alexandru Jan 10, 2018

Member

A good start, just 2 things:

  1. minor mistake: you have an E parameter, so you can't use Throwable anymore
  2. we need to be forward thinking and allow cancellation behavior in release — e.g. Monix does not emit an error on cancellation, possibly diverging from other designs and the error type is not enough to distinguish between termination in error and cancellation anyway

I propose this:

trait Bracket[F[_], E] extends MonadError[F, E] {
  def bracket[A, B](acquire: F[A])
    (use: A => F[B])
    (release: (A, Option[Either[E, B]]) => F[Unit]): F[B] 
}

This comment has been minimized.

@alexandru

alexandru Jan 10, 2018

Member

You'll notice that the current cats.effect.IO will never emit None. Which is actually quite OK, given that this IO can't be cancelled, seems fitting.

import cats.implicits._
import cats.laws._
trait BracketLaws[F[_], E] extends MonadErrorLaws[F, E] {

This comment has been minimized.

@johnynek

johnynek Jan 10, 2018

can you add a comment giving an example of why all MonadError does not satify these laws.

etb <- eta.fold(IO.raiseError, use).attempt
_ <- eta.fold(_ => IO.unit, a => release(a, etb))
b <- etb.fold(IO.raiseError, b => IO(b))
} yield b

This comment has been minimized.

@johnynek

johnynek Jan 10, 2018

in library code, I prefer to write things the most efficient way, Wonder about all the use of fold and allocation of closures here. What about:

for {
  a <- this
  etb <- use(a).attempt
  _ <- release(a, etb)
  b <- IO.fromEither(etb)
} yield b

Also note, IO.fromEither would probably be better as:

def fromEither[A](e: Either[Throwable, A]): IO[A] =
  e match {
    case Right(a) => pure(a)
    case Left(err) => raiseError(err)
  }

and avoid the allocations of the closures.

Luka Jacobowitz added some commits Jan 10, 2018

Luka Jacobowitz
Luka Jacobowitz
@alexandru

This comment has been minimized.

Member

alexandru commented Jan 12, 2018

@LukaJCB I'm very happy about this proposal, however it needs to distinguish cancellation, or lets say for now the notion of non-termination:

def bracket[A, B](acquire: F[A])
  (use: A => F[B])
  (release: (A, Option[Either[E, B]]) => F[Unit]): F[B] 

Otherwise it's not very useful for me and I now need it for Iterant.
Any objections to this signature?

@SystemFw

This comment has been minimized.

Collaborator

SystemFw commented Jan 12, 2018

@alexandru
No objections, but I would be interested if you could expand (when you have time) on

it's enough to say that I believe a cancelled task should be non-terminating

Since you also mention non-termination here :)

@alexandru

This comment has been minimized.

Member

alexandru commented Jan 12, 2018

@SystemFw I don't have the energy atm, I'd like to postpone that one, because I'm arguing for async abstractions that have been in Java and .NET and against Haskell and that's not a good place to be in 😀

IMO receiving a Throwable for cancellation is irrelevant in release, since you can't act much on it; maybe there are use-cases that I don't see, hence why I prefer Option because it's simpler.

Either way, the ability to distinguish between cancellation and normal termination is a must that needs to be in the signature. If the signature is now too complicated, we can add helpers as well

@SystemFw

This comment has been minimized.

Collaborator

SystemFw commented Jan 12, 2018

I don't have the energy atm,

Yeah I sensed that, that's why I added "when you have time" 😄
I'm not coming from a place of arguing against it, just genuinely interested in your opinion ;)

Either way, the ability to distinguish between cancellation and normal termination is a must that needs to be in the signature

Yep 👍

@LukaJCB

This comment has been minimized.

Collaborator

LukaJCB commented Jan 12, 2018

@alexandru I agree, that failure and cancellation should be separate, but didn't implement it yet, because I'm not sure if an option of either is the best encoding of that and when or if we come up with something different I don't have to change all of the code :)

For now I have no better suggestion than what you suggested, however :D

@pchlupacek

This comment has been minimized.

Collaborator

pchlupacek commented Jan 12, 2018

I am perhaps missing something, but how you would get B as result when you cancel it ?

@alexandru

This comment has been minimized.

Member

alexandru commented Jan 12, 2018

@pchlupacek you don't, you can either get None, Some(Right(b)) or Some(Left(error)).

@LukaJCB

This comment has been minimized.

Collaborator

LukaJCB commented Jan 12, 2018

@pchlupacek You don't, you'd get a None (in the case @alexandru suggested).

I was thinking just now, maybe it should be three different functions?

def bracket[A, B](acquire: F[A])
  (use: A => F[B])
  (releaseOnError: (A, E) => F[Unit])
  (releaseOnSuccess: (A, B) => F[Unit])
  (releaseOnCancel: A => F[Unit]): F[B]

WDYT? Doesn't have to be curried in this way btw.

@pchlupacek

This comment has been minimized.

Collaborator

pchlupacek commented Jan 12, 2018

I am still a confused here, bracket's result is F[B] if I am reading correctly so how you get that B when you cancel use or acquire ?

@alexandru

This comment has been minimized.

Member

alexandru commented Jan 12, 2018

So we have 3 choices:

  1. Option, which I think is simple and clear and I prefer it
  2. Either[Throwable, Either[Throwable, B]] — which nobody will understand unless they read the docs
  3. we introduce our own ADT with 3 states — Success(B), Error(E) and Cancelled(Option[E])

If you want to accommodate all possible variations, then do number 3.
But I'd like to know of a usecase for Cancelled(Some(error)).

@alexandru

This comment has been minimized.

Member

alexandru commented Jan 12, 2018

OK, so the problem with different functions is that you don't understand from the signature that only one of those 3 functions will get called, whereas if we do a single function then it's pretty clear due to the input.

@pchlupacek

This comment has been minimized.

Collaborator

pchlupacek commented Jan 12, 2018

Also the signature assumes you may cancel after acquire, but how you would like to resovle asynchronous acquire? That won't be cancellable?

@alexandru

This comment has been minimized.

Member

alexandru commented Jan 12, 2018

Also the signature assumes you may cancel after acquire, but how you would like to resovle asynchronous acquire? That won't be cancellable?

That's not a problem because if acquire fails or is cancelled, there's nothing to release and the user can always do error handling for the acquire task specifically. bracket is the equivalent of this:

val resource = acquire()
try {
  use(resource)
} finally {
  release(resource)
}

Obviously you can't put val resource = acquire() inside of try.

@pchlupacek

This comment has been minimized.

Collaborator

pchlupacek commented Jan 12, 2018

yeh, I am on board with that, hence you just release the resource when it is acquired. Agree when it fails, it fails. Having said that how you plan to produce B when you cancel use ?

@LukaJCB

This comment has been minimized.

Collaborator

LukaJCB commented Apr 15, 2018

I'll change it as soon as I'm home 👍

@alexandru

This comment has been minimized.

Member

alexandru commented Apr 15, 2018

@LukaJCB no worries, I'm going to push that in another PR, along with added documentation for the microsite.

Luka Jacobowitz added some commits Apr 15, 2018

@LukaJCB LukaJCB force-pushed the LukaJCB:add-bracket branch from 7cc7d1d to 646ec4c Apr 15, 2018

@LukaJCB

This comment has been minimized.

Collaborator

LukaJCB commented Apr 15, 2018

Well, it's already done 😄 Thanks again!

@alexandru

This comment has been minimized.

Member

alexandru commented Apr 15, 2018

Ah, OK, cool. Will follow up with another PR with some changes anyway 😜

@LukaJCB

This comment has been minimized.

Collaborator

LukaJCB commented Apr 15, 2018

So CI looks good and I think this PR is mergeable now, anyone care for a re-review? @rossabaker @pchlupacek @SystemFw

alexandru and others added some commits Apr 15, 2018

Merge pull request #3 from alexandru/LukaJCB-add-bracket2
Bracket changes: rename bracketE, add microsite docs

@alexandru alexandru changed the title from WIP: Add Bracket type class to Add Bracket type class Apr 15, 2018

@alexandru

This comment has been minimized.

Member

alexandru commented Apr 15, 2018

Well, I also contributed to it, so I can't approve it 😄

One thing to consider is that this is a blocker for the scheduled 1.0.0-RC.
And I'd really like to do the release tomorrow.

After this gets merged, we'll release a hash version first, to ensure that the laws are passing in Monix as well — laws tend to be written assuming IO's current implementation and miss certain details, especially for the side effects, a problem that I think we got rid of by introducing Pledge in those laws, as an alternative to working with var.

LukaJCB and others added some commits Apr 15, 2018

Luka Jacobowitz
### bracketCase
The `bracketCase` operation is the generalized `bracket`, also receiving
an `ExitCode` in `release` in order to distinguish between:

This comment has been minimized.

@durban

durban Apr 15, 2018

Contributor

-> ExitCase

This comment has been minimized.

@LukaJCB

LukaJCB Apr 15, 2018

Collaborator

Thank you, fixed!

Luka Jacobowitz
* (via `MonadError[F, E]`)
* - [[ExitCase$.Canceled Canceled]]: for abortion
*/
sealed abstract class ExitCase[+E]

This comment has been minimized.

@alexandru

alexandru Apr 15, 2018

Member

Damn, this needs to implement Serializable at least, maybe Product too 😐

This comment has been minimized.

@LukaJCB

LukaJCB Apr 15, 2018

Collaborator

On it!

Luka Jacobowitz added some commits Apr 15, 2018

Luka Jacobowitz
Luka Jacobowitz
@rossabaker

Given that "bracket" is already in extensive and incompatible use in the streaming types (e.g., fs2.Stream. monix.tail.Iterant), I think we might consider a documentation about why this bracket is not that bracket. I don't think it needs to block this merge, but I fully anticipate it being an FAQ.

* (via `MonadError[F, E]`)
* - [[ExitCase$.Canceled Canceled]]: for abortion
*/
sealed abstract class ExitCase[+E] extends Serializable

This comment has been minimized.

@rossabaker

rossabaker Apr 15, 2018

Member

I imagine we want Product here, too?

@alexandru

This comment has been minimized.

Member

alexandru commented Apr 16, 2018

@rossabaker you're right, we'll need a FAQ of some sort.

Well, I don't view the bracket on those streaming types to be incompatible, it's the same spirit, but the signature is a little different. We'll need to mention it and explain the difference though.

Can it happen after the merge?

@rossabaker

This comment has been minimized.

Member

rossabaker commented Apr 16, 2018

Yes, I don't want the FAQ to delay the merge.

I tried to think of a nice alternate name for either this bracket or Stream/Iterant style bracket, and failed. They both seem pretty well established under this name.

@alexandru alexandru merged commit 6e5a992 into typelevel:master Apr 16, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment