Merge pull request #37 from alaviss/handles-zero-invalid

handles: change storage strategy and conform API

Since nim-lang/Nim#19139 was closed as a
"won't fix", the "workaround" has to become permanent.

If we are to store a boolean next to the FD, it would double the
structure size, which is undesirable.

Instead, storage is now done via a lossless map so that zero-ed values
are invalid by default.

In this commit, Handle[T] API is changed to match other parts of the
project. In short:

    get -> fd
    take -> takeFd

src/sys/handles.nim

@@ -125,12 +125,29 @@ when false:
 
 # XXX: T should be a concept, but those don't work atm
 type
-  Handle*[T] {.requiresInit.} = object
+  Handle*[T] = object
     ## An object used to associate a handle with a lifetime.
-    # Walkaround for nim-lang/Nim#16607
-    when true or not defined(release):
-      initialized: bool
-    fd: T
+    shiftedFd: FDImpl ## The file descriptor mapped to a different range that
+                      ## is invalid when zero-ed.
+
+func fd*[T](h: Handle[T]): T {.inline.} =
+  ## Returns the resource handle held by `h`.
+  ##
+  ## The returned handle will stay valid for the duration of `h`.
+  ##
+  ## **Note**: Do **not** close the returned handle. If ownership is wanted,
+  ## use `takeFd` instead.
+  getFdImpl()
+
+func `fd=`[T](h: var Handle[T], fd: T) {.inline.} =
+  ## Set the handle to be tracked. This does not destroy the previous handle.
+  setFdImpl()
+
+func takeFd*[T](h: var Handle[T]): T {.inline.} =
+  ## Returns the resource handle held by `h` and release ownership to the
+  ## caller. `h` will then be invalidated.
+  result = h.fd
+  `fd=`(h, InvalidFD.T)
 
 proc close*[T](h: var Handle[T]) {.inline.} =
   ## Close the handle owned by `h` and invalidating it.
@@ -140,18 +157,10 @@ proc close*[T](h: var Handle[T]) {.inline.} =
     close h.fd
   finally:
     # Always invalidate `h.fd` to avoid double-close on destruction.
-    h.fd = InvalidFD.T
+    `fd=`(h, InvalidFD.T)
 
 proc `=destroy`[T](h: var Handle[T]) =
   ## Destroy the file handle.
-  when false:
-    # TODO: Once nim-lang/Nim#16607 is fixed, make this into a debug check
-    assert h.initialized, "Handle was not initialized before destruction, this is a compiler bug."
-  else:
-    # Walkaround for nim-lang/Nim#16607
-    if not h.initialized:
-      return
-
   if h.fd != InvalidFD:
     close h
 
@@ -163,31 +172,11 @@ proc `=copy`*[T](dest: var Handle[T], src: Handle[T]) {.error.}
 proc initHandle*[T](fd: T): Handle[T] {.inline.} =
   ## Creates a `Handle` owning the passed `fd`. The `fd` shall then be freed
   ## automatically when the `Handle` go out of scope.
-  when false:
-    Handle[T](fd: fd)
-  else:
-    Handle[T](initialized: true, fd: fd)
+  `fd=`(result, fd)
 
 proc newHandle*[T](fd: T): ref Handle[T] {.inline.} =
   ## Creates a `ref Handle` owning the passed `fd`. The `fd` shall then be
   ## freed automatically when the returned `ref Handle[T]` is collected by the
   ## GC.
-  when false:
-    (ref Handle[T])(fd: fd)
-  else:
-    (ref Handle[T])(initialized: true, fd: fd)
-
-proc get*[T](h: Handle[T]): T {.inline.} =
-  ## Returns the resource handle held by `h`.
-  ##
-  ## The returned handle will stay valid for the duration of `h`.
-  ##
-  ## **Note**: Do **not** close the returned handle. If ownership is wanted,
-  ## use `take` instead.
-  h.fd
-
-proc take*[T](h: var Handle[T]): T {.inline.} =
-  ## Returns the resource handle held by `h` and release ownership to the
-  ## caller. `h` will then be invalidated.
-  result = h.fd
-  h.fd = InvalidFD.T
+  new result
+  result[] = initHandle(fd)

src/sys/ioqueue.nim

