From adc1419499d6e02438374e820321b3fbe293fb56 Mon Sep 17 00:00:00 2001 From: Ray Imber Date: Sun, 6 Oct 2019 16:02:12 -0700 Subject: [PATCH 1/9] Test + fix for epoll and kqueue selector modules to properly unregister event handles that have the key type "User" --- lib/pure/ioselects/ioselectors_epoll.nim | 2 +- lib/pure/ioselects/ioselectors_kqueue.nim | 4 ++-- tests/async/testmanyasyncevents.nim | 23 +++++++++++++++++++++++ 3 files changed, 26 insertions(+), 3 deletions(-) create mode 100644 tests/async/testmanyasyncevents.nim diff --git a/lib/pure/ioselects/ioselectors_epoll.nim b/lib/pure/ioselects/ioselectors_epoll.nim index 7c6f151648..da7ef8a9ac 100644 --- a/lib/pure/ioselects/ioselectors_epoll.nim +++ b/lib/pure/ioselects/ioselectors_epoll.nim @@ -197,7 +197,7 @@ proc unregister*[T](s: Selector[T], fd: int|SocketHandle) = "Descriptor $# is not registered in the selector!" % $fdi) if pkey.events != {}: when not defined(android): - if pkey.events * {Event.Read, Event.Write} != {}: + if pkey.events * {Event.Read, Event.Write, Event.User} != {}: var epv = EpollEvent() # TODO: Refactor all these EPOLL_CTL_DEL + dec(s.count) into a proc. if epoll_ctl(s.epollFD, EPOLL_CTL_DEL, fdi.cint, addr epv) != 0: diff --git a/lib/pure/ioselects/ioselectors_kqueue.nim b/lib/pure/ioselects/ioselectors_kqueue.nim index 0c51e67f49..9e1d1a800c 100644 --- a/lib/pure/ioselects/ioselectors_kqueue.nim +++ b/lib/pure/ioselects/ioselectors_kqueue.nim @@ -375,7 +375,7 @@ proc unregister*[T](s: Selector[T], fd: int|SocketHandle) = "Descriptor [" & $fdi & "] is not registered in the queue!") if pkey.events != {}: - if pkey.events * {Event.Read, Event.Write} != {}: + if pkey.events * {Event.Read, Event.Write, Event.User} != {}: if Event.Read in pkey.events: modifyKQueue(s, uint(fdi), EVFILT_READ, EV_DELETE, 0, 0, nil) dec(s.count) @@ -632,4 +632,4 @@ template withData*[T](s: Selector[T], fd: SocketHandle|int, value, body1, proc getFd*[T](s: Selector[T]): int = - return s.kqFD.int \ No newline at end of file + return s.kqFD.int diff --git a/tests/async/testmanyasyncevents.nim b/tests/async/testmanyasyncevents.nim new file mode 100644 index 0000000000..5453810134 --- /dev/null +++ b/tests/async/testmanyasyncevents.nim @@ -0,0 +1,23 @@ +discard """ +output: ''' +hasPendingOperations: false +triggerCount: 512 +''' +""" + +import asyncDispatch + +var triggerCount = 0 +var evs = newSeq[AsyncEvent]() + +for i in 0 ..< 512: # has to be lower than the typical physical fd limit + var ev = newAsyncEvent() + evs.add(ev) + addEvent(ev, proc(fd: AsyncFD): bool {.gcsafe,closure.} = triggerCount += 1; true) + +for ev in evs: + ev.trigger() + +drain() +echo "hasPendingOperations: ", hasPendingOperations() +echo "triggerCount: ", triggerCount From 0338516e705f4ea26769509c535d2a10f34039aa Mon Sep 17 00:00:00 2001 From: Ray Imber Date: Sun, 6 Oct 2019 17:33:58 -0700 Subject: [PATCH 2/9] Additional fix for User key unregister in the KQueue backend --- lib/pure/ioselects/ioselectors_kqueue.nim | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/pure/ioselects/ioselectors_kqueue.nim b/lib/pure/ioselects/ioselectors_kqueue.nim index 9e1d1a800c..6471b76245 100644 --- a/lib/pure/ioselects/ioselectors_kqueue.nim +++ b/lib/pure/ioselects/ioselectors_kqueue.nim @@ -376,7 +376,7 @@ proc unregister*[T](s: Selector[T], fd: int|SocketHandle) = if pkey.events != {}: if pkey.events * {Event.Read, Event.Write, Event.User} != {}: - if Event.Read in pkey.events: + if Event.Read in pkey.events or Event.User in pkey.events: modifyKQueue(s, uint(fdi), EVFILT_READ, EV_DELETE, 0, 0, nil) dec(s.count) if Event.Write in pkey.events: From b347490f9160bf02dda5333cbc76817e0dcbce50 Mon Sep 17 00:00:00 2001 From: Ray Imber Date: Mon, 7 Oct 2019 10:06:42 -0700 Subject: [PATCH 3/9] lowered the number of events in the test because some CI's have an extremely low FD limit --- tests/async/testmanyasyncevents.nim | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/async/testmanyasyncevents.nim b/tests/async/testmanyasyncevents.nim index 5453810134..5a103736fd 100644 --- a/tests/async/testmanyasyncevents.nim +++ b/tests/async/testmanyasyncevents.nim @@ -1,7 +1,7 @@ discard """ output: ''' hasPendingOperations: false -triggerCount: 512 +triggerCount: 100 ''' """ @@ -10,7 +10,7 @@ import asyncDispatch var triggerCount = 0 var evs = newSeq[AsyncEvent]() -for i in 0 ..< 512: # has to be lower than the typical physical fd limit +for i in 0 ..< 100: # has to be lower than the typical physical fd limit var ev = newAsyncEvent() evs.add(ev) addEvent(ev, proc(fd: AsyncFD): bool {.gcsafe,closure.} = triggerCount += 1; true) From aa84d35d8668e40e99e1a94ef859941bae029a72 Mon Sep 17 00:00:00 2001 From: Ray Imber Date: Thu, 10 Oct 2019 16:49:56 -0700 Subject: [PATCH 4/9] Fix io slector unregister for windows as well. --- lib/pure/ioselects/ioselectors_select.nim | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/pure/ioselects/ioselectors_select.nim b/lib/pure/ioselects/ioselectors_select.nim index 1f608e43e2..2d53da26b9 100644 --- a/lib/pure/ioselects/ioselectors_select.nim +++ b/lib/pure/ioselects/ioselectors_select.nim @@ -286,7 +286,7 @@ proc unregister*[T](s: Selector[T], fd: SocketHandle|int) = s.withSelectLock(): let fd = fd.SocketHandle var pkey = s.getKey(fd) - if Event.Read in pkey.events: + if pkey.events * {Event.Read, Event.User} != {}: IOFD_CLR(fd, addr s.rSet) dec(s.count) if Event.Write in pkey.events: From 4e6f2b5313418c5f070611adb3b29a70c621782b Mon Sep 17 00:00:00 2001 From: Ray Imber Date: Thu, 10 Oct 2019 17:53:44 -0700 Subject: [PATCH 5/9] Fix drain to correctly take into account hasPendingOperations and the timeout value --- lib/pure/asyncdispatch.nim | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/pure/asyncdispatch.nim b/lib/pure/asyncdispatch.nim index f540c52074..38946e82fa 100644 --- a/lib/pure/asyncdispatch.nim +++ b/lib/pure/asyncdispatch.nim @@ -1559,8 +1559,8 @@ proc drain*(timeout = 500) = ## Waits for completion events and processes them. Raises ``ValueError`` ## if there are no pending operations. In contrast to ``poll`` this ## processes as many events as are available. - if runOnce(timeout): - while hasPendingOperations() and runOnce(0): discard + if runOnce(timeout) or hasPendingOperations(): + while hasPendingOperations() and runOnce(timeout): discard proc poll*(timeout = 500) = ## Waits for completion events and processes them. Raises ``ValueError`` From 233455a6856e33486c534efc0dd8bb3db1d1e601 Mon Sep 17 00:00:00 2001 From: Ray Imber Date: Fri, 11 Oct 2019 12:24:29 -0700 Subject: [PATCH 6/9] Remove unnecessary change to ioselectors_kqueue.nim found by @cheatfate. --- lib/pure/ioselects/ioselectors_kqueue.nim | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/pure/ioselects/ioselectors_kqueue.nim b/lib/pure/ioselects/ioselectors_kqueue.nim index 6471b76245..65dc0c4961 100644 --- a/lib/pure/ioselects/ioselectors_kqueue.nim +++ b/lib/pure/ioselects/ioselectors_kqueue.nim @@ -375,8 +375,8 @@ proc unregister*[T](s: Selector[T], fd: int|SocketHandle) = "Descriptor [" & $fdi & "] is not registered in the queue!") if pkey.events != {}: - if pkey.events * {Event.Read, Event.Write, Event.User} != {}: - if Event.Read in pkey.events or Event.User in pkey.events: + if pkey.events * {Event.Read, Event.Write} != {}: + if Event.Read in pkey.events: modifyKQueue(s, uint(fdi), EVFILT_READ, EV_DELETE, 0, 0, nil) dec(s.count) if Event.Write in pkey.events: From bef1c4437db3ec2afdcc0180c9eefb87aaf8fba6 Mon Sep 17 00:00:00 2001 From: Ray Imber Date: Wed, 16 Oct 2019 14:13:09 -0700 Subject: [PATCH 7/9] fixes based on code review by @dom96 - For clarity: Changed the unregister if statement to use the in operator instead of the set intersection operator in ioselectors_epoll.nim and ioselectors_select.nim. - Fixed unregister of Event.User case on the Android branch. --- lib/pure/ioselects/ioselectors_epoll.nim | 4 ++-- lib/pure/ioselects/ioselectors_select.nim | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/pure/ioselects/ioselectors_epoll.nim b/lib/pure/ioselects/ioselectors_epoll.nim index da7ef8a9ac..bf13cc83e8 100644 --- a/lib/pure/ioselects/ioselectors_epoll.nim +++ b/lib/pure/ioselects/ioselectors_epoll.nim @@ -197,7 +197,7 @@ proc unregister*[T](s: Selector[T], fd: int|SocketHandle) = "Descriptor $# is not registered in the selector!" % $fdi) if pkey.events != {}: when not defined(android): - if pkey.events * {Event.Read, Event.Write, Event.User} != {}: + if Event.Read in pkey.events or Event.Write in pkey.events or Event.User in pkey.events: var epv = EpollEvent() # TODO: Refactor all these EPOLL_CTL_DEL + dec(s.count) into a proc. if epoll_ctl(s.epollFD, EPOLL_CTL_DEL, fdi.cint, addr epv) != 0: @@ -237,7 +237,7 @@ proc unregister*[T](s: Selector[T], fd: int|SocketHandle) = if posix.close(cint(fdi)) != 0: raiseIOSelectorsError(osLastError()) else: - if pkey.events * {Event.Read, Event.Write} != {}: + if Event.Read in pkey.events or Event.Write in pkey.events or Event.User in pkey.events: var epv = EpollEvent() if epoll_ctl(s.epollFD, EPOLL_CTL_DEL, fdi.cint, addr epv) != 0: raiseIOSelectorsError(osLastError()) diff --git a/lib/pure/ioselects/ioselectors_select.nim b/lib/pure/ioselects/ioselectors_select.nim index 2d53da26b9..b90a01c061 100644 --- a/lib/pure/ioselects/ioselectors_select.nim +++ b/lib/pure/ioselects/ioselectors_select.nim @@ -286,7 +286,7 @@ proc unregister*[T](s: Selector[T], fd: SocketHandle|int) = s.withSelectLock(): let fd = fd.SocketHandle var pkey = s.getKey(fd) - if pkey.events * {Event.Read, Event.User} != {}: + if Event.Read in pkey.events or Event.User in pkey.events: IOFD_CLR(fd, addr s.rSet) dec(s.count) if Event.Write in pkey.events: @@ -462,4 +462,4 @@ template withData*[T](s: Selector[T], fd: SocketHandle|int, value, proc getFd*[T](s: Selector[T]): int = - return -1 \ No newline at end of file + return -1 From bec82254486a72c34bc2c78a8aa841d1eca498fd Mon Sep 17 00:00:00 2001 From: Ray Imber Date: Wed, 16 Oct 2019 19:14:18 -0700 Subject: [PATCH 8/9] Updated the changelog --- changelog.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/changelog.md b/changelog.md index 7381e66975..7433dba9d7 100644 --- a/changelog.md +++ b/changelog.md @@ -7,6 +7,8 @@ ### Breaking changes in the standard library +- `asyncdispatch.drain` now properly takes into account `selector.hasPendingOperations` and only returns once all pending async operations are guaranteed to have completed. +- `asyncdispatch.drain` now consistently uses the passed timeout value for all iterations of the event loop, and not just the first iteration. This is more consistent with the other asyncdispatch apis, and allows `asyncdispatch.drain` to be more efficient. ### Breaking changes in the compiler @@ -43,3 +45,4 @@ ## Bugfixes +- The `FD` variant of `selector.unregister` for `ioselector_epoll` and `ioselector_select` now properly handle the `Event.User` select event type. From 91661c16a1ec209982b99554b8a0af5b32b55d0c Mon Sep 17 00:00:00 2001 From: Ray Imber Date: Sat, 19 Oct 2019 14:30:10 -0700 Subject: [PATCH 9/9] Update changelog.md based on feedback from Dom96 --- changelog.md | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/changelog.md b/changelog.md index 7433dba9d7..7649b846f5 100644 --- a/changelog.md +++ b/changelog.md @@ -7,8 +7,6 @@ ### Breaking changes in the standard library -- `asyncdispatch.drain` now properly takes into account `selector.hasPendingOperations` and only returns once all pending async operations are guaranteed to have completed. -- `asyncdispatch.drain` now consistently uses the passed timeout value for all iterations of the event loop, and not just the first iteration. This is more consistent with the other asyncdispatch apis, and allows `asyncdispatch.drain` to be more efficient. ### Breaking changes in the compiler @@ -24,7 +22,8 @@ ## Library changes - +- `asyncdispatch.drain` now properly takes into account `selector.hasPendingOperations` and only returns once all pending async operations are guaranteed to have completed. +- `asyncdispatch.drain` now consistently uses the passed timeout value for all iterations of the event loop, and not just the first iteration. This is more consistent with the other asyncdispatch apis, and allows `asyncdispatch.drain` to be more efficient. ## Language additions