Allow providing alternative implementations for generating FiberIds
#8778
Conversation
| * used in cases that rely on strict ordering of fibers (e.g., in zio-test) | ||
| */ | ||
| object Live extends Gen { | ||
| def make(location: Trace)(implicit unsafe: Unsafe): FiberId.Runtime = { |
There was a problem hiding this comment.
@kyri-petrou , isn't it possible to use something like the current thread's id instead of relying on random?
the way I see it we want (mostly) time ordered fiber Ids with a tie breaker, I suspect the spawning thread ID is a good enough tiebreaker and if not it we can add a thread local counter
There was a problem hiding this comment.
We can't use the thread id here because if the same thread is spawning multiple fibers (e.g., when using foreachPar), then the FiberId.Runtime will be the same for all the child fibers within the same millisecond, which is bad as IDs will no longer be unique.
As for using a thread local counter, unfortunately that wouldn't work with Loom very well since we get a new Thread per task. There's also no guarantee that 2 separate fibers would need to create a child fiber at the same time and end up having the same value in the counter. Which means we'd have to include both the parent thread ID and a thread local counter in FiberId.Runtime, which breaks both binary and source compatibility :(
There was a problem hiding this comment.
Also, fiber ordering is only needed for zio-test, so for normal execution we don't need ordering at all
| @@ -122,6 +122,8 @@ trait Runtime[+R] { self => | |||
|
|
|||
| protected abstract class UnsafeAPIV1 extends UnsafeAPI with UnsafeAPI3 { | |||
|
|
|||
| private val fiberIdGen = self.fiberRefs.getOrDefault(FiberRef.currentFiberIdGenerator) | |||
There was a problem hiding this comment.
shouldn't this be a def? the fiberRef can be overridden during fiber's execution
There was a problem hiding this comment.
We're within the Runtime trait here, and self.fiberRefs is immutable. The only way to "override" it here is by creating a new Runtime, and this val will be recreated in that case
|
I run the test suite 4 times and didn't get any of the TestClock flakiness that we've been experiencing so far, so I'm marking this PR ready for review |
Since #8745 got merged in, the TestClock tests have become flaky. It's a bit unclear what the exact reason is, but my current assumption is that it's due to fiber ordering (since TestClock relies on the ordering of fibers).
Leaving this as draft for the moment to run CI a few times to see if this resolves it