Fix: some specs rely on Fiber.yield behavior #6953
Conversation
Some specs are relying on the `Fiber.yield` behavior is actually the event loop behavior (libevent2). They start failing whenever the `Fiber.yield` algorithm changes —for example to push the current fiber to the runnables queue, instead of adding a resume event. This patch proposes changes to make the fiber synchronization expectations explicit. Either by looping until somethig is ready, or using Channel::Unbuffered's sync ability, or with an explicit enqueue/resume for specs testing Channel itself.
ysbaddaden
force-pushed
the
fix-specs-rely-on-fiber-yield-behavior
branch
from
f5ccf66 to
d623176
Compare
| main = Fiber.current | ||
|
|
||
| sender = Fiber.new do | ||
| ch.send(nil) |
There was a problem hiding this comment.
Related to #6948 : I'd expect ch.send to raise here since it couldn't deliver the value.
There was a problem hiding this comment.
Commented in the issue. I think raising only in the second send is the right thing.
I would leave the rescue block to catch upon the second send.
|
The behaviour provided by the spec helpers Maybe |
| four = true | ||
| two = false | ||
| three = false | ||
| four = false |
There was a problem hiding this comment.
Because we eventually assert that they'll be true, so I wanted to also make sure the variables were changed —meaning that the fibers have run. We could even initialize them to nil.
There was a problem hiding this comment.
I guess because the assertions at the end are should be_true so the test might have passed before if none of the spawns were executed.
|
Fiber synchronization should be achieved through synchronous channels, as we do for the
|
| main = Fiber.current | ||
|
|
||
| sender = Fiber.new do | ||
| ch.send(nil) |
There was a problem hiding this comment.
Commented in the issue. I think raising only in the second send is the right thing.
I would leave the rescue block to catch upon the second send.
| @@ -34,13 +34,12 @@ describe Concurrent::Future do | |||
|
|
|||
| describe "future" do | |||
| it "computes a value" do | |||
| chan = Channel(Int32).new(1) | |||
| chan = Channel::Unbuffered(Int32).new | |||
There was a problem hiding this comment.
This is changing the spec to run with buffered channel to an unbuffered.
Is there any reason for that change other than buffered channel of size 1 seems to be an unbuffered channel?
There was a problem hiding this comment.
An unbuffered channel is synchronous, so it forces main/fiber to sync as we expect it. Unlike Buffered that are async.
Unrelated to this PR, but I think both buffered/unbuffered should have both (a)synchronous modes —in fact, I would have just buffered channels with a blocking argument to select sync/async.
There was a problem hiding this comment.
Ok, it's true the behavior is different. And that change does not affect the spec.
I think is more clear to have the async/sync nature in the type and not in the type argument.
| four = true | ||
| two = false | ||
| three = false | ||
| four = false |
There was a problem hiding this comment.
I guess because the assertions at the end are should be_true so the test might have passed before if none of the spawns were executed.
Some specs are relying on the
Fiber.yieldbehavior, that's actually the event loop behavior (libevent2). They start failing whenever theFiber.yieldalgorithm changes —for example to push the current fiber to the runnables queue, instead of adding a resume event.This patch proposes changes to make the fiber synchronization expectations explicit. Either by looping until somethig is ready, or using Channel::Unbuffered's sync ability, or with an explicit enqueue/resume for specs testing Channel itself.