Fs2: Stream.bracket releases promptly only when onComplete is used

[matil019]

Nice to meet you!

An effect registered by Stream.bracket is run as soon as possible only if Stream#onComplete is applied to the Stream.

Please see the example:

Stream(1, 2)
  .flatMap{ x =>
    val acquire = IO { println(s"acquire: $x"); x }
    val release = IO { println(s"release: $x") }
    Stream.bracket(acquire)(_ => release)
      .onComplete(Stream.empty)  // <- should be no-op
  }
  .evalMap(x => IO { println(s"consume: ${x}") })
  .compile.drain.unsafeRunSync()

The following is the output, which indicates the release effects are run promptly:

acquire: 1
consume: 1
release: 1
acquire: 2
consume: 2
release: 2

However, if we remove the apparently pointless .onComplete(Stream.empty) from the code, it prints the following output instead:

acquire: 1
consume: 1
acquire: 2
consume: 2
release: 2
release: 1

that is, all of the "resources" are kept open and are closed at the end in the reverse order.

Actually I prefer the former behavior (prompt release) as it is more resource-friendly and has the same behavior as Haskell's conduit package.

Is it possible to have the former as the intended, the default and the permanent behavior?

[djspiewak]

This is immensely interesting. The former behavior does strike me as more correct.

[SystemFw]

https://github.com/functional-streams-for-scala/fs2/pull/1417
https://github.com/functional-streams-for-scala/fs2/issues/1505
https://github.com/functional-streams-for-scala/fs2/issues/1491

[matil019]

@SystemFw Thank you for the info, I can reproduce the same behavior by replacing .onComplete(...) with .scope. I'm relieved to know that there is already a dedicated method for the particular use case instead of falling back to some hack.

That said, is there any reason why it's not the default? I couldn't find any hints by reading the refs. Is there any pitfall/known breakage with bracket + scope ?

[SystemFw]

You might want to take a closer look at the first link :)

Summary: you can think of a Scope as a lifetime region, when a resource is opened it is registered in a given scope, and when the scope terminates all the resources held in that scope are closed.
Scopes are inserted in several places, for example every time a Pull is converted to a Stream (which happens in almost every method) a scope is inserted. So the behaviour that you want as the default is already what happens in the vast majority of cases, and, in case you have a Stream where resources are installed in a scope that you consider too wide, you can insert a scope manually with scope.

Before the first PR I linked, things worked as you expected in _all_ cases, not just most cases, without you needing to insert scope to cover the rest. However, that resulted in a lot more complication in the internals (detailed in the first PR I linked). So the question is whether is worth tolerating these corner cases in exchange for a big simplification of the internals.

Anyway, I think it's time to hear @mpilquist opinion as well (although he might be on holiday for a few days). Daniel let me know what you think as well ofc :)

[djspiewak]

I'm deeply uncomfortable with the currently magical nature of this, but at present I don't have a good answer of how to improve it from a big picture standpoint. I read through the three issues you linked (thanks for that, btw!) and I understand what's going on, but it's deeply unintuitive from a surface standpoint. This is very topical because it gets to the heart of how dynamically scoped monadic regions should behave in practice, which is something I've been thinking about a lot lately. No good answers so far, and the literature is pretty barren, but I'm still thinking.

[SystemFw]

Ah, one thing I can tell you for sure: in retrospect, we made a mistake making this change in a point release, it deserved a lot more attention.

@djspiewak I'm keen to hear your further thoughts when they're ready. Note that in theory we could revert and get "perfect" behaviour, it that's deemed necessary

[djspiewak]

Note that in theory we could revert and get "perfect" behaviour, it that's deemed necessary

What would perfect behavior be, exactly? You mean the behavior where .scope is inserted? I agree that's more intuitive in this case, but taking a step back, abstractly, why is that better?

[pchlupacek]

Just to add my opinion here. I am also not that happy with current situation. I liked previous so much better that in fact this is one of the reasons why we are stick in production with pre-change release.
In large codebase of ours which is fs2 only, and where we have lot of hand-written combinators I am really scared where this magic behaviour will popup and how long it will take us to find out the problem.

If possible, I would like really to get back to previous behaviour. I am feeling uncomfortable that we did this change just to cleanse internals.

[SystemFw]

If possible, I would like really to get back to previous behaviour. I am feeling uncomfortable that we did this change just to cleanse internals.

