wait for the fiber to update the ref in FiberSpec #652
Conversation
| @@ -11,7 +13,7 @@ class FiberSpec(implicit ee: org.specs2.concurrent.ExecutionEnv) extends TestRun | |||
| for { | |||
| ref <- Ref.make(false) | |||
| fiber <- IO.unit.bracket(_ => ref.set(true))(_ => IO.never).fork | |||
| _ <- fiber.toManaged.use(_ => IO.unit) | |||
| _ <- fiber.toManaged.use(_ => clock.sleep(20.millis)) | |||
There was a problem hiding this comment.
To make the test deterministic and run as fast as possible, let's instead just join the fiber after we use it:
for {
ref <- Ref.make(false)
fiber <- IO.unit.bracket(_ => ref.set(true))(_ => IO.never).fork
_ <- fiber.toManaged.use(_ => IO.unit)
_ <- fiber.awaitThere was a problem hiding this comment.
Hmm, actually, maybe I'm wrong here, shouldn't toManaged already do that?
There was a problem hiding this comment.
we can add clock.sleepbefore we call toManaged because the execution of IO.unit is faster and the fiber will be interrupted before updating the value of ref
There was a problem hiding this comment.
Ah, toManaged interrupts the fiber after use. So we need a latch to make test deterministic and fast.
Here's what we need, I think:
for {
ref <- Ref.make(false)
latch <- Promise.make[Nothing, Unit]
fiber <- IO.unit.bracket(_ => ref.set(true) *> latch.succeed(()))(_ => IO.never).fork
_ <- fiber.toManaged.use(_ => latch.await)There was a problem hiding this comment.
@wi101 The sleep won't be reliable and will delay test. So let's try to use a latch here if we can. See above code snippet.
There was a problem hiding this comment.
@jdegoes 👍 will do that
|
@wi101 THANK YOU. These flaky tests have been driving me crazy. 👹 |
| _ <- fiber.toManaged.use(_ => IO.unit) | ||
| latch <- Promise.make[Nothing, Unit] | ||
| fiber <- IO.unit.bracket(_ => ref.set(true) *> latch.succeed(()))(_ => IO.never).fork | ||
| _ <- fiber.toManaged.use(_ => latch.await) |
There was a problem hiding this comment.
One more thing if you want (only if you want!): to make this test better, after the value <- ref.get, we could await the fiber. if it has been successfully interrupted, then it will return right away; but if it has not been interrupted, then the await will time out because the fiber will still be in IO.never.
There was a problem hiding this comment.
I added it and the test now returns timeout :(
There was a problem hiding this comment.
Anyway, this is a huge improvement and will make the test pass reliably, I think. So 👍
There was a problem hiding this comment.
for {
ref <- Ref.make(false)
latch <- Promise.make[Nothing, Unit]
fiber <- IO.unit.bracket(_ => ref.set(true) *> latch.succeed(()))(_ => IO.never).fork
_ <- fiber.toManaged.use(_ => latch.await)
_ <- fiber.await
value <- ref.get
} yield value must beTrueThere was a problem hiding this comment.
It sounds like the finalizer is not interruptible and we have interrupt there..
There was a problem hiding this comment.
I was confused (it's wrong what I said -_- ^^
There was a problem hiding this comment.
How about this one:
for {
ref <- Ref.make(false)
latch <- Promise.make[Nothing, Unit]
fiber <- IO.unit.bracket(_ => ref.set(true))(_ => latch.succeed(()) *> IO.never).fork
_ <- fiber.toManaged.use(_ => latch.await)
_ <- fiber.await
value <- ref.get
} yield value must beTrueThere was a problem hiding this comment.
I see now (I think 😆 ). The race is when the fiber starts execution!
So this should work:
for {
ref <- Ref.make(false)
latch <- Promise.make[Nothing, Unit]
fiber <- (latch.succeed(()) *> IO.unit).bracket_(ref.set(true))(IO.never).fork
_ <- latch.await
_ <- fiber.toManaged.use(_ => IO.unit)
_ <- fiber.await
value <- ref.get
} yield value must beTrueThere was a problem hiding this comment.
Yes!! It works now, I will push that
Thank you! :)
* fixed test case
Sometimes the ref doesn't change its value yet and the fiber is interrupted when we use UIO.unit, it will be faster