Fix hold1

[armanbilge]

Fixes #3081.

[Jasper-M]

Still a shame that uncons and concurrently don't seem to be composable.

[mpilquist]

Still a shame that uncons and concurrently don't seem to be composable.

I wouldn't necessarily conclude that. There's some bug hiding here that broke hold1 but I'd be surprised if that bug was something fundamental about uncons and concurrently interactions.

[armanbilge]

I'd be surprised if that bug was something fundamental about uncons and concurrently interactions.

Yes, it needed the zip as well.

I don't really understand how all this scoping works, but it seemed like what was happening was some scope was being opened in the uncons, and because the tail was being run elsewhere concurrently, it was unable to locate this already open scope. If my understanding is right, then we are lacking some mechanism to thread that opened scope through.

[armanbilge]

Wait, maybe there is a way :) let me try something

fs2/core/shared/src/main/scala/fs2/Pull.scala

Lines 460 to 467 in 597102b

	/** Extends the scope of the currently open resources to the specified stream,
	* preventing them from being finalized until after `s` completes execution,
	* even if the returned pull is converted to a stream, compiled, and
	* evaluated before `s` is compiled and evaluated.
	*/
	def extendScopeTo[F[_], O](
	s: Stream[F, O]
	)(implicit F: MonadError[F, Throwable]): Pull[F, Nothing, Stream[F, O]] =

[armanbilge]

No, that didn't work either. be8d3e7

[armanbilge]

Yeah, so extendScopeTo extended the lifetime of the scope, but it still didn't use that scope to compile the concurrent stream.

[armanbilge]

I experimented with the idea that the scope of the background stream in concurrently should be part of the scope tree of the foreground stream, instead of creating its own root scope. I believe that would solve this issue, without requiring to change the implementation of hold1.

I tried two different approaches, either interfacing directly with Pull.compile or adding a new API stream.compileScope(scope).

• 779a170 fixes hold1, but breaks concurrently
• 576acc4 didn't fix hold1, but didn't break concurrently

Once I've mustered up new courage I'll probably take another shot at it.

[Jasper-M]

This will result in Stream.never if the signal never gets completed, e.g. if this is empty.

[armanbilge]

I think that's the behavior we want, no?

[Jasper-M]

I'm not sure. An empty stream is also an option. That's what the original implementation did afaict.

[armanbilge]

Ah I see, we are not concerned with the case Stream.never.hold1 (which never emits but never ends) but with the case Stream.empty.hold1. So there should be an onFinalize or something.

[armanbilge]

Thanks, the fix was even easier than I realized, no need for onFinalize :)