Fix for flaky worker_steal_count test #6932
Conversation
tokio/tests/rt_unstable_metrics.rs
Outdated
| let n: u64 = (0..metrics.num_workers()) | ||
| .map(|i| metrics.worker_steal_count(i)) | ||
| .sum(); |
There was a problem hiding this comment.
We should recreate the runtime on failure. Otherwise a failed round could result in this number being wrong.
There was a problem hiding this comment.
Ah, I guess this clould be the case for #6916 too.
There was a problem hiding this comment.
Ah, I guess this clould be the case for #6916 too.
I don't think the global_queue_depth in #6916 could be tainted by a task from the previous try to block the runtime. That would require both tasks from the next try to be scheduled before the one unscheduled (but already queued) task of the previous try, which I don't believe can happen? If that could happen, the test would be racy. I'm not exactly sure what guarantees Tokio has for task execution order. Even if my assumption is true, if we are relying on an implementation detail, I think we should recreate the runtime in that test, too, to bullet proof it against future changes.
There was a problem hiding this comment.
If the deadlock scenario is what I mentioned here, then one injection queue task will remain in the queue. So I think starting another iteration without refreshing the runtime could taint the queue.
There was a problem hiding this comment.
So I think starting another iteration without refreshing the runtime could taint the queue.
That would require both tasks from the current try to block the runtime to be executed before the one remaining task from the previous try. Is that possible under the consideration that a task blocks the worker it is executed on?
(Sorry for my ignorance of the implementation details, I've yet to fully grasp the execution model of the multi-threaded runtime)
There was a problem hiding this comment.
Yes, but it's probably pretty unlikely.
There was a problem hiding this comment.
Okay, then I should definitely revisit #6916 and recreate the runtime on each try. Should I make a new PR or do the changes here?
There was a problem hiding this comment.
Up to you. I would probably make a new PR.
jofas
force-pushed
the
fix-flaky-worker-steal-count-test
branch
from
a47cc64
to
a06fa00
Compare
Darksonn
added
A-tokio
jofas
force-pushed
the
fix-flaky-worker-steal-count-test
branch
from
a06fa00
to
f44896c
Compare
Fixes #6470. Same approach as in #6916 for a similar flaky test that also relies on blocking the runtime. I tried corroborating the validity of this fix by running this test ~250k times successfully on my local machine, but I was unable to hit even a single retry in those runs.