Virtual async events #12232

rayman22201 · 2019-09-22T10:05:11Z

AsyncEvents currently create a new physical file descriptor for every new event. This can quickly exhaust the file descriptors on your system if you create too many events.

This is a big rewrite of AsyncEvent to use "virtual file descriptors" that are multiplexed over a single global physical file descriptor. As a side effect, also makes AsyncEvent threadsafe.

This should be completely compatible with the old asyncEvents. I have tried to maintain complete backwards compatibility of the behavior.

The only thing an end user should notice is that they can have a lot more AsyncEvents than they used to.

This PR will finally allow #11724 to work properly.

zevv · 2019-09-30T21:04:19Z

@dom96 and or other stdlib people, could you give this some feedback?

I also heard people are concerned about performance because of the locking, but I'm not sure how this is a valid point at this time. There is no alternative implementation to compare performance with, so there is nothing to gain from benchmarking at this time.

If this implementation proves correct, please lets merge it so we can have this feature. then people can complain about how terrible the implementation is and improve on that - while keeping the implementation correct.

Araq · 2019-10-01T07:18:26Z

@zevv I'm actually waiting on the followup PRs.

rayman22201 · 2019-10-01T17:00:10Z

AFAIK, PR #11724 should work with this PR with no changes.

Araq · 2019-10-01T20:06:23Z

lib/pure/asyncdispatch.nim

+        eventLock: RLock
+      p: PDispatcher
+      vFD: VirtualFD
+      callbacks: seq[Callback]


This is not threadsafe. seq are thread-local.

The callback seq should only ever be accessed by a single thread: the thread that owns the async event loop.

It would be very odd for a thread to register a callback on a different threads event loop. Even if this were made this thread safe, the callback itself might access memory from a different thread.

A couple of options:

I could factor the seq out and create a global seq in the asyncDispatcher singleton.

Or I could make it a single callback. I made this a seq to make it more extensible, but it's not required. (In fact, the old system only allows a single callback to be tied to an AsyncEvent).

old AsyncEvent is able to synchronize different threads, but proposed AsyncEvent is unable to be used in equal scenarios.

@cheatfate, can you give an example?

Old async events is not threadsafe. The old AsyncEvent allowed cross thread access to the asyncEvent memory with no synchronization or race protection at all. This system explicitly synchronizes threads through locking.

Old async events is not threadsafe. The old AsyncEvent allowed cross thread access to the asyncEvent memory with no synchronization or race protection at all. This system explicitly synchronizes threads through locking.

Windows:
Sorry but not only locks are used to synchronize threads: https://github.com/nim-lang/Nim/blob/devel/lib/pure/asyncdispatch.nim#L810 . Windows OS has events (which in Posix often called conditional variables).
Linux:
Linux version uses eventfd https://github.com/nim-lang/Nim/blob/devel/lib/pure/ioselects/ioselectors_epoll.nim#L112-L118.
So lets check about thread safety of eventfd() via man eventfd:

ATTRIBUTES For an explanation of the terms used in this section, see attributes(7). ┌──────────┬───────────────┬─────────┐ │Interface │ Attribute │ Value │ ├──────────┼───────────────┼─────────┤ │eventfd() │ Thread safety │ MT-Safe │ └──────────┴───────────────┴─────────┘

BSD/MacOS and all other Posix systems:
All this systems are using OS pipes:
https://github.com/nim-lang/Nim/blob/devel/lib/pure/ioselects/ioselectors_kqueue.nim#L136
https://github.com/nim-lang/Nim/blob/devel/lib/pure/ioselects/ioselectors_poll.nim#L191

And some information about pipes form man pipe:

pipe() creates a pipe, a unidirectional data channel that can be used for interprocess communication.

So i think if its created for interprocess communication it will be fine to use it for interthread communication.

This is example of use:
https://gist.github.com/cheatfate/87b672034be5b71ab6a975266558ed8c
But if you encounter any problems its some internal asyncdispatch bugs, i'm not involved in asyncdispatch development anymore so i'm not following it. But this sample is exactly shows how it must work.

@cheatfate, your test actually fails on current devel Nim as follows:

