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

Fix scope leak with Stream.retry and in general, with pulls that do... #1885

Merged
merged 1 commit into from
Jun 7, 2020

Conversation

mpilquist
Copy link
Member

@mpilquist mpilquist commented Jun 5, 2020

…not consume all elements from source.

Some observations prior to this PR:

// No leak:
Stream.eval(IO.unit).attempt.repeat.compile.drain.unsafeRunSync()

// Leaks in 2.1.0+, does not leak in 2.0.1:
Stream.eval(IO.unit).attempts(Stream.constant(1.second)).head.repeat.compile.drain.unsafeRunSync()

// Leak fixed by inserting an explicit scope after head
Stream.eval(IO.unit).attempts(Stream.constant(1.second)).head.scope.repeat.compile.drain.unsafeRunSync()

The reason a leak was introduced in 2.1.0 is that handleErrorWith was modified to introduce a scope. The use of head, which is an alias for take(1), is implemented with a pull that discards the remainder of the stream once a single output element has been output. This has the effect of discarding the release of the scope opened by handleErrorWith.

More generally, any pull which discards the remainder of the stream has the potential of causing a similar scope leak. Hence, this PR returns to the FS2 1.0.x design of introducing a scope every time a pull is converted to a stream. Doing so causes the scope introduced by handleErrorWith to be a child scope of the pull-to-stream scope, and hence, when the pull-to-stream scope is closed, all descendant scopes are closed too.

@@ -1793,7 +1793,7 @@ final class Stream[+F[_], +O] private[fs2] (private val free: FreeC[F, O, Unit])
* }}}
*/
def map[O2](f: O => O2): Stream[F, O2] =
this.pull.echo.mapOutput(f).stream
this.pull.echo.mapOutput(f).streamNoScope
Copy link
Member Author

Choose a reason for hiding this comment

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

Without this change, the test for resourceWeak fails b/c there's a map in the implementation of resourceWeak. It's okay to not have a scope here b/c this pull consumes the entire source stream. Note back in 1.0.5, we had this same definition of map and it was the only such invocation in the library. So while this does feel a bit ad-hoc, we also have the most mileage on it.

Copy link
Collaborator

@SystemFw SystemFw left a comment

Choose a reason for hiding this comment

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

Very clear explanation, I hadn't fully read it yesterday :)

@mpilquist mpilquist merged commit 12a06a1 into typelevel:master Jun 7, 2020
@mpilquist mpilquist added this to the 2.4.0 milestone Jun 8, 2020
@mpilquist mpilquist deleted the bugfix/1884 branch February 18, 2024 13:33
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.

3 participants