@@ -191,6 +191,6 @@ proc unregister*(fd: AnyFD) =
   ##   *after* it and its duplicates are closed.
   unregisterImpl()
 
-proc unregister*(fd: Handle[AnyFD]) =
+proc unregister*(handle: Handle[AnyFD]) =
   ## An overload of `unregister` for `Handle`
-  ioqueue.unregister(fd.get)
+  ioqueue.unregister(handle.fd)

src/sys/ioqueue/iocp.nim

@@ -102,7 +102,7 @@ proc poll(runnable: var seq[Continuation], timeout = none(Duration)) {.used.} =
   # Obtain completion events
   var selected: ULONG
   if GetQueuedCompletionStatusEx(
-    wincore.Handle(eq.iocp.get), addr eq.eventBuffer[0], ULONG(eq.eventBuffer.len),
+    wincore.Handle(eq.iocp.fd), addr eq.eventBuffer[0], ULONG(eq.eventBuffer.len),
     addr selected, timeout, wincore.FALSE
   ) == wincore.FALSE:
     let errorCode = GetLastError()
@@ -150,7 +150,7 @@ proc persist(fd: AnyFD) {.raises: [OSError].} =
 
   # Register the handle with IOCP
   if CreateIoCompletionPort(
-    wincore.Handle(fd), wincore.Handle(eq.iocp.get), ULongPtr(fd), 0
+    wincore.Handle(fd), wincore.Handle(eq.iocp.fd), ULongPtr(fd), 0
   ) == wincore.Handle(0):
     let errorCode = GetLastError()
 

src/sys/private/files_posix.nim

@@ -31,11 +31,11 @@ template newAsyncFileImpl() {.dirty.} =
   result = AsyncFile newFile(fd)
 
 template getFDImpl() {.dirty.} =
-  result = get f.handle
+  result = f.handle.fd
 
 template takeFDImpl() {.dirty.} =
   cleanupFile f
-  result = take f.handle
+  result = f.handle.takeFd
 
 proc commonRead(fd: FD, buf: pointer, len: Natural): int {.inline.} =
   ## A wrapper around posix.read() to retry on EINTR.

src/sys/private/files_windows.nim

@@ -46,11 +46,11 @@ template newAsyncFileImpl() {.dirty.} =
   result[] = AsyncFileImpl fileImpl
 
 template getFDImpl() {.dirty.} =
-  result = get f.handle
+  result = f.handle.fd
 
 template takeFDImpl() {.dirty.} =
   cleanupFile f
-  result = take f.handle
+  result = f.handle.takeFd
 
 proc initOverlapped(f: FileImpl, o: var Overlapped) {.inline.} =
   ## Initialize an overlapped object for I/O operations on `f`

src/sys/private/handles_posix.nim

@@ -43,6 +43,17 @@ template setBlockingImpl() {.dirty.} =
     flags = flags or O_NONBLOCK
   posixChk fcntl(fd.cint, F_SETFL, flags), ErrorSetBlocking
 
+template getFdImpl() {.dirty.} =
+  cast[T](cast[cuint](h.shiftedFd) - 1)
+
+template setFdImpl() {.dirty.} =
+  # The FD is casted to unsigned so that `high(cint)` can be mapped forward
+  # correctly.
+  #
+  # As all architectures running Linux uses two's complement, this also handles
+  # InvalidFD -> 0 correctly.
+  h.shiftedFd = cast[FDImpl](cast[cuint](fd) + 1)
+
 when false:
   # NOTE: Staged until process spawning is added.
   template duplicateImpl() {.dirty.} =

src/sys/private/handles_windows.nim

@@ -34,3 +34,9 @@ template setInheritableImpl() {.dirty.} =
       raise newOSError(GetLastError(), ErrorSetInheritable)
   else:
     {.error: "setInheritable is not available for this variant of FD".}
+
+template getFdImpl() {.dirty.} =
+  cast[T](cast[uint](h.shiftedFd - 1))
+
+template setFdImpl() {.dirty.} =
+  h.shiftedFd = cast[FDImpl](cast[uint](fd) + 1)

src/sys/private/ioqueue_bsd.nim

@@ -90,7 +90,7 @@ proc queue(eq: var EventQueueImpl, cont: Continuation, fd: FD, event: ReadyEvent
     flags: EvAdd or EvEnable or EvDispatch
   )
 
-  posixChk eq.kqueue.get.kevent(changeList = [kevent]), $Error.Queue
+  posixChk eq.kqueue.fd.kevent(changeList = [kevent]), $Error.Queue
   eq.waiters[fd] = Waiter(cont: cont, event: event)
 
 func toTimespec(d: Duration): Timespec =
@@ -109,9 +109,9 @@ template pollImpl() {.dirty.} =
   # Obtain events from kevent
   let nevents =
     if timeout.isNone:
-      eq.kqueue.get.kevent(eventList = eq.eventBuffer)
+      eq.kqueue.fd.kevent(eventList = eq.eventBuffer)
     else:
-      eq.kqueue.get.kevent(
+      eq.kqueue.fd.kevent(
         eventList = eq.eventBuffer,
         timeout = toTimespec(timeout.get)
       )
@@ -155,7 +155,7 @@ template unregisterImpl() {.dirty.} =
   # If the FD is in the waiter list
   if fd in eq.waiters:
     # Deregister it from kqueue
-    let status = eq.kqueue.get.kevent([
+    let status = eq.kqueue.fd.kevent([
       # kqueue map events using a tuple of filter & identifier, so we
       # need to reproduce both to delete the event that we want.
       Kevent(ident: Ident(fd), filter: toFilter(eq.waiters[fd].event),

src/sys/private/ioqueue_linux.nim

@@ -79,7 +79,7 @@ func toEvents(ev: Ev): set[Event] =
 proc queue(eq: var EventQueueImpl, cont: Continuation, fd: AnyFD, event: ReadyEvent) =
   var epEvent = epoll.Event(events: toEv({event}), data: Data(fd: fd.cint))
   # If adding `fd` to epoll fails
-  if eq.epoll.get.ctl(CtlAdd, fd, epEvent) == -1:
+  if eq.epoll.fd.ctl(CtlAdd, fd, epEvent) == -1:
     # In case `fd` was already registered
     if errno == EEXIST:
       # If there is a waiter in the queue
@@ -88,7 +88,7 @@ proc queue(eq: var EventQueueImpl, cont: Continuation, fd: AnyFD, event: ReadyEv
         raise newException(ValueError, $Error.QueuedFD % $fd.cint)
 
       # Otherwise re-register our event
-      posixChk eq.epoll.get.ctl(CtlMod, fd, epEvent), $Error.Queue
+      posixChk eq.epoll.fd.ctl(CtlMod, fd, epEvent), $Error.Queue
     else:
       posixChk -1, $Error.Queue
 
@@ -120,7 +120,7 @@ template pollImpl() {.dirty.} =
   eq.eventBuffer.setLen eq.waiters.len
 
   # Obtain the events that are ready
-  let selected = eq.epoll.get.wait(eq.eventBuffer, timeout)
+  let selected = eq.epoll.fd.wait(eq.eventBuffer, timeout)
   posixChk selected, $Error.Poll
   # Set the length of the buffer to the amount of events received
   eq.eventBuffer.setLen selected
@@ -161,7 +161,7 @@ template unregisterImpl() {.dirty.} =
   # If the FD is in the waiter list
   if fd in eq.waiters:
     # Deregister it from epoll
-    let status = eq.epoll.get.ctl(CtlDel, fd, nil)
+    let status = eq.epoll.fd.ctl(CtlDel, fd, nil)
     if status == -1:
       # If the FD is in the waiter list but epoll said that it's never
       # registered or that it's invalid, then its already closed before being

tests/handles/tdestroy.nim

@@ -33,25 +33,28 @@ suite "Test close() and Handle[T] destructions":
     expectDefect:
       close(SocketFD InvalidFD)
 
-  test "Handle[T] not destroyed before construction":
-    skip("nim-lang/Nim#16607")
+  test "Handle[T] initializes to invalid":
+    var handle: Handle[FD]
 
-    when not compileOption("assertions"):
-      skip("assertions are disabled")
+    check handle.fd == InvalidFD
 
-    proc pair(): tuple[a, b: Handle[FD]] =
-      result.a = initHandle(FD InvalidFD)
-      result.b = initHandle(FD InvalidFD)
+  test "Handle[T] can store the highest handle possible correctly":
+    const MaxFD =
+      when defined(posix):
+        FD(high cint)
+      elif defined(windows):
+        cast[FD](high uint)
+      else:
+        {.error: "A maximum FD is not defined for this platform".}
 
-    discard pair()
+    var handle = initHandle(MaxFD)
 
-    proc pairRef(): tuple[a, b: ref Handle[FD]] =
-      result.a = newHandle(FD InvalidFD)
-      result.b = newHandle(FD InvalidFD)
-
-    discard pairRef()
-    when declared(GC_fullCollect):
-      GC_fullCollect()
+    try:
+      check handle.fd == MaxFD
+    finally:
+      # This handle is also invalid, so remove it to prevent Handle[T] from
+      # trying to close it.
+      discard handle.takeFd()
 
   var
     rd = FD InvalidFD
@@ -92,4 +95,4 @@ suite "Test close() and Handle[T] destructions":
     expectDefect:
       close hrd
 
-    check hrd.get == InvalidFD
+    check hrd.fd == InvalidFD

tests/ioqueue/asyncio.nim

@@ -58,7 +58,7 @@ proc write*[T: byte or char](fd: Handle[FD], data: openArray[T]) {.sync.} =
   ## Write all bytes in `data` into `fd`.
   var totalWritten = 0
   while totalWritten < data.len:
-    let written = write(fd.get.cint, data[totalWritten].unsafeAddr, data.len - totalWritten)
+    let written = write(fd.fd.cint, data[totalWritten].unsafeAddr, data.len - totalWritten)
     case written
     of -1:
       raise newIOError(totalWritten, errno.int32)
@@ -69,7 +69,7 @@ proc read*[T: byte or char](fd: Handle[FD], buf: var openArray[T]): int {.sync.}
   ## Read all bytes from `fd` into `buf` until it's filled or there is
   ## nothing left to read
   while result < buf.len:
-    let readBytes = read(fd.get.cint, buf[result].addr, buf.len - result)
+    let readBytes = read(fd.fd.cint, buf[result].addr, buf.len - result)
     case readBytes
     of -1:
       raise newIOError(result, errno.int32)
@@ -87,14 +87,14 @@ proc readAsync*(rd: ref Handle[FD], buf: ref string) {.asyncio.} =
   ## information outside of cps yet
   when defined(windows):
     let overlapped = new Overlapped
-    persist(rd.get)
+    persist(rd.fd)
     if ReadFile(
-      winim.Handle(rd.get), addr buf[0], DWORD(buf.len), nil,
+      winim.Handle(rd.fd), addr buf[0], DWORD(buf.len), nil,
       addr overlapped[]
     ) == winim.FALSE:
       let errorCode = GetLastError()
       if errorCode == ErrorIoPending:
-        wait(rd.get, overlapped)
+        wait(rd.fd, overlapped)
       else:
         discard "Error handling below"
     let errorCode = DWORD(overlapped.Internal)
@@ -115,22 +115,22 @@ proc readAsync*(rd: ref Handle[FD], buf: ref string) {.asyncio.} =
         e.bytesTransferred += offset
         if e.errorCode == EAGAIN:
           offset = e.bytesTransferred
-          wait rd.get, Read
+          wait rd.fd, Read
         else:
           raise e
 
 proc writeAsync*(wr: ref Handle[FD], buf: string) {.asyncio.} =
   ## Write all bytes in `buf` into `wr` asynchronously
   when defined(windows):
     let overlapped = new Overlapped
-    persist(wr.get)
+    persist(wr.fd)
     if WriteFile(
-      winim.Handle(wr.get), unsafeAddr buf[0], DWORD(buf.len), nil,
+      winim.Handle(wr.fd), unsafeAddr buf[0], DWORD(buf.len), nil,
       addr overlapped[]
     ) == winim.FALSE:
       let errorCode = GetLastError()
       if errorCode == ErrorIoPending:
-        wait(wr.get, overlapped)
+        wait(wr.fd, overlapped)
       else:
         discard "Error handling below"
     let errorCode = DWORD(overlapped.Internal)
@@ -148,6 +148,6 @@ proc writeAsync*(wr: ref Handle[FD], buf: string) {.asyncio.} =
         e.bytesTransferred += offset
         if e.errorCode == EAGAIN:
           offset = e.bytesTransferred
-          wait wr.get, Write
+          wait wr.fd, Write
         else:
           raise e