Wrong poll timeout behavior in asyncdispatch

[yglukhov]

import asyncdispatch, times

proc foo(): Future[int] {.async.} =
  return 1

proc bar(): Future[int] {.async.} =
  return await foo()

echo "start: ", epochTime()
echo waitFor(bar())
echo "end: ", epochTime()

The program will take 0.5 seconds to execute, but it should be instant

[cheatfate]

I think the best resolution for this issue is to set default value for asyncdispatch.poll timeout to 0ms.

[dom96]

I don't think that's a good idea. It can lead to 100% CPU usage when there is absolutely nothing being done by the program.

[SSPkrolik]

@cheatfate maybe we should use (-1 / indefinite blocking timeout) as a default value, not 0ms which will use 100% cppu

[dom96]

I'm not sure what setting it to -1 will solve.

[SSPkrolik]

@dom96 that was a thought in general (against 0ms timeout), that default poll timeout must be infinite, it doesn't fix the bug though

[cheatfate]

@dom96, of course it can lead to 100% cpu usage, but if it happens, then something wrong with your application...

[dom96]

No, there is nothing wrong with your application in that case. It's the poll that should be blocking for some time, nothing else.

[cheatfate]

@dom96, we are using this case only for tests... there no real word case to use asyncdispatch in such way...

[cheatfate]

@dom96 and also, if your case exist why asyncdispatch has this check:
https://github.com/nim-lang/Nim/blob/devel/lib/pure/asyncdispatch.nim#L464-L466

[dom96]

If we change poll's default timeout to 0 then runForever will use 100% CPU. You're arguing that the dozens of applications which use runForever are "wrong".

[cheatfate]

Ok, one more idea.
We can check number of descriptors registered in selector, and if selector is empty, then we don't call select() at all...

[dom96]

I'm guessing that @yglukhov's example is simplified. He might actually have descriptors registered in his selector.

[SSPkrolik]

@dom96 actually, firstly the bug was discovered when nimongo package async client tests began to run times slower than before after recent Nim updates.

[cheatfate]

I think my new idea will resolve bug, because nimongo uses some kind of hack for asyncdispatch to poll connections.

[dom96]

because nimongo uses some kind of hack for asyncdispatch to poll connections.

Link?

[cheatfate]

I dont know @dom96, what hack using nimongo, i just think it has... because this is very strange situation...

[cheatfate]

@yglukhov, so we just leaving this issue?

[cheatfate]

@yglukhov, @SSPkrolik i think i know what the problem has nimongo.
You trying to maintain connection pool, so you make connect for every socket in pool and after all or some sockets becomes idle, this causes asyncdispatch to update(socket) with no events (and selectors.nim removes this socket from watching queue). So some times you have empty selector's queue, and this causes timeouts of 500ms because queue is empty.

[dom96]

If that is indeed the problem then it seems the solution is simple:

• If there are no sockets waiting for events and there is a callback in the queue then
  • skip the call to select.

[SSPkrolik]

@cheatfate Yes, you're right, the problem is with the next proc, which returns socket from the pool: it returns socket immediately, but instead the callback is callSooned, and while there are no sockets registered when poll is called, the 500ms timeout happens.

Anyway, in this case (calling await for function that returns immediately in certain cases), we receive a situation when there are pending unprocessed callbacks right before call of poll, which is not a good behavior. Can we make additional call in poll right before select to processPendingCallbacks?

P.S.
Or the solution that @dom96 suggests

[cheatfate]

this is not @dom96 solution, this is mine solution :) the problem is that selectors.nim don't count registered/unregistered descriptors.

[cheatfate]

@SSPkrolik but i still can't understand why you are fall to 500ms timeout if you have sleepAsync(1) in your next proc.

[dom96]

this is not @dom96 solution, this is mine solution :)

Great minds think alike :)

[cheatfate]

@dom96 ok, but my post about this idea was 2 days ago :)

[SSPkrolik]

@cheatfate We fall into timeout because sleepAsync(1) is only called when all sockets are busy, and in case of tests, all requests are done sequentially, that's why we always get first socket which is free, and sleepAsync(1) is never called.