Fix asyncdispatch drain behavior (#14820) #14838
Conversation
|
I'm investigating the Mac failures. It seems drain is ending one iteration early on Mac... because freaking Mac 😢 |
tests/async/t14820.nim
Outdated
| echo "async done" | ||
| done = true | ||
|
|
||
| discard somethingAsync() |
There was a problem hiding this comment.
I know this line is my fault, but just a heads up and maybe @dom96 would chime in that there's talk about forbidding discarding of futures (discardable pragma on async is already an error in devel), so maybe asyncCheck here instead would alleviate this becoming a blocker for such a PR in the future.
There was a problem hiding this comment.
yeah, asyncCheck would be wise here
tests/async/t14820.nim
Outdated
| echo "async done" | ||
| done = true | ||
|
|
||
| discard somethingAsync() |
There was a problem hiding this comment.
yeah, asyncCheck would be wise here
|
Test passed fine on a macbook air: |
Really strange. I wonder why the Azure CI is failing the Mac build... It looks like Azure is using MacOS 10.15. |
|
I tried to reproduce on my own Macbook air with MacOS 10.15 and I can't get the test to fail. At this point I'm not sure what to do other than push a bunch of debug logging to the PR in order to get more details into what is going on inside Azure... |
…de to account for slow Azure VMs
|
After some help from Leorize on irc, it's because the Azure VM's are slow and can easily stall async. Slowing down the test by increasing the timeouts solved the problem. I'm not sure I'm completely satisfied by this solution, but it's good enough for now, and hopefully good enough to merge. |
Drain was totally broken. Thanks to @D-Nice for pointing it out. This should be a better implementation.
I also made a tweak to the docs to make the behavior more clear, and added a test case based on the original #14820 report.
It should now have much more intuitive behavior.
@dom96 If you could please give me a code review I would very much appreciate it! Thanks! 😄 ❤️