Stop searching for work when stolen timers filled the local queue

[ichraf7]

Add a check that prevent searcher thread from stealling from other thread or from external queue when it queue is no longer empty. The current behavior cause a forver spin of searcher thread when expiring timer fill all the local queue

issue discussed here #issues/4674

[reardonj]

@armanbilge , @djspiewak , looks like we have the answer to this question!

We should probably steal both timers and fibers; why only steal one?
-- #4247 (comment)

[reardonj]

This makes sense to me.

[reardonj]

Do stolen timer callbacks always resume fibers? Wondering if we could just check stoleTimers instead of queue.nonEmpty()

[ichraf7]

I don't know if we can have a race condition if we have a cancel that happens in firing moment making it don't resume fiber ( just hypothesis)
I think stoleTimers is a sufficient guard and it is the shape the commit originally had,
both are correct just matter of clarity and taste , I can change it if you want

[reardonj]

The nonEmpty call is likely going to be a bit more expensive since it ends up reading and comparing several AtomicIntegers, instead of the boolean we just computed, so it would be nice to avoid it if possible.

[ichraf7]

I updated

[djspiewak]

This is a really good catch, but as written I think it runs the risk of creating permanently asymmetric workloads by always prioritizing timer theft, particularly if an application has a lot of timer expiries that schedule new timers. It may be better to alternate this prioritization, either swapping back and forth (steal fibers first, then timers; steal timers first, then fibers) or by efficiently randomly choosing which one to do first.

[ichraf7]

This is a really good catch, but as written I think it runs the risk of creating permanently asymmetric workloads by always prioritizing timer theft, particularly if an application has a lot of timer expiries that schedule new timers. It may be better to alternate this prioritization, either swapping back and forth (steal fibers first, then timers; steal timers first, then fibers) or by efficiently randomly choosing which one to do first.

Valid point, for having alternating the prio per worker it add state to the worker and more branches, also for app with periodic load having a lot of timers in case of alternation it makes the worker search for work first where there is nothing to steal from other worker queue, the good part is that simple to test, deterministic and costs almost nothing.
For adding randomness , testing will be harder as it not deterministic, I don't know if you're ok with
maybe what we can do keep doing both but limit the number of stolen expiring timer fibers as it done in stealInto, so we have a room for both expiring timers and work stolen from other thread, and anyway fix the hang in enqueueBatch

[ichraf7]

I checked the git history and my understanding is that enqueueBatch has condition that never changed which is that it's only called when the queue has spare capacity , otherwise it will spin forever.
After some changes this precondition was broken in stealFromOtherWorkerThread , it assumes implicitly that the queue is still empty when it don't stole from other threads, or in our case it stole fibers of the expired timer and made the queue full.
So I think it is adequate to keep the same contract for enqueueBatch , and just before stealing from external queue check that there is enough capacity.
We stay fair and avoid asymmetry by resolving the bug that broke method contract, without adding any complexity of randomness or alternate between both of them. So ignore what I said in my previous suggestion.