/home/ray/Nim/Nim-devel/cheatfateAsync.nim(38) threadProc2 /home/ray/.choosenim/toolchains/nim-#devel/lib/pure/asyncdispatch.nim(1879) waitFor /home/ray/.choosenim/toolchains/nim-#devel/lib/pure/asyncdispatch.nim(1569) poll /home/ray/.choosenim/toolchains/nim-#devel/lib/pure/asyncdispatch.nim(1335) runOnce /home/ray/.choosenim/toolchains/nim-#devel/lib/pure/asyncdispatch.nim(210) processPendingCallbacks /home/ray/.choosenim/toolchains/nim-#devel/lib/pure/asyncmacro.nim(34) asyncProc2NimAsyncContinue /home/ray/Nim/Nim-devel/cheatfateAsync.nim(28) asyncProc2Iter /home/ray/Nim/Nim-devel/cheatfateAsync.nim(17) wait /home/ray/.choosenim/toolchains/nim-#devel/lib/pure/asyncdispatch.nim(1556) addEvent /home/ray/.choosenim/toolchains/nim-#devel/lib/pure/ioselects/ioselectors_epoll.nim(368) registerEvent /home/ray/.choosenim/toolchains/nim-#devel/lib/pure/selectors.nim(279) raiseIOSelectorsError [[reraised from: /home/ray/Nim/Nim-devel/cheatfateAsync.nim(38) threadProc2 /home/ray/.choosenim/toolchains/nim-#devel/lib/pure/asyncdispatch.nim(1881) waitFor /home/ray/.choosenim/toolchains/nim-#devel/lib/pure/asyncfutures.nim(383) read ]] Error: unhandled exception: File exists (code: 17) Async traceback: /home/ray/Nim/Nim-devel/cheatfateAsync.nim(38) threadProc2 /home/ray/.choosenim/toolchains/nim-#devel/lib/pure/asyncdispatch.nim(1879) waitFor /home/ray/.choosenim/toolchains/nim-#devel/lib/pure/asyncdispatch.nim(1569) poll /home/ray/.choosenim/toolchains/nim-#devel/lib/pure/asyncdispatch.nim(1335) runOnce /home/ray/.choosenim/toolchains/nim-#devel/lib/pure/asyncdispatch.nim(210) processPendingCallbacks /home/ray/.choosenim/toolchains/nim-#devel/lib/pure/asyncmacro.nim(34) asyncProc2NimAsyncContinue /home/ray/Nim/Nim-devel/cheatfateAsync.nim(28) asyncProc2Iter /home/ray/Nim/Nim-devel/cheatfateAsync.nim(17) wait /home/ray/.choosenim/toolchains/nim-#devel/lib/pure/asyncdispatch.nim(1556) addEvent /home/ray/.choosenim/toolchains/nim-#devel/lib/pure/ioselects/ioselectors_epoll.nim(368) registerEvent /home/ray/.choosenim/toolchains/nim-#devel/lib/pure/selectors.nim(279) raiseIOSelectorsError Exception message: File exists (code: 17) Exception type: [IOSelectorsException] Error: execution of an external program failed: '/home/ray/Nim/Nim-devel/cheatfateAsync '

On the VirtualAsyncPR it does not have this issue but hits a race condition between the addEvent and trigger procs.
line 17 and lines 22/28 respectively.

It's possible to trigger the event in one thread, before the other thread has "re-registered" the event. I'm not sure if AsyncEvent was meant to be constantly unregistered and re-registered this way.

The following change fixes the race condition I observe with my PR: https://gist.github.com/rayman22201/e6a9dcde22e009dbd397b8755eae93d7#file-asyncevent-nim-L22
That guarantees the event is registered before it is awaited on either thread.

Note: the test still fails with the File exists (code: 17) Exception on Devel.

dom96

I haven't delved deep into the details of how you've implemented this, but did leave a bunch of feedback about how the code is organised.

As an overall question, I wonder whether we could keep the current AsyncEvent as-is and introduce a VirtualAsyncEvent on top of it? This would allow the best of both worlds for those that don't want this virtual logic and should make the code easier to deal with. What do you think?

dom96 · 2019-10-01T22:19:35Z

lib/pure/asyncdispatch.nim

+
+    var data = newAsyncData()
+    data.readList.add(virtualMuxCB)
+    result.selector.registerEvent(result.virtualMuxHandle, data)


This sort of code would do well to be put in a separate procedure called initVirtualAsyncEvents or the like.

dom96 · 2019-10-01T22:20:10Z

lib/pure/asyncdispatch.nim

+
+    ev.callbacks.add(cb)
+    ev.vFD = p.nextVirtualHandle
+    p.nextVirtualHandle = VirtualFD( int(p.nextVirtualHandle) + 1 )