well, the more issues and bugs we get, the more I kinda agree

I liked previous so much better that in fact this is one of the reasons why we are stick in production with pre-change release.

I consider this a really strong datapoint coming from you

I agree that's more intuitive in this case, but taking a step back, abstractly, why is that better?

This question is why I put quotes around "perfect", and I don't have a conceptual answer (at least not right now), I'm going to say that the it's better though because we had less users inquire about it (for most of them things would just work) and because the rules were more clear cut: a ++ b releases things in a before b, for example.

---

Another avenue to explore would be keeping the internals as they are (just Scope, no Release) and see whether we can insert scope in select places. Generally every time we discussed inserting a scope after ++ (for example) there were concerns about performance, but I don't think they were ever validated one way or another.

[pchlupacek]

@SystemFw inserting scope after ++ was always significant hit, unfortunately. I did number of testing earlier when developing interruption and it was really great deal.

The problem I really see here is that it is "surprising" behaviour. I wouldn't dare to call it incorrect, but it is surprising. Obviously we double killed ourselves doing that change as minor release :-(

Anyhow let's live with it collect data and hopefully we will learn with it and longer term I am sure we will find a solution for it.

[djspiewak]

Scope termination at ++ definitely seems correct to me. The way I've been thinking about it is, with dynamic resource scoping, any acquired resource can theoretically remain accessible through an entire bind chain, right up to the end. ++ isn't bind though, and the values after the ++ cannot be dependent on the values before it, thus any resources should be released as it is in fact quite safe to do so.

Nested binds are a different question, and we need to be careful here because it would be very easy to break monadic associativity by doing things incorrectly.

[pchlupacek]

@djspiewak indeed scope termination at ++ is correct. Problem is how to implement it. The cost of scope creation and release is high and ++ is used extensively /i.e. flatMap/.

It perhaps worth of exploring however if somehow this could be implemented better now.

[djspiewak]

I'm not sure if flatMap should create a scope. Why would it? Shouldn't scopes be introduced by bracket and eliminated by… well, other things? (including ++)

[pchlupacek]

There is definitely area to explore that. I know we have been playing a lot with it in this field, but things changed now, so perhaps it opened window of opportunity :-)

[djspiewak]

Is there a law anywhere which implies anything like the following?

Stream(fa, fb).flatten <-> fa ++ fb

Edit I guess this is something like distributativity in the AlternativeLaws. Seen slightly differently:

(Stream.emit(fa) ++ Stream.emit(fb)).flatten <-> fa ++ fb

// taken together with

Stream.emit(fa).flatten <-> fa

// ...which is the same as

fa.flatMap(a => Stream.emit(a)) <-> fa

[pchlupacek]

Not that I am aware of. I am correcting ++ statement to be used in flatMap. FreeC is now in bind, which is just more complex than one used in ++.

[djspiewak]

I've got it. It's not about flatten, it's about map. map extends a scope, flatten can still be connected to ++. Still need to sort through what the "right" thing is here but I think I at least have an intuition which is consistent with the monad laws.

[pchlupacek]

@djspiewak cool. Would be excellent if you take a sneak peak on this :-)

[djspiewak]

Working on it. :-) Like I mentioned above, I've been thinking a lot about the proper semantics for this type of monadic region. I think I'm getting close to something…

[djspiewak]

After pondering for a while and considering some other examples of this type of construct…

I'm pretty convinced that the only reasonable semantic here is to close scopes at ++ boundaries. In other words:

bracket(fa)(ra).flatMap(f1) ++ bracket(fa)(rb).flatMap(f2) <-> fa.flatMap(f1).flatMap(ra) ++ fb.flatMap(f2).flatMap(rb)

Give or take.

A strong motivation why this has to be the case comes from repeat:

Stream.bracket(open)(close).flatMap(r => doSomething(r)).repeat

Right now, we have two options for handling the above. Either each resource remains open until the end of the world and is effectively never closed (well, not until the stream shuts down), or they are closed but we lose the following equivalence:

def repeat(s: Stream[F, A]) = s ++ repeat(s)

I think we definitely want the above to hold, and we want the "bracket + repeat" formation (and similar) to work, which means we need ++ to close scopes.

Edit As a note tying this back to the OP… Stream(a, b) <-> Stream.emit(a) ++ Stream.emit(b), and so if we break scopes at ++ boundaries, we also must break scopes between the elements of the sequence in the OP.

