Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add bracketCancellable #1317

Merged
merged 2 commits into from
Nov 11, 2018

Conversation

SystemFw
Copy link
Collaborator

@SystemFw SystemFw commented Nov 9, 2018

Useful for cases like #1312
Better have something like this in the public than creating an Algebra node in a high level operation like groupWithin

@SystemFw SystemFw requested a review from mpilquist November 9, 2018 23:37
def bracketCaseCancellable[F[x] >: Pure[x], R](acquire: F[R])(
release: (R, ExitCase[Throwable]) => F[Unit]): Stream[F, (Stream[F, INothing], R)] =
bracketWithToken(acquire)(release).map {
case (token, r) => (Stream.fromFreeC(Algebra.release(token, None)), r)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about the None here

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've come to the conclusion that None is fine there, given that the canceler stream we are building here is not linked to the termination of the bracketed stream, and actually can't be because that defeats the point of an early finaliser. I therefore think this is ready for merge.

@SystemFw
Copy link
Collaborator Author

SystemFw commented Nov 10, 2018

I decided to change the finaliser from Stream[F, INothing] to Stream[F, Unit], because the former proved error prone and counterintuitive, e.g in

...
Stream.emit(1).flatTap(releaseStreamFromBracketCancellable)

Which would compile and return an empty stream.

On a separate note, it's not clear whether the initial motivation for making this combinator public (a leak in groupWithin) is actually fixable with this, so we also need to decide if having this in the public api is useful in general

@mpilquist mpilquist merged commit cecdba8 into typelevel:series/1.0 Nov 11, 2018
@mpilquist mpilquist added this to the 1.0.1 milestone Dec 3, 2018
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants