Virtual async events 2 #12372

rayman22201 · 2019-10-06T23:52:58Z

Implemented VirtualAsyncEvent separate from AysncEvent

As suggested by @dom96. This leaves the original AsyncEvent API intact.
This also includes a small test suite for both AsyncEvent and VirtualAsyncEvent.

The code is mostly the same as #12232 except that it uses a single AsyncEvent as the multiplexer, instead of reaching into the global ioselector directly.

Pros:

This makes the code simpler and more modular

Cons:

This relies on the correctness and performance of the old AsyncEvent
There may be a small performance penalty from piggybacking off AsyncEvent instead of using the ioselector directly (likely very small though)

A Few Notes:

I still think the original AsyncEvent is inefficient and has some issues.
- But this PR is much more conservative and may be easier to accept. (It is non-breaking and additive only)
A very rough performance measure of the test suite shows that VirtualAsyncEvent is much faster than the old AsyncEvent, as well as being able to handle an order of magnitude more events than the old AsyncEvent. 😛
Knowledgeable users will be able decide which version they want to use.
- I have a TODO: to document the differences between the two types of AsyncEvent.
Unlike the previous version, Make Flowvar compatible with Async #11724 will no longer transparently work.
- It will need a small rewrite to use this new VirtualAsyncEvent API.

Calling @zevv, @dom96, @Araq, @cheatfate for code review and thoughts.

rayman22201 · 2019-10-06T23:54:19Z

This is going to need #12371 as a prerequisite. The test suite will fail without that fix.

dom96

Some quick comments for now.

dom96 · 2019-10-13T22:35:24Z

lib/pure/asyncdispatch.nim

@@ -924,8 +957,8 @@ when defined(windows) or defined(nimdoc):
    ## receiving notifications.
    registerWaitableEvent(fd, cb, FD_WRITE or FD_CONNECT or FD_CLOSE)

-  template registerWaitableHandle(p, hEvent, flags, pcd, timeout,
-                                  handleCallback) =
+  proc registerWaitableHandle(p: PDispatcher, hEvent: Handle, flags: DWORD, pcd: PostCallbackDataPtr, timeout: int,


dom96 · 2019-10-13T22:38:21Z

lib/pure/asyncdispatch.nim

+    VirtualFD* = distinct int
+
+    VirtualAsyncEventImpl = object
+      triggered: bool
+      when compileOption("threads"):
+        eventLock: RLock
+      p: PDispatcher
+      vFD: VirtualFD
+      cb: Callback
+    VirtualAsyncEvent* = ptr VirtualAsyncEventImpl
+


These are defined twice. Can you reduce the duplication?

Sadly no. I tried.
TLDR; It has a type dependency loop that forces the duplication.

It might be possible with {.experimental: "codeReordering".}, but here is the problem:

First, the VirtualAsyncEventImpl keeps a reference to a pDispatcher here:

Nim/lib/pure/asyncdispatch.nim

Line 292 in 264bfee

p: PDispatcher

and here:

Nim/lib/pure/asyncdispatch.nim

Line 1143 in 264bfee

p: PDispatcher

Which requires VirtualAsyncEventImpl must be declared after PDispatcher.

But,
'VirtualEventDispatcherkeeps references toVirtualAsyncEvent` here:

Nim/lib/pure/asyncdispatch.nim

Line 262 in 264bfee

virtualHandles: Table[VirtualFD, VirtualAsyncEvent] # pseudo handles for custom AsyncEvents.

and here:

Nim/lib/pure/asyncdispatch.nim

Line 1153 in 264bfee

virtualHandles: Table[VirtualFD, VirtualAsyncEvent] # pseudo handles for custom AsyncEvents.

And PDispatcher keeps a reference to VirtualEventDispatcher here:

Nim/lib/pure/asyncdispatch.nim

Line 259 in 264bfee

vd: VirtualEventDispatcher

and here:

Nim/lib/pure/asyncdispatch.nim

Line 1150 in 264bfee

vd: VirtualEventDispatcher

Which requires VirtualAsyncEventImpl must be declared before PDispatcher.

It's a type dependency loop! The only way this is allowed, is if both VirtualAsyncEventImpl and PDispatcher are declared in the same type definition block.
Because of the when defined(windows): split,
PDispatcher is declared twice, so VirtualAsyncEventImpl must also be declared twice.

Edit: I got the reason why this exists wrong. It has nothing to do with the lazy loading stuff. (Sorry, it's a complicated PR and it's been a few days. Hard to keep it all in my head.)
Even simpler:
The VirtualEventDispatcher needs to hold the physical AsyncEvent (it needs the physical event to multiplex on), and the PDispatcher needs to hold a VirtualEventDispatcher. So all three types rely on each other, and must be declared in the same type block.
If you declare the VirtualEventDispatcher first, then AsyncEvent hasn't been defined yet, and VritualEventDispatcher will error.
If you declare AsyncEvent first, then VirtualEventDispatcher hasn't been declared yet, and PDispatcher will error (note: that PDispatcher and AsyncEvent are declared together in the same type block).

@dom96,
Sorry :/
This is a terrible and convoluted explanation. Hit me up on irc if you can. I might be able to explain it better by walking through it with you live.

@Araq, I haven't been able to catch you on irc the last few days. But I would like to get your thoughts on this type dependency nonsense. I don't see a way around it, but maybe an expert(TM) can help :-P

I talked to @Araq briefly about this on IRC. He didn't look too hard, but generally this kind of thing will happen in x-platform situations until we get recursive module definitions. So it's a necessary evil for now.

dom96 · 2019-10-13T22:46:22Z

lib/pure/asyncdispatch.nim

@@ -1851,6 +1907,105 @@ proc send*(socket: AsyncFD, data: string,

  return retFuture

+proc deInitVirtualEventDispatcher(p: PDispatcher) =


This should be called close and take a VirtualEventDispatcher.

rayman22201 · 2019-10-14T22:06:48Z

lib/pure/asyncdispatch.nim

+      withLockIfThreads ev.eventLock:
+        if ev.triggered:
+          ev.triggered = false
+          p.callbacks.addLast(vcbFactory(vfd))


I really want initVirtualEventDispatcher to take a VirtualEventDispatcher object, and not a PDispatcher object.
The main reason I need the PDispatcher here is because I can think of no other way to add a virtual callback into Dispatcher callback queue.

I could have VirtualEventDispatcher maintain a separate dispatch queue. But that could cause all kinds of problems, because callback order matters. As seen here for example: #7197

It is just much safer to insert the callback into the parent PDispatcher queue.

Can't you use getGlobalDispatcher?

No. Two reasons:
1.) It's not thread safe.
2.) Related to 1, just like physical AsyncEvents, VirtualAsyncEvents are not assigned a dispatcher by default. They can technically be assigned to different dispatchers.
The API allows for this, so it has to be accounted for.

To clarify this further:

It's ok to use getGlobalDispatcher() on creation. see:

Nim/lib/pure/asyncdispatch.nim

Line 1983 in 264bfee

proc addEvent*(ev: VirtualAsyncEvent, cb: Callback) =

But the trigger event may be called from a different thread with a completely different dispatcher!

In that case, we need a way to trace back to the owning PDispatcher instance.

I changed my mind. I thought about this more, and getGlobalDispatcher is thread safe (it returns a thread local global.)

I changed the code to use getGlobalDispatcher and removed the closure requirement.

dom96

More comments, I like this approach overall much better than the previous, so I think we'll be able to get this merged soon :)

dom96 · 2019-10-16T20:28:18Z

tests/threads/testvirtualasynceventwiththreads.nim

@@ -0,0 +1,27 @@
+discard """


Maybe it would make sense to put all these tests into a virtualevent subdirectory?

That would be cool, does testament support this?

dom96 · 2019-10-16T20:29:04Z

tests/threads/testasynceventwiththreads.nim

+try:
+  runForever()
+except ValueError:
+  echo "runForever should throw ValueError, this is expected"


Why is this expected? Add a comment in code to explain.

Good question. I stole it from here (where it also doesn't have a comment to explain):
https://github.com/nim-lang/Nim/blob/devel/tests/async/t8982.nim

@narimiran is who you need to yell at for not commenting their code! /rant :-P

The answer is that runOnce() will throw an exception when it has no more pending operations:

Nim/lib/pure/asyncdispatch.nim

Line 361 in 1e5a071

raise newException(ValueError,

This is basically a hacky alternative to drain()
Remember I discussed this in this comment: #12371 (comment)

This test was the reason I started looking into drain() in the first place, and discovered #12371

Once #12371 is merged I think I can refactor this test to use drain()

dom96 · 2019-10-16T20:30:52Z

lib/pure/asyncdispatch.nim

+
+proc close*(ev: VirtualAsyncEvent) =
+  ## Closes event ``ev``.
+  doAssert(ev.vFD != InvalidVirtualFD, "Must unregister Event before you close it!")


IIRC for pretty much all other things in asyncdispatch, the close proc will implicitly unregister the FD. Please keep that consistent.

Actually, this is safer than what happens in other places in asyncdispatch.

If you look at AsyncEvent, the following happens:

It calls close on the underlying ioselector, which closes it at the kernel level, and dealloc's the memory, but it doesn't actually unregister the FD from the select dispatcher:

Nim/lib/pure/asyncdispatch.nim

Line 1546 in 233455a

proc close*(ev: AsyncEvent) =

Nim/lib/pure/ioselects/ioselectors_epoll.nim

Line 104 in 233455a

proc close*[T](s: Selector[T]) =

Maybe that is a bug?

sigh

Yeah, it's not consistent with sockets at all then...

Nim/lib/pure/asyncdispatch.nim

Lines 1320 to 1334 in 5288b54

proc closeSocket*(sock: AsyncFD) =

let selector = getGlobalDispatcher().selector

if sock.SocketHandle notin selector:

raise newException(ValueError, "File descriptor not registered.")

let data = selector.getData(sock.SocketHandle)

sock.unregister()

sock.SocketHandle.close()

# We need to unblock the read and write callbacks which could still be

# waiting for the socket to become readable and/or writeable.

for cb in data.readList & data.writeList:

if not cb(sock):

raise newException(

ValueError, "Expecting async operations to stop when fd has closed."

)

dom96 · 2019-10-16T20:37:34Z

lib/pure/asyncdispatch.nim

+    # send the signal to wake up the dispatcher thread.
+    trigger(ev.vd.virtualMuxHandle)
+
+proc addEvent*(ev: VirtualAsyncEvent, cb: Callback) =


Argh, I know you're being consistent with the AsyncEvent API here, but it annoys me that this API isn't consistent with the socket API...

In any case, it might be worth to add a signal async proc that works for both VirtualAsyncEvent and AsyncEvent. We aim to avoid callbacks after all...

This is arguably a lower level api that you wouldn't use in async level code.
The FlowVar -> Promise integration for example. A user never actually sees AsyncEvent.

IDK if it's necessary, but it might be a nice convenience to have some api that wraps a AsyncEvent / VirtualAsyncEvent in a Promise for you.

I'd rather focus on getting the fundamentals in and making that a separate PR, since I consider that sugar.

It's important to think about how the API will evolve, remember that once this is released we won't be able to change it in a breaking manner.

dom96 · 2019-10-16T20:39:01Z

lib/pure/asyncdispatch.nim

+    # The main reason we need this is to make hasPendingOperations still work
+    # without modifying the ioselector code.


can you elaborate on this? Don't really understand why this lazy loading is necessary.

It has to do with hasPendingOperations() and the multiplexer io select object.

The simplest approach is to simply create a singleton AsyncEvent object attached to the VirtualEventDispatcher , which you initialize on creation.
This AsyncEvent essentially lives forever and waits for VirtualEvents to trigger it.

This has one big problem. The lives forever part. The io selector is never closed. It must remain alive. This means the underlying ioselector implementation will always have at least one open io select that it is listening on.

If any code relies on all io selects being closed before continuing, that code will never be able to run. hasPendingOperations() and any code that relies on it is a prime example of code like this.

In #12232 I "fixed" this problem by modifying all of the io_selector implementations and asyncdispatch.nim to treat a single open io select as the same as 0 open io selects (no pending events).

After thinking about it, I realized that has approach has big problems:

First, it is a very dramatic and invasive change to the ioselector implementations. That code is foundational, so I want to be very careful with changes that I make there. The changes in Virtual async events #12232 seemed too big, and not safe.

Second, It's possible that code might rely on the process actually having all physical kernel io selectors closed (for a graceful shutdown or resource management for example). If we treat 1 open io selector the same as 0, we are essentially lying to the user, and that is not ok IMO.

I don't love lazy loading, because it is non-deterministic, but in this case, I think it's the right tool for the job. This approach is a modular solution to all of the above problems:

It works with 0 changes to the underyling ioselector code

It works with 0 changes to the core asyncdispatch code.

By core I mean no changes to AsyncEvent or any core run loop code such as runOnce()

hasPendingOperations() works correctly with 0 changes and without lying to the user

All changes are contained within the VirtualEvent additions.

All of that adds up to make this a much safer PR.

dom96 · 2019-10-16T20:45:39Z

lib/pure/asyncdispatch.nim

+proc initVirtualEventDispatcher(p: PDispatcher) =
+  p.vd.virtualMuxHandle = newAsyncEvent()
+
+  proc virtualMuxCB(fd: AsyncFD): bool {.closure,gcsafe.} =


Suggested change

proc virtualMuxCB(fd: AsyncFD): bool {.closure,gcsafe.} =

proc nativeEventCB(nativeFD: AsyncFD): bool {.closure, gcsafe.} =

dom96 · 2019-10-16T20:46:09Z

lib/pure/asyncdispatch.nim

+
+    proc vcbFactory(vfd: VirtualFD) : proc() {.closure, gcsafe.} =
+      let ev = p.vd.virtualHandles[vfd]
+      result = proc() {.closure,gcSafe.} =


Nit: move the proc () ... down to the next line and indent it.

dom96 · 2019-10-16T20:47:33Z

lib/pure/asyncdispatch.nim

+    # add them to the callback list.
+    # Not very efficient, but requires the least coordination between threads.
+
+    proc vcbFactory(vfd: VirtualFD) : proc() {.closure, gcsafe.} =


Nit:

Suggested change

proc vcbFactory(vfd: VirtualFD) : proc() {.closure, gcsafe.} =

proc vcbFactory(vfd: VirtualFD): proc() {.closure, gcsafe.} =

But also, I would probably try to avoid this being a closure (and use getGlobalDispatcher if possible like suggested below).

as I mention here: #12372 (comment)
it's not thread-safe or consistent with the API to use getGlobalDispatcher.
A closure is the easiest way to get access to the correct dispatcher given those constraints.

As a general note:
getGlobalDispatcher is not appropriate for a threaded environment. You cannot assume there is only a single asyncdispatcher in a threaded application. IMO this might cause a lot of problems in the future.

dom96 · 2019-10-16T20:48:05Z

lib/pure/asyncdispatch.nim

@@ -1851,6 +1909,105 @@ proc send*(socket: AsyncFD, data: string,

  return retFuture

+proc close(vd: VirtualEventDispatcher) =
+  assert vd.virtualHandles.len == 0, "Cannot de-Init Virtual Event Dispacter. There are still Pending Events."


dom96 · 2019-10-16T20:50:43Z

lib/pure/asyncdispatch.nim

+  ##
+  ## New ``VirtualAsyncEvent`` object is not automatically registered with
+  ## dispatcher like ``AsyncSocket``.
+  result = cast[VirtualAsyncEvent](allocShared0(sizeof(VirtualAsyncEventImpl)))


I guess we can't get around this being shared?

It's meant for multithreading so no :-P

to be more specific:
the trigger proc for VirtualAsyncEvent is meant to be callable from other threads by design. So this is shared memory by design.

@dom96

…vent As suggested by @dom96. This leaves the original AsyncEvent api intact. Also includes a small test suite for both AsyncEvent and VirtualAsyncEvent

- changed deInitVirtualEventDispatcher(p: PDispatcher) to close(vd: VirtualEventDispatcher) - changed VirtualEventDispatcher to a ref object and cleaned up some code related to this change

… of a closure.

stale · 2020-10-27T18:02:29Z

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.

stale · 2021-11-09T09:32:47Z

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.

stale · 2022-11-09T15:11:25Z

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.

dom96 reviewed Oct 13, 2019

View reviewed changes

rayman22201 commented Oct 14, 2019

View reviewed changes

dom96 requested changes Oct 16, 2019

View reviewed changes

rayman22201 added 7 commits October 28, 2019 10:32

Implemented VirtualAsyncEvent sperate from and built on top of AysncE…

4dc9e4b

…vent As suggested by @dom96. This leaves the original AsyncEvent api intact. Also includes a small test suite for both AsyncEvent and VirtualAsyncEvent

Added missing callback definition for Windows.

f9f9074

Changes based on code review from @dom96

4017da0

- changed deInitVirtualEventDispatcher(p: PDispatcher) to close(vd: VirtualEventDispatcher) - changed VirtualEventDispatcher to a ref object and cleaned up some code related to this change

Changes based on code review from @dom96

3d3174d

Updated the changelog

5541d92

Changed initVirtualEventDispatcher to use getGlobalDispatcher instead…

29b6e93

… of a closure.

Updated some async event tests.

2fabbb9

rayman22201 force-pushed the VirtualAsyncEvents2 branch from 3b05eab to 2fabbb9 Compare October 28, 2019 17:55

rayman22201 mentioned this pull request Nov 11, 2019

Make Flowvar compatible with Async #11724

Closed

zedeus mentioned this pull request Nov 26, 2019

Can't use await in templates #12743

Closed

ringabout mentioned this pull request Aug 6, 2020

A sleep in one request handler stops other handlers from serving planety/prologue#31

Closed

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

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

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 9, 2021

dom96 removed the stale Staled PR/issues; remove the label after fixing them label Nov 9, 2021

dom96 mentioned this pull request Dec 18, 2021

Awaiting for Future[void] fails if future never completes or fails #14564

Open

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

stale bot closed this Dec 16, 2022

		@@ -1851,6 +1907,105 @@ proc send*(socket: AsyncFD, data: string,

		return retFuture

		proc deInitVirtualEventDispatcher(p: PDispatcher) =

	proc closeSocket*(sock: AsyncFD) =
	let selector = getGlobalDispatcher().selector
	if sock.SocketHandle notin selector:
	raise newException(ValueError, "File descriptor not registered.")

	let data = selector.getData(sock.SocketHandle)
	sock.unregister()
	sock.SocketHandle.close()
	# We need to unblock the read and write callbacks which could still be
	# waiting for the socket to become readable and/or writeable.
	for cb in data.readList & data.writeList:
	if not cb(sock):
	raise newException(
	ValueError, "Expecting async operations to stop when fd has closed."
	)

		# The main reason we need this is to make hasPendingOperations still work
		# without modifying the ioselector code.

	proc virtualMuxCB(fd: AsyncFD): bool {.closure,gcsafe.} =
	proc nativeEventCB(nativeFD: AsyncFD): bool {.closure, gcsafe.} =

	proc vcbFactory(vfd: VirtualFD) : proc() {.closure, gcsafe.} =
	proc vcbFactory(vfd: VirtualFD): proc() {.closure, gcsafe.} =

Virtual async events 2 #12372

Virtual async events 2 #12372

Conversation

rayman22201 commented Oct 6, 2019 • edited Loading

rayman22201 commented Oct 6, 2019

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

rayman22201 Oct 14, 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

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 17, 2019 • edited Loading

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rayman22201 Oct 16, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rayman22201 Oct 16, 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 Oct 16, 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 16, 2019 • edited Loading

Choose a reason for hiding this comment

stale bot commented Oct 27, 2020

stale bot commented Nov 9, 2021

stale bot commented Nov 9, 2022

rayman22201 commented Oct 6, 2019 •

edited

Loading

rayman22201 Oct 14, 2019 •

edited

Loading

rayman22201 Oct 17, 2019 •

edited

Loading

rayman22201 Oct 16, 2019 •

edited

Loading

rayman22201 Oct 16, 2019 •

edited

Loading

rayman22201 Oct 16, 2019 •

edited

Loading

rayman22201 Oct 16, 2019 •

edited

Loading