[mpilquist]

I agree with closing scopes on ++.

I'd also love a minimized set of tests (written in the style of the current test suite) that capture these types of issues. We could then evaluate current behavior vs old behavior vs some new design. We could roll back #1417 but I don't think that will help much, as it won't cover all scenarios -- notably ++.

[SystemFw]

Agreed. I'd especially like to understand what is it about scope insertion
on ++ that is heavy, perhaps we can insert some sort of lightweight scope

On Fri, 19 Jul 2019, 15:47 Michael Pilquist, [e-mail]
wrote:

I agree with closing scopes on ++.

I'd also love a minimized set of tests (written in the style of the
current test suite) that capture these types of issues. We could then
evaluate current behavior vs old behavior vs some new design. We could roll
back #1417 https://github.com/functional-streams-for-scala/fs2/pull/1417
but I don't think that will help the much, as it won't cover all scenarios
-- notably ++.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/functional-streams-for-scala/fs2/issues/1535?email_source=notifications&email_token=AB3I33RJ5AD2CCBU3GWJY33QAGZVHA5CNFSM4ICEF7I2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2LQZFQ#issuecomment-513215638,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AB3I33XBLAXGCKVA3XO7VWLQAGZVHANCNFSM4ICEF7IQ
.

[matil019]

I'm not sure if ++ is the right place to put a scope, at least with the current implementation. .scope has no effect if it is put outside of a .flatMap. From my example,

Stream(1, 2)
  .flatMap{ x =>
    val acquire = IO { println(s"acquire: $x"); x }
    val release = IO { println(s"release: $x") }
    Stream.bracket(acquire)(_ => release)
  }
  .scope
  .evalMap(x => IO { println(s"consume: ${x}") })
  .compile.drain.unsafeRunSync()

no release actions are run until the end of the stream, as if the .scope weren't there.

I'm not sure whether it (nested Streams) is one of the intended use cases of scope, but it looks like a pitfall to me, as a relative beginner to fs2.

[mpilquist]

Consider this program:

val s = Stream.bracket(IO(println("acquired")))(_ => IO(println("released"))) ++ Stream.eval_(IO(println("post-append")))
s.evalMap(_ => IO(println("using"))).compile.drain.unsafeRunSync

With 1.0.2:

using
released
post-append

With 1.0.3, 1.0.4, 1.0.5, 1.1.0-M1:

using
post-append
released

1.0.2 was the last version that did the expected thing here and ran the finalizer before the RHS of ++ was evaluated. This is important b/c we've thought the problematic behavior was introduced as a result of #1417 but that PR was not present in the 1.0.3 release. I checked #1368 but with that PR, the 1.0.3-SNAPSHOT was still behaving correctly.

Git bisecting led me to this commit introducing the problematic behavior: 7634ef8b018cbbc58ae29528c391ddbe1fbf8165

Okay! This is somewhat of a red herring -- to make s.compile.resource useful, we disabled explicit resource finalization in the root scope, extending finalization to root scope closure. Let's come back to this in a moment.

Let's modify our test case to insert a scope, bypassing the special case of root scope resources:

s.scope.evalMap(_ => IO(println("using"))).compile.drain.unsafeRunSync

With 1.0.2, 1.0.3:

using
released
post-append

With 1.0.4, 1.0.5, 1.1.0-M1:

using
post-append
released

This behavior did in fact change as a result of 17b978736d75d4921d83d9c19b10145fd3a1bf81, as we have suspected (phew!).

I have a branch where I introduce a scope on each bracket. This seems to work very well but we need to decide about root scoped resources. When each bracket introduces a new scope, there are no such thing as root scoped resources. We could say that only the direct child scopes from the root scope have the special no-release logic to restore the current behavior. Alternatively, we could engage the special no-release logic only when using compile.resource and not when using the other compilers. Thoughts?

[pchlupacek]

I think scope on each bracket (which I think is usually the resource) may work very well. I don't think so you need and significant performance on resource acquisition and release, and if so, thats ok if in such scenarios we would allow some non-scoped constructors perhaps, as this would be very advanced code anyhow.

I am not sure if I follow root scope issue correctly. I would be ok to say that compile.resource behaves differently than other compilations that sounds to me as reasonable compromise for now.

[mpilquist]

Confirmed that the fix in #1574 fixes the original example here.

[djspiewak]

@mpilquist I am irrationally excited about this