wasip2: Fix zero-wait pollables#13511
Conversation
This commit fixes an accidental bug introduced in bytecodealliance#13085 where repeatedly calling `ready()` on a zero-wait pollable in WASIp2 would never resolve. In bytecodealliance#13085 zero-length waits were updated to unconditionally yield to tokio to improve fairness, but this didn't take into account where the yield was repeatedly cancelled and never completed. The fix in this commit is to attempt the yield once and then never attempt it again. If the original yield is cancelled the next check on the pollable will go through. This is sort of a cancellation-safety fix where the previous implementation wasn't necessarily cancellation safe in the sense that repeatedly checking-and-cancelling never let anything progress, which is counterintuitive. Closes bytecodealliance#13507
| @@ -142,6 +143,7 @@ impl Pollable for Deadline { | |||
| // are hypothetically other ways to generate a pollable that's | |||
| // always immediately ready, which this hack doesn't cover, but | |||
| // we consider this sufficient for now. | |||
There was a problem hiding this comment.
I'm primarily commenting to say: "Thank you" 🙏 and secondarily in a selfish attempt to better understand what's going on, i.e. from a place of ignorance.
I understood from your text that Pollable::ready is called multiple time and I would have absolutely written the code in the same way it was, i.e. it's surprising to me that the status quote cannot make progress. Wouldn't all futures returned by yield_now get resolved eventually even if some of them get dropped?
Since I don't even have the context why Pollable::ready gets called multiple times, I'm wondering if this can lead to some re-ordering of tasks? First call will yield, second won't which means a second continuation may get scheduled earlier than the first. This is probably not an issue since tokio doesn't give any ordering guarantees but may still be surprising 🤷.
Maybe there's some way in the downstream consuming code to address this issue rather than the conditional yielding? Then again, I've no clue and you should do what makes the most sense.
From a mere user's perspective, I would expect repeated yields from WASM to lead to repeated yields to the tokio runtime ... as long as that's the case 🤷♀️
EDIT: specifically await Timer(0) is a common approach in many languages (JS, Dart, ...) to yield to the runtime/event-loop.
There was a problem hiding this comment.
The implementation of pollable.ready(), which is what wstd is calling, creates this Pollable::ready future defined here, polls it, then drops it. With the previous implementation it would never make progress since the yield was attempted to happen every time. It looks like wstd never actually blocked on the pollable, it only kept asking if it was ready. This patched implementation still preserves yielding behavior, but the yield is only attempted once.
2 participants
This commit fixes an accidental bug introduced in #13085 where repeatedly calling
ready()on a zero-wait pollable in WASIp2 would never resolve. In #13085 zero-length waits were updated to unconditionally yield to tokio to improve fairness, but this didn't take into account where the yield was repeatedly cancelled and never completed. The fix in this commit is to attempt the yield once and then never attempt it again. If the original yield is cancelled the next check on the pollable will go through. This is sort of a cancellation-safety fix where the previous implementation wasn't necessarily cancellation safe in the sense that repeatedly checking-and-cancelling never let anything progress, which is counterintuitive.Closes #13507