Asynchronous scheduler behaviour

[cheatfate]

Asynchronous dead end.

Recent changes to asyncdispatch caused damage to core scheduler behavior. This changes called:
Lets finish all Futures[T], which we can finish in one tick (poll() call).

Now i want to show you some circumstances:

import asyncdispatch
proc testProc() {.async.} =
  for i in 1..1_000:
    await sleepAsync(5)
    echo "Timeout event " & $i
proc callbackProc() =
  echo "Callback event"
when isMainModule:
  discard getGlobalDispatcher()
  asyncCheck testProc()
  for i in 1..100:
    echo "Iteration " & $i
    poll()
    callbackProc()

This code is pretty simple and its output will be looking like this:

Iteration 1
Callback event
Iteration 2
Callback event
Iteration 3
Callback event
Iteration 4
Callback event
Iteration 5
Callback event
Iteration 6
Timeout event 1
Callback event
Iteration 7
Callback event
Iteration 8
Callback event
Iteration 9
Callback event
Iteration 10
Callback event
Iteration 11
Callback event
Iteration 12
Callback event
Iteration 13
Timeout event 2

You can see here that callbackProc() is called every tick, also you can notice
here work of timer, and everything works as expected.

Now lets see almost equal code sample. Actually it was equal to previous one,
only before "Let finish all Futures[T] we can".

import asyncdispatch
proc testProc() {.async.} =
  for i in 1..1_000:
    await sleepAsync(5)
    echo "Timeout event " & $i
proc callbackProc() =
  echo "Callback event"
  callSoon(callbackProc)
when isMainModule:
  discard getGlobalDispatcher()
  asyncCheck testProc()
  callSoon(callbackProc)
  for i in 1..100:
    echo "Iteration " & $i
    poll()

This code change is callSoon(), which was introduced to fix recursion bugs, and allow developers to use it to set callbacks for every tick.
So callSoon(callbackProc) is just adding itself to the list of callbacks which going to be scheduled inside of poll().

Code output is looks like this:

Iteration 1
Callback event
Callback event
Callback event
Callback event
Callback event
Callback event
Callback event
Callback event
Callback event
Callback event
Callback event
Callback event
...

This code will never ends, just because scheduler tries to finish all callbacks.

And because new callback is added, this code will never ends. Also you will never see in output timer events, so timer and any IO operations become unreliable.

[zielmicha]

Isn't this expected? callbackProc is clearly an infinite loop, would you expect while true: discard to work correctly?

proc callbackProc() =
  echo "Callback event"
  callSoon(callbackProc)

[cheatfate]

Of course this is not expected, expected result is showed in first code sample,

So code inside of callbackProc() in 2nd code sample runs every one tick/poll() call and adds itself in callback list for next tick/poll() call. It must not create infinite loop, and it must allow IO operations and timers to be processed concurrently.

So this is not actually while true: discard, but recent additions to asyncdispatch, made it.

[cheatfate]

Overall current approach is not totally wrong and can be avoided, but for synchronization between tasks its not very good to have such behavior.

[nitely]

Events should be dispatched in FIFO order. Or maybe is sleep not getting scheduled?. I can understand why pool would never return, though. FWIW, that is how Python asyncio works[0], and I've relied on this in JS more than once.

[0] https://gist.github.com/nitely/c143752aa71bc4ca6cb42e9946422438

[cheatfate]

@nitely, first of all sleep will never get scheduled, my both sample will give you equal results in python asyncio. While current asyncdispatch implementation do not handle timers at all.

[nitely]

@cheatfate well yeah, I was agreeing with you. If you check the gist I posted it behaves as you want it to.

[cheatfate]

@nitely sorry, not mentioned that you posted actually Python code, not Nim code.

[narimiran]

In #7759, which was meant to be a fix for this, it is mentioned:

This was fixed by @dom96 differently. Closing.

@cheatfate did the fix work? Can we close this?

[Clyybber]

@narimiran No this is not fixed. I just tested it.

[ghost]

Still runs forever:

import asyncdispatch

proc testProc() {.async.} =
  for i in 1..1_000:
    await sleepAsync(5)
    echo "Timeout event " & $i

proc callbackProc() {.gcsafe.} =
  echo "Callback event"
  callSoon(callbackProc)

when isMainModule:
  discard getGlobalDispatcher()
  asyncCheck testProc()
  callSoon(callbackProc)
  for i in 1..100:
    echo "Iteration " & $i
    poll()

[dom96]

Honestly, I'm tempted to close this as a wontfix.

Practically speaking this hasn't been a problem for me or for anyone from what I've seen, and in some ways as @zielmicha highlights this isn't even surprising behaviour.

Anyway, the reason these are the semantics is because of this code:

Nim/lib/pure/asyncdispatch.nim

Lines 205 to 209 in 4ae3413

	proc processPendingCallbacks(p: PDispatcherBase; didSomeWork: var bool) =
	while p.callbacks.len > 0:
	var cb = p.callbacks.popFirst()
	cb()
	didSomeWork = true

So as far as I can tell the fix is simple, but is it really "right"? This contrived example does not convince me that we should potentially break code in changing the semantics here. If someone has a good argument for why it's important that we do change the semantics then please share them.