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

Make listen return Resource[F, Stream] instead of Stream to handle readiness #765

Closed
mbaechler opened this issue Dec 19, 2022 · 4 comments
Closed

Comments

@mbaechler
Copy link
Contributor

In ChannelTest, we have:

for {
      // There is a race here. If this fiber doesn't start running quickly enough all the data will
      // be written to the channel before we execute LISTEN. We can't add a latch to `listen` that
      // indicates LISTEN has completed because it makes it impossible to implement `mapK` for
      // `Channel` and thus for `Session`. So for now we're just going to sleep a while. I'm not
      // sure it's a problem in real life but it makes this test hard to write.
      f <- ch.listen(42).map(_.value).takeThrough(_ != data.last).compile.toList.start
      _ <- IO.sleep(1.second) // sigh
      _ <- data.traverse_(ch.notify)
      o <- f.join
      Succeeded(fa) = o
      d <- fa
      _ <- assert(s"channel data $d $data", data.endsWith(d)) // we may miss the first few
    } yield "ok"

If we change listen to return a Resource, we could write something like that:

      ch.listenR(42).use { r =>
        for {
          f <- r.map(_.value).takeThrough(_ != data.last).compile.toList.start
          _ <- data.traverse_(ch.notify)
          o <- f.join
          Succeeded(fa) = o
          d <- fa
          _ <- assert(s"channel data $d $data", data == d)
        } yield "ok"
      }

Because once we acquire the Resource, we know that we started listening, so we can't miss any event.

@mpilquist
Copy link
Member

Sounds good to me, want to create a PR?

mbaechler added a commit to mbaechler/skunk that referenced this issue Dec 19, 2022
@mbaechler
Copy link
Contributor Author

mbaechler commented Dec 19, 2022

I tried but I can't make it working. Would you mind looking at it?

mbaechler added a commit to mbaechler/skunk that referenced this issue Dec 19, 2022
mbaechler added a commit to mbaechler/skunk that referenced this issue Dec 19, 2022
mbaechler added a commit to mbaechler/skunk that referenced this issue Dec 19, 2022
@mbaechler
Copy link
Contributor Author

I managed to make it work thanks to me peers @trobert and @rlecomte

mbaechler added a commit to conduktor/skunk that referenced this issue Dec 20, 2022
mbaechler added a commit to mbaechler/skunk that referenced this issue Dec 20, 2022
mbaechler added a commit to conduktor/skunk that referenced this issue Dec 20, 2022
mbaechler added a commit to conduktor/skunk that referenced this issue Dec 20, 2022
mbaechler added a commit to conduktor/skunk that referenced this issue Dec 20, 2022
mbaechler added a commit to conduktor/skunk that referenced this issue Dec 20, 2022
mbaechler added a commit to conduktor/skunk that referenced this issue Dec 20, 2022
mbaechler added a commit to conduktor/skunk that referenced this issue Dec 20, 2022
mbaechler added a commit to mbaechler/skunk that referenced this issue Jan 4, 2023
mbaechler added a commit to mbaechler/skunk that referenced this issue Jan 4, 2023
mpilquist added a commit that referenced this issue Jan 17, 2023
@mbaechler
Copy link
Contributor Author

It has been merged

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

No branches or pull requests

2 participants