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

Broadcast #2312

Merged
merged 34 commits into from
Mar 2, 2021
Merged

Broadcast #2312

merged 34 commits into from
Mar 2, 2021

Conversation

SystemFw
Copy link
Collaborator

@SystemFw SystemFw commented Mar 2, 2021

This PR revisits the broadcasting abstractions in fs2:

  • .broadcast is removed. In its raw form, this method is almost impossible to use safely, and depends on subtle scoping details (e.g. chunkN works, foldMap(List(_))) doesn't, and at the same time it's not flexible enough to encode fully dynamic subscribers like Topic is.
  • (not in this PR, just for clarity) The improved Topic is recommended for cases where you might have used .broadcast. If you managed to get that to work, that is.
  • the main method for static broadcasting is broadcastThrough, which is safe and easy to use. It has been reimplemented using Topic.
  • The various broadcast* overloads have even removed
  • Broadcast has been removed.
  • Fixes a bug with topic which causes all subscribers to freeze when one was removed

SystemFw added 30 commits March 2, 2021 00:17
Note: Latch gets completed correctly
At the moment there is deadlock if one of the pipes interrupts,
probably the bug is in Topic or even Queue
@SystemFw SystemFw requested a review from mpilquist March 2, 2021 10:21
Comment on lines 235 to 243
Stream
.resource(topic.subscribeAwait(1))
.flatMap { sub =>
// crucial that awaiting on the latch is not passed to
// the pipe, so that the pipe cannot interrupt it and alter
// the latch count
Stream.exec(latch.release >> latch.await) ++
sub.unNoneTerminate.flatMap(Stream.chunk).through(pipe)
}
Copy link
Contributor

@diesalbla diesalbla Mar 2, 2021

Choose a reason for hiding this comment

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

Could we extract this lambda block (the one used int the map { pipe => ) as a function def pump(pipe: Pipe[F2, O, O2]) ?

Copy link
Collaborator Author

@SystemFw SystemFw Mar 2, 2021

Choose a reason for hiding this comment

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

I'm factored out the unNoneTerminate... stuff, but I want to keep the waiting logic all together, so it's easy to understand how the parJoin, the latch and the Resource cooperate

Comment on lines 247 to 248
Stream.eval(latch.await) ++
chunks.noneTerminate.through(topic.publish)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we extract this as a value backgroundPublish?

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 factored out the chunks.noneTerminate stuff, I want to keep the await logic where it is

Comment on lines 135 to 138
q.tryTake.flatMap {
case None => F.unit
case Some(_) => drainQueue
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps another traverse?

Suggested change
q.tryTake.flatMap {
case None => F.unit
case Some(_) => drainQueue
}
q.tryTake.flatMap(_.traverse_(_ => drainQueue))

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Used iterateUntil, which avoid redoing the get as well

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