Nit: no space after ( or before it.

dom96 · 2019-10-01T22:21:08Z

lib/pure/asyncdispatch.nim

+                                       cast[pointer](p.ovl))
+  {.pop.}
+
+  template registerWaitableHandle(p, hEvent, flags, pcd, timeout,


Could this be a proc instead?

IDK. I suppose so? It was this way when I found it. I saw no reason to change it. I just moved the template up because I needed it earlier in the code and Nim doesn't allow forward declarations.

I didn't want to change more things than I already did. This PR is enough of a bulldozer already...

ahh, I didn't realise you moved it up. It really needs to be refactored IMO...

dom96 · 2019-10-01T22:22:07Z

lib/pure/asyncdispatch.nim

+      virtualHandles: Table[VirtualFD, AsyncEvent] # pseudo handles for custom AsyncEvents.
+      nextVirtualHandle: VirtualFD
+      virtualMuxHandle: Handle     # all the virtual handles get multiplexed through a single real handle.
+      virtualPCD: PostCallbackDataPtr # needed for the selector callback plumbing.


perhaps all these fields could be put in a separate object?

Sure. I left it all together for simplicity (and I was organically adding fields as I needed them), but I can see the organizational advantages of making a separate object.

rayman22201 · 2019-10-01T23:55:07Z

@dom96

I haven't delved deep into the details of how you've implemented this, but did leave a bunch of feedback about how the code is organised.

As an overall question, I wonder whether we could keep the current AsyncEvent as-is and introduce a VirtualAsyncEvent on top of it? This would allow the best of both worlds for those that don't want this virtual logic and should make the code easier to deal with. What do you think?

First, thanks for the feedback <3

Even without multi-threading, the old AsyncEvent is broken. The old AsyncEvent makes it way too easy to exhaust the file descriptors on your OS. It's dangerous and a waste of system resources.

I see this PR as essentially an elaborate bug fix that happens to benefit the multi-threaded use case as a side effect...

Threads and FlowVar is what exposed the bug, but there may be other cases where you might need many AsyncEvents, even in a single threaded environment.

cheatfate · 2019-10-03T07:48:08Z

Even without multi-threading, the old AsyncEvent is broken. The old AsyncEvent makes it way too easy to exhaust the file descriptors on your OS. It's dangerous and a waste of system resources.

I see this PR as essentially an elaborate bug fix that happens to benefit the multi-threaded use case as a side effect...

Threads and FlowVar is what exposed the bug, but there may be other cases where you might need many AsyncEvents, even in a single threaded environment.

AsyncEvent is not supposed to be used in single-threaded environment, its a tool to sync multiple thread's dispatchers, so its exactly not broken, its purpose is different from what you expected (I think documentation can be improved). To work in single-threaded environment you can use more simple mechanism like this https://github.com/cheatfate/asynctools/blob/master/asynctools/asyncsync.nim and you actually do not need any patches to existing asyncdispatch.nim codebase to start using it.

rayman22201 · 2019-10-03T16:23:07Z

@cheatfate Let me abundantly clear here:

its a tool to sync multiple thread's dispatchers, so its exactly not broken, its purpose is different from what you expected

I know exactly what it's meant to be used for. Araq, Dom96, and I used it for it's intended purpose, live on a video stream. And we found this bug.

Let me reiterate the bug for you:
"AsyncEvents currently create a new physical file descriptor for every new event. This can quickly exhaust the file descriptors on your system if you create too many events."

I know that pipes (events on windows) synchronize. There is still a pipe used in this new implementation. It provides the same level of synchronization. The difference is one pipe / event is shared to save resources.

I have added extra locking because the synchronization from the pipe is not guaranteed to protect the memory accessed in the trigger proc.

I just ran your test with this new implementation and it worked exactly as expected.
Thank you for the test, I will add it to the test suite. It's a good one to have.

rayman22201 · 2019-10-03T16:26:10Z

@dom96

I wonder whether we could keep the current AsyncEvent as-is and introduce a VirtualAsyncEvent on top of it?

To be clear, I don't think there is a need for two different solutions, because this should work exactly like the old AsyncEvent.

AFAICT, this should be a transparent change from the API point of view, only with more efficient use of system FD resources.

cheatfate · 2019-10-03T21:17:19Z

lib/pure/asyncdispatch.nim

+        eventLock: RLock
+      p: PDispatcher
+      vFD: VirtualFD
+      cb: Callback


Callback is closure so it still from thread-local heap, and PDispatcher is also from thread-local heap.

The only field that is accessed from multiple threads is the triggered bool, and it is protected by eventLock.
The other fields are never accessed from a different thread.

AsyncEvent used to create a new File Descriptor for every new event. This will quickly exhaust all the file handles available on the machine. This rewrite multiplexes all the AsyncEvents with virtual File Descriptors over a single physical File Descriptor.

- implemented a withLockIfThreads template to reduce when compileOptions("threads") boilerplate. Suggested by Zevv. - Added an InvalidVirtualFD constant for semantic clarity - Bug Fix: Triggering an AsyncEvent that is not registered would crash with a segv. It now simply acts as a noop and returns.

AsyncEvent used to create a new File Descriptor for every new event. This will quickly exhaust all the file handles available on the machine. This rewrite multiplexes all the AsyncEvents with virtual File Descriptors over a single physical File Descriptor.

…rigger and addEvent

stale · 2022-12-04T01:20:55Z

This pull request has been automatically marked as stale because it has not had recent activity. If you think it is still a valid PR, please rebase it on the latest devel; otherwise it will be closed. Thank you for your contributions.

Araq reviewed Oct 1, 2019

View reviewed changes

dom96 reviewed Oct 1, 2019

View reviewed changes

cheatfate reviewed Oct 3, 2019

View reviewed changes

rayman22201 and others added 8 commits October 4, 2019 15:38

Fixes to make the windows impl of AsyncEvent happy.

7f30542

Implemented code review comments from Dom and Araq

e78060b

[backport] run nimpretty on async

5e7e045

Implemented code review comments from Dom and Araq

75f29a3

Added a test suggested by Cheatface. Fixed a race condition between t…

38ebbf1

…rigger and addEvent

rayman22201 force-pushed the VirtualAsyncEvents branch from 5e66623 to 38ebbf1 Compare October 4, 2019 22:50

Harden the cheatfate by producing more predictable output.

0b09eb9

This was referenced Oct 6, 2019

Test + fix for epoll and kqueue selector modules to properly unregister event handles that have the key type "User" #12371

Merged

Virtual async events 2 #12372

Closed

stale bot added the stale Staled PR/issues; remove the label after fixing them label Oct 4, 2020

stale bot closed this Nov 3, 2020

Araq reopened this Nov 3, 2020

stale bot removed the stale Staled PR/issues; remove the label after fixing them label Nov 3, 2020

Merge branch 'devel' into VirtualAsyncEvents

8f4fc97

dom96 mentioned this pull request Dec 18, 2020

Completing the Nim async/await implementation nim-lang/RFCs#304

Open

stale bot added the stale Staled PR/issues; remove the label after fixing them label Nov 3, 2021

stale bot closed this Dec 3, 2021

nim-lang deleted a comment from stale bot Dec 3, 2021

ringabout reopened this Dec 3, 2021

stale bot removed the stale Staled PR/issues; remove the label after fixing them label Dec 3, 2021

stale bot added the stale Staled PR/issues; remove the label after fixing them label Dec 4, 2022

stale bot closed this Jan 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Virtual async events #12232

Virtual async events #12232

rayman22201 commented Sep 22, 2019

zevv commented Sep 30, 2019

Araq commented Oct 1, 2019

rayman22201 commented Oct 1, 2019

Araq Oct 1, 2019

rayman22201 Oct 1, 2019

rayman22201 Oct 1, 2019

cheatfate Oct 2, 2019 •

edited

Loading

rayman22201 Oct 2, 2019 •

edited

Loading

cheatfate Oct 3, 2019 •

edited

Loading

cheatfate Oct 3, 2019

rayman22201 Oct 3, 2019

rayman22201 Oct 4, 2019 •

edited

Loading

dom96 left a comment

dom96 Oct 1, 2019

dom96 Oct 1, 2019

dom96 Oct 1, 2019

rayman22201 Oct 1, 2019 •

edited

Loading

dom96 Oct 3, 2019

dom96 Oct 1, 2019

rayman22201 Oct 1, 2019

rayman22201 commented Oct 1, 2019

cheatfate commented Oct 3, 2019 •

edited

Loading

rayman22201 commented Oct 3, 2019

rayman22201 commented Oct 3, 2019

cheatfate Oct 3, 2019

rayman22201 Oct 3, 2019

stale bot commented Dec 4, 2022

Virtual async events #12232

Virtual async events #12232

Conversation

rayman22201 commented Sep 22, 2019

zevv commented Sep 30, 2019

Araq commented Oct 1, 2019

rayman22201 commented Oct 1, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cheatfate Oct 2, 2019 • edited Loading

Choose a reason for hiding this comment

rayman22201 Oct 2, 2019 • edited Loading

Choose a reason for hiding this comment

cheatfate Oct 3, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rayman22201 Oct 4, 2019 • edited Loading

Choose a reason for hiding this comment

dom96 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rayman22201 Oct 1, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rayman22201 commented Oct 1, 2019

cheatfate commented Oct 3, 2019 • edited Loading

rayman22201 commented Oct 3, 2019

rayman22201 commented Oct 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stale bot commented Dec 4, 2022

cheatfate Oct 2, 2019 •

edited

Loading

rayman22201 Oct 2, 2019 •

edited

Loading

cheatfate Oct 3, 2019 •

edited

Loading

rayman22201 Oct 4, 2019 •

edited

Loading

rayman22201 Oct 1, 2019 •

edited

Loading

cheatfate commented Oct 3, 2019 •

edited

Loading