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

Add interop.flow.pipeToProcessor & interop.flow.processorToPipe #3449

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

BalmungSan
Copy link
Contributor

@BalmungSan BalmungSan commented Jun 14, 2024

Convenient helpers.

Roadmap:

  • safe version.
  • Scaladoc.
  • unsafe version.
  • Unit test.

): Resource[F, StreamProcessor[F, I, O]] =
for {
streamSubscriber <- Resource.eval(StreamSubscriber[F, I](chunkSize))
inputStream = streamSubscriber.stream(subscribe = F.unit)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't have access to the upstream Publisher yet, so we can't do the subscribe here. So we do nothing and wait for it.

Copy link
Member

Choose a reason for hiding this comment

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

So we do nothing and wait for it.

I'm not entirely sure I understand the waiting part. Can/should we use a Deferred here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No sorry, my point is that subscribe should be something like IO(publisher.subscribe(streamSubscriber)) as you can see in the implementation of syntax.fromPublisher.
Here, however, we don't have a Publisher yet, so we don't control when we are published.

Basically, this is very similar to the fromPublisher overload that accepts a Subscriber => F[Unit], just that rather than receiving the lambda, we are just returning a raw Processor / Subscriber.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, I understand now.

I think what confused me was that this is the first time we are effectively exposing StreamSubscriber. Our existing {to,from}Publisher APIs have kept it hidden.

So I do feel weird about exposing it now. But I guess nothing here is unreasonable 🤔

@BalmungSan BalmungSan marked this pull request as ready for review June 14, 2024 22:41
@BalmungSan BalmungSan changed the title WIP: Add interop.flow.pipeToProcessor Add interop.flow.pipeToProcessor Jun 14, 2024
core/shared/src/main/scala/fs2/interop/flow/package.scala Outdated Show resolved Hide resolved
): Resource[F, StreamProcessor[F, I, O]] =
for {
streamSubscriber <- Resource.eval(StreamSubscriber[F, I](chunkSize))
inputStream = streamSubscriber.stream(subscribe = F.unit)
Copy link
Member

Choose a reason for hiding this comment

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

So we do nothing and wait for it.

I'm not entirely sure I understand the waiting part. Can/should we use a Deferred here?

Comment on lines 30 to 38
final class ProcessorPipeSpec extends Fs2Suite {
test("should process upstream input and propagate results to downstream") {
forAllF(Arbitrary.arbitrary[Seq[Int]], Gen.posNum[Int]) { (ints, bufferSize) =>
// Since creating a Flow.Processor is very complex,
// we will reuse our Pipe => Processor logic.
val processor = unsafePipeToProcessor[Int, Int](
pipe = stream => stream.map(_ * 1),
chunkSize = bufferSize
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if there is a better way to test this.
Also, not sure if there is something else we may test.

@BalmungSan BalmungSan changed the title Add interop.flow.pipeToProcessor Add interop.flow.pipeToProcessor & interop.flow.processorToPipe Nov 24, 2024
@armanbilge armanbilge added this to the v3.12.0 milestone Dec 28, 2024
@armanbilge armanbilge closed this Dec 28, 2024
@armanbilge armanbilge reopened this Dec 28, 2024
@armanbilge
Copy link
Member

CI passed this time, but one of the tests seems flaky. Possible race condition?

==> X fs2.interop.flow.StreamProcessorSpec.should process upstream input and propagate results to downstream  30.006s java.util.concurrent.TimeoutException: test timed out after 30 seconds

https://github.com/BalmungSan/fs2/actions/runs/12014747489/job/33491269929#step:11:1615

@BalmungSan
Copy link
Contributor Author

@armanbilge I have been re-running the test for a while now, and it always succeeds in a couple of seconds.
Do you have any idea how I could try to force the race condition? Or maybe it was just something weird? It may as well be the case that the merge with main fixed it?
I also have checked the code and everything looks correct as far as I can tell.

@armanbilge
Copy link
Member

armanbilge commented Jan 26, 2025

It may as well be the case that the merge with main fixed it?

I see the exact same CI failure in your latest merge commit 😕 it must be something legit. I will try to reproduce ...

https://github.com/typelevel/fs2/actions/runs/12967866568/job/36170223920?pr=3449#step:11:1615

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.

2 participants