Skip to content
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

Change scoping behavior to open a scope on each bracket #1574

Merged
merged 4 commits into from
Sep 5, 2019

Conversation

mpilquist
Copy link
Member

@mpilquist mpilquist commented Sep 2, 2019

Resolved #1535, alternative to #1562.

Scopes are now inserted fully automatically (no more s.scope) and are inserted around each call to bracket*. The resulting behavior is much more intuitive -- e.g. bracket(r)(..) ++ s now guarantees that r is released when s is evaluated.

One exception to the above is resources acquired in the root scope (or rather, resources acquired in a direct descendant of the root scope given that each call to bracket* creates a scope) and when using s.compile.resource. Compiling to a Resource[F, X] extends the lifetime of such resources to the resource lifecycle. E.g.,

(Stream.bracket(r1)(..) ++ Stream.bracket(r2)(..) ++ s).compile.resource.toVector.use(_ => /* r1 & r2 are not finalized yet here but will be when `use` completes */

(There are tests for this behavior in the current test suite).

To implement this exception case, this PR introduces the internal notion of soft scopes and hard scopes. A soft scope is created for each bracket call whereas a hard scope is created in all other cases. When evaluating s.compile.resource, soft scopes opened off root are skipped. This PR was updated -- instead of soft/hard scopes, we now just delay closure of last top-level scope, where top-level is defined as a direct descendant of the root scope.

@@ -43,37 +43,9 @@ final class Pull[+F[_], +O, +R] private (private val free: FreeC[Algebra[Nothing
*/
def stream(implicit ev: R <:< Unit): Stream[F, O] = {
val _ = ev
Stream.fromFreeC(this.scope.get[F, O, Unit])
Stream.fromFreeC(this.asInstanceOf[Pull[F, O, Unit]].get)
Copy link
Member Author

Choose a reason for hiding this comment

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

Note converting a Pull to a Stream no longer inserts a scope -- this is an optional part of this PR but this always felt super arbitrary to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

makes sense

*/
def acquire[F[_]: RaiseThrowable, R](r: F[R])(cleanup: R => F[Unit]): Pull[F, INothing, R] =
acquireCancellable(r)(cleanup).map(_.resource)

/**
* Like [[acquire]] but the result value consists of a cancellation
* pull and the acquired resource. Running the cancellation pull frees the resource.
* This allows the acquired resource to be released earlier than at the end of the
* containing pull scope.
*/
def acquireCancellable[F[_]: RaiseThrowable, R](r: F[R])(
Copy link
Member Author

@mpilquist mpilquist Sep 2, 2019

Choose a reason for hiding this comment

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

This should probably be deleted. I'll open a follow-up PR after this one is merged. IIRC, I added it when writing a custom pull that did log rotation -- the file handles were all opened in the same scope as a result of the custom pull. At the time, I couldn't find a way to close the old file handle when opening a new one. This was back in 0.9 with a significantly different pull API though.

@@ -1361,7 +1362,7 @@ final class Stream[+F[_], +O] private (private val free: FreeC[Algebra[Nothing,
* }}}
*/
def handleErrorWith[F2[x] >: F[x], O2 >: O](h: Throwable => Stream[F2, O2]): Stream[F2, O2] =
Stream.fromFreeC(Algebra.scope(get[F2, O2]).handleErrorWith(e => h(e).get[F2, O2]))
Stream.fromFreeC(get[F2, O2].handleErrorWith(e => h(e).get[F2, O2]))
Copy link
Member Author

Choose a reason for hiding this comment

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

No need for a scope insertion here anymore.

Copy link
Contributor

Choose a reason for hiding this comment

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

cool. I remember this was done on the interruption, when we signalled it with the Throwable. If all interruption test go green this is ok.

* Note: see the disclaimer about the use of `streamNoScope`.
*/
def scope: Stream[F, O] = Stream.fromFreeC(Algebra.scope(get))
private def scope(hard: Boolean): Stream[F, O] =
Copy link
Member Author

Choose a reason for hiding this comment

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

This is now private as there's no need for manual scope control anymore.

@@ -3966,7 +3957,7 @@ object Stream extends StreamLowPriority {

resourceEval {
F.delay(init())
.flatMap(i => Algebra.compile(s.get, scope, i)(foldChunk))
.flatMap(i => Algebra.compile(s.get, scope, true, i)(foldChunk))
Copy link
Member Author

Choose a reason for hiding this comment

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

When using s.compile.resource, we enable suppression of soft scopes (aka scopes that are a result of resource acquisition) for the root scope.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thats an ultra-magic statement :-) I think we would need to clarify it for users.

@@ -84,7 +83,8 @@ object Balance {
def through[F[_]: Concurrent, O, O2](chunkSize: Int)(pipes: Pipe[F, O, O2]*): Pipe[F, O, O2] =
_.balance(chunkSize)
.take(pipes.size)
.zipWith(Stream.emits(pipes)) { case (stream, pipe) => stream.through(pipe) }
.zipWithIndex
Copy link
Member Author

Choose a reason for hiding this comment

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

This had to change as it makes an assumption of scopes involved when zipping. With the more granular scopes used here, the leasing of the step legs was not effective.

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting. Also we do have some tests that were covering interleaving scopes. I wonder what these tests will do

@pchlupacek
Copy link
Contributor

Looks very nice @mpilquist

ecs.toList.foreach(_ shouldBe ExitCase.Canceled)
Succeeded
"interruption" in {
pending // Completes with ExitCase.Completed instead of expected Canceled
Copy link
Member Author

@mpilquist mpilquist Sep 2, 2019

Choose a reason for hiding this comment

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

This is currently broken, but I think it's for the same reason as #1531 and recent reports in Gitter -- https://gitter.im/functional-streams-for-scala/fs2?at=5d644fb6022dba538e66cdfe.

I think when there's more granular scopes, the interruption applies to innermost scope but then the parents are terminated w/ Completed. I spent an hour or so looking in to this but couldn't figure it out. I think the issue is in Algebra#scope0, where the handler for interruption closes the interrupted scope but then terminates with a Result.Pure(()).

Copy link
Contributor

Choose a reason for hiding this comment

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

I remember these was really hard to solve and reason about and took really long time to figure them out. Some of the changes on scope was purposely driven by these tests. I hope they won't block us now.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree, though I don't think the status quo is much better, assuming it's the cause of the other reported issues with ExitCase.Completed appearing where ExitCase.Canceled is expected.

Copy link
Member Author

@mpilquist mpilquist Sep 3, 2019

Choose a reason for hiding this comment

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

Okay fixed, it was a very simple issue: 04b893e

@@ -1614,6 +1615,7 @@ class StreamSpec extends Fs2Spec {
case Some((hd, tl)) => Pull.eval(IO.never)
}
.stream
.interruptScope
Copy link
Member Author

Choose a reason for hiding this comment

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

Note b/c pulls no longer introduce scopes, I had to add this manually here. I think this is the right tradeoff though.

Copy link
Contributor

Choose a reason for hiding this comment

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

agree

@djspiewak
Copy link
Member

@mpilquist Is there a particular reason that things need to be extended to the full resource scope? I guess Resource doesn't currently have any way of closing off the current scope directly, but it can be encoded thusly:

def close[F[_]: Bracket[?[_], Throwable], A](r: Resource[F, A]): Resource[F, Unit] =
  Resource.liftF(r.use(_ => unit[F]))

Does that allow you to pinch things off sooner?

@mpilquist
Copy link
Member Author

@djspiewak It's admittedly fairly ad-hoc behavior but it's to enable programs like this:

              def resource =
                Stream
                  .emit("resource - start")
                  .onFinalize(record("resource - done"))
                  .compile
                  .resource
                  .lastOrError
                  .use(x => f(x))

Here, we want onFinalize to run at the end of the returned Resource, not after emit finishes.

@djspiewak
Copy link
Member

@mpilquist Since we're in ad hoc land, can we just make the last scope extend to the end of the resource, but any prior scopes pinch off prior to that?

@mpilquist
Copy link
Member Author

@djspiewak That's a good idea and is consistent with the behavior Fabio wanted. I'll try to implement it...

@Daenyth
Copy link
Contributor

Daenyth commented Sep 3, 2019

I'm a little confused about the api here:

bracket(r)(..) ++ s now guarantees that r is released when s is evaluated.

(Stream.bracket(r)(..) ++ Stream.bracket(r2)(..) ++ s).compile.resource.toVector.use(_ => /* r1 & r2 are not finalized yet here but will be when use completes */

This implies to me:

val foo = Stream.bracket(r)(..) ++ s
foo.compile.drain // `r` is closed before `s` begins
foo.compile.resource.. // `r` is closed after `s` begins

do I have that right? If so that feels pretty dangerous and non-composable to me :(

@mpilquist
Copy link
Member Author

@Daenyth Yeah, it's definitely strange at first glance. Take a look at ScalaDoc of compile.resource for motivation.

@Daenyth
Copy link
Contributor

Daenyth commented Sep 3, 2019

What api would I use to guarantee that r is closed regardless of how the stream is compiled?

@djspiewak
Copy link
Member

@Daenyth If Mike is able to implement my proposed semantics, then r will be closed regardless of compilation.

@mpilquist
Copy link
Member Author

I just pushed a commit that extends the scope of the last top-level scope (where top-level = direct descendant of root scope). Some examples:

def r(tag: String) = Stream.bracket(IO(println("acquired " + tag)))(_ => IO(println("released " + tag)))

val a = r("1").compile.resource.lastOrError.use(_ => IO(println("using"))).unsafeRunSync
// acquired 1
// using
// released 1

val b = r("1").append(Stream(1)).compile.resource.lastOrError.use(_ => IO(println("using"))).unsafeRunSync
// acquired 1
// using
// released 1

val c = r("1").append(r("2")).compile.resource.lastOrError.use(_ => IO(println("using"))).unsafeRunSync
// acquired 1
// released 1
// acquired 2
// released 2
// acquired 3
// using
// released 3

val d = r("1").append(Stream.empty.onFinalize(IO(println("finalizer")))).compile.resource.lastOrError.use(_ => IO(println("using"))).unsafeRunSync
// acquired 1
// released 1
// using
// finalizer

@SystemFw
Copy link
Collaborator

SystemFw commented Sep 3, 2019

Still at Scala World, I'll have a few comments when I'm back but overall I'm pretty excited by how things are going :)

@mpilquist
Copy link
Member Author

mpilquist commented Sep 4, 2019

One thing to be aware of -- in 1.0.{3,4,5}, bracket and onFinalize did not introduce scopes. Hence, consider the behavior of these:

def out(s: String): IO[Unit] = IO(println(s))
Stream(1)
  .onFinalize(out("1"))
  .onFinalize(out("2"))
  .compile
  .resource
  .drain
  .use(_ => out("using"))
  .unsafeRunSync
// using
// 1
// 2

With this PR though:

def out(s: String): IO[Unit] = IO(println(s))
Stream(1)
  .onFinalize(out("1"))
  .onFinalize(out("2"))
  .compile
  .resource
  .drain
  .use(_ => out("using"))
  .unsafeRunSync
// 1
// using
// 2

The same problem exists for nested brackets -- e.g. Stream.bracket(r1)(f1).flatMap(x => Stream.bracket(r2)(f2)) -- r2 will be released "on schedule" whereas r1 will be extended over the lifetime of the resource.

Note: this isn't as bad as it looks as 1.0.{3,4,5} exhibits the same behavior if a scope is introduced between the finalizers, and in 1.0.{3,4,5}, scopes are introduced in many places implicitly. For example, the following insertion of take(2) changes the finalization such that the first finalizer is run eagerly:

Stream(1, 2, 3)
  .onFinalize(out("1"))
  .take(2)
  .onFinalize(out("2"))
  .compile
  .resource
  .drain
  .use(_ => out("using"))
  .unsafeRunSync
// 1
// using
// 2

@SystemFw
Copy link
Collaborator

SystemFw commented Sep 4, 2019

What happens with

a.concurrently(b).onFinalize(c).compile.resource

@mpilquist
Copy link
Member Author

r("1").concurrently(r("2").flatMap(_ => Stream.never[IO])).onFinalize(out("3")).compile.resource.lastOrError.use(_ => out("using")).unsafeRunSync
// acquired 1
// acquired 2
// released 1
// released 2
// using
// 3

@SystemFw
Copy link
Collaborator

SystemFw commented Sep 4, 2019

@mpilquist that doesn't work then, right?
Imagine async.hold, the Signal will never get updated by the time using runs

@mpilquist
Copy link
Member Author

Right, though it doesn't work today for many stream types. E.g., stick a take(1) after the brackets and the finalizers are run eagerly again (in all versions with compile.resource).

@SystemFw
Copy link
Collaborator

SystemFw commented Sep 4, 2019

Right, though it doesn't work today for many stream types. E.g., stick a take(1) after the brackets and the finalizers are run eagerly again (in all versions with compile.resource).

I see -.-"

Let's attack it from another angle maybe then. My use case for this has always been Stream.emit(dataStruct).concurrently(update dataStruct).compile.resource.lastOrError.
Can you think of other use cases? Also pinging @Daenyth @ChristopherDavenport

Maybe we can just make a compile.concurrently thing and limit both the cases that don't work and the special casing of scope.
I'm also open to other ideas about scoping semantics ofc, but this approach seems good except for the compile.resource aspect, right?

@SystemFw
Copy link
Collaborator

SystemFw commented Sep 4, 2019

On the other hand, our original goals were the behaviour of 1.0.2 + simplified interpreter (no Release) and we achieve that with this PR only

@Daenyth
Copy link
Contributor

Daenyth commented Sep 4, 2019

The main use case I care about right now is Stream.bracket(r)(..) ++ s2 where s2's correctness depends on the closure of r being guaranteed before start - examples might be using a semaphore/mvar/ref operation, or database transaction, or other remote system interaction.

The use case of (s1 concurrently s2).compile.resource is interesting but for that use case I've always "just" stayed in Stream. Most of my IOApp switch to Stream at a very high level anyway, so I don't have good insight into that pattern

@djspiewak
Copy link
Member

FWIW, I use emit(a).concurrently(s).compile.resource.lastOrError a lot

@SystemFw
Copy link
Collaborator

SystemFw commented Sep 4, 2019

FWIW, I use emit(a).concurrently(s).compile.resource.lastOrError a lot

Yeah, me too

s interesting but for that use case I've always "just" stayed in Stream.

In library code though, exposing singleton Stream is confusing.
Compare server: Stream[F, Stream[F, Socket[F]] to server: Stream[F, Resource[F, Socket[F]]

@mpilquist
Copy link
Member Author

  • Works: emit(a).concurrently(s).compile.resource.lastOrError // Last scope is from concurrently
  • Works: emit(a).concurrently(Stream.never.onFinalize(f)).compile.resource.lastOrError// Last scope is from concurrently, and the background stream never completes naturally
  • Works depending on perspective: emit(a).concurrently(Stream.empty.onFinalize(f)).compile.resource.lastOrError// Last scope is from concurrently, the finalizer f is run immediately though as it's created in its own scope, not registered in the root scope
  • Doesn't work: emit(a).concurrently(s).onFinalize(f).compile.resource.lastOrError // Last scope is from the final onFinalize, so s is terminated soon after a is emitted

@djspiewak
Copy link
Member

I wonder if that last one can be fixed in onFinalize? I think the only reason it is surprising is because there's an implicit expectation that onFinalize just tacks a finalizer onto the existing scope, rather than creating a new one.

@SystemFw
Copy link
Collaborator

SystemFw commented Sep 4, 2019

@mpilquist your latest description conforms to my expectations.
On one hand it seems "strange" that adding onComplete will break it (when looking from a distance), but on the other hand

  • A variant of this problem has always existed, as you point out
  • The initial bug is fixed
  • We have a fairly simple criterion to describe scope insertion (on each bracket)

So I would classify this PR as successful (thank you!). Apologies if I'm sounding extra picky, just a few points that maybe are worth exploring:

  • Do we have to treat onFinalize and onComplete as bracket? To me it feels like they should just attach things to the current scope, so basically onFinalize(a).onFinalize(b) == onFinalize(a >> b) roughly (sketched, the code might be imprecise).
  • The zipWith thing in Broadcast is a bit concerning, mostly because the old code is how someone would instinctively write that, and the new one isn't. That being said, maybe the problem is in zip, after all StepLeg was also an ad hoc addition
  • I'm quite happy about decoupling pull-to-stream from scope insertion, but I remember we did it once and had to revert , and one of your comments was "ofc pull-to-stream needs to insert scope, because... " (my memory fails me in the most important bit), so just wanted to triple check that we are not in the same situation anymore with the new semantics

@djspiewak
Copy link
Member

Do we have to treat onFinalize and onComplete as bracket? To me it feels like they should just attach things to the current scope, so basically onFinalize(a).onFinalize(b) == onFinalize(a >> b) roughly (sketched, the code might be imprecise).

Slight correction: onComplete isn't a bracket, it's just a fancy ++. onFinalize is, and I think that's what's slightly weird. I agree with your proposed law that onFinalize(a).onFinalize(b) <-> onFinalize(a *> b), if at all possible.

@mpilquist
Copy link
Member Author

I've considered changing onFinalize to not introduce a scope, though then we're back to needing the scope combinator and manually inserting it before ++. There are a few cases in both Stream and io package that have expressions like s.onFinalize(f) ++ t where t assumes f has run.

@SystemFw
Copy link
Collaborator

SystemFw commented Sep 4, 2019

@mpilquist ah, you're right

How about we do the opposite: onFinalize introduces a scope (common case solved), but there's a onFinalizeCurrentScope (or w/e) for this case? Too ad-hoc?

I'd also like some clarification on what hard scopes are for, is it just interruption?

@mpilquist
Copy link
Member Author

s.eventuallyFinalize(f) :)

I'll play with it some and see if there's something I can come up with. Thoughts on merging this as-is and following up with new PR for what comes out of that experiment? I'd like to get a milestone build out so http4s & other downstream projects can kick the tires a bit on the new scopes.

@mpilquist
Copy link
Member Author

Sorry, forgot to reply to your other 2 comments:

The zipWith thing in Broadcast is a bit concerning, mostly because the old code is how someone would instinctively write that, and the new one isn't. That being said, maybe the problem is in zip, after all StepLeg was also an ad hoc addition

The strange part is zipWith works fine with scopes normally -- all the zip tests pass for instance. The weird part about Broadcast/Balance is that the elements of the stream are inner streams -- it's not as simple as zipping 2 resourceful streams. We've made no claims that such a thing should work.

I'm quite happy about decoupling pull-to-stream from scope insertion, but I remember we did it once and had to revert , and one of your comments was "ofc pull-to-stream needs to insert scope, because... " (my memory fails me in the most important bit), so just wanted to triple check that we are not in the same situation anymore with the new semantics

Not that I know of. I vaguely remember what you're referring to here but I think last time we tried this, we ran in to issues well before getting the full test suite passing.

@Daenyth
Copy link
Contributor

Daenyth commented Sep 4, 2019

We've made no claims that such a thing should work.

That's a pretty reasonable stance to take, but absent a spec for which way it should work, downstream libraries and applications will assume a kind of "reference implementation" semantic of what it does, and may not be unit testing that they get the specific behavior they want, instead assuming that the library semantics in the published version are the semantics and shouldn't be tested (in terms of "test your own code, assume your libraries work")

@SystemFw
Copy link
Collaborator

SystemFw commented Sep 4, 2019

I'll play with it some and see if there's something I can come up with. Thoughts on merging this as-is and following up with new PR for what comes out of that experiment?

Given that things now are more broken, I'm onboard with that (assuming Daniel and Pavel feel the same)

The strange part is zipWith works fine with scopes normally -- all the zip tests pass for instance. The weird part about Broadcast/Balance is that the elements of the stream are inner streams -- it's not as simple as zipping 2 resourceful streams. We've made no claims that such a thing should work.

That's fair enough, yet on the other hand it did work in that code, but I don't think it's a blocker right now

Not that I know of. I vaguely remember what you're referring to here but I think last time we tried this, we ran in to issues well before getting the full test suite passing.

I remember it differently, that we actually released that behaviour for a while and we found through an issue, I'd have to double check because I'm not sure

@mpilquist mpilquist merged commit f03f82b into typelevel:series/1.1 Sep 5, 2019
@mpilquist mpilquist modified the milestones: 1.1.0-M2, 1.1.0 Sep 6, 2019
@marcodippy
Copy link

marcodippy commented Oct 11, 2019

I'm not sure if this is a known corner case or not, but considering that bracket(r)(..) ++ s now guarantees that r is released when s is evaluated, I find this behaviour a bit surprising:

val r1 = Resource.make(IO(println("acquire 1")))(_ => IO(println("release 1")))
val r2 = Resource.make(IO(println("acquire 2")))(_ => IO(println("release 2")))
Stream.resource(r1)
  .evalMap(_ => IO.unit)
  .handleErrorWith(_ => Stream.emit(())) ++ Stream.resource(r2)

/* prints
acquire 1
release 1
acquire 2
release 2
*/
Stream.resource(r1)
  .evalMap(_ => IO.raiseError(new RuntimeException))
  .handleErrorWith(_ => Stream.emit(())) ++ Stream.resource(r2)

/* prints
acquire 1
acquire 2
release 2
release 1
*/

The obvious way to restore the expected behaviour (the former) is to add .scope after handleErrorWith, but iiuc it shouldn't be needed anymore

@djspiewak
Copy link
Member

That really does seem odd. It's like the scope isn't getting closed correctly in the event that an error has propagated at any point?

@marcodippy
Copy link

@djspiewak I think this change is the cause of that behaviour (I tried my example with the original version of handleErrorWith and it works as expected)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants