Fixes incorrect fd==0 test on Unix; Conserves handles by default. (#5512)

* Fix 2 problems.  First, 0 is a valid fd on Unix (easily gotten if user first
closes all fds and then starts using memfiles).  Use -1 instead for an invalid
fd.  Second, it is best practice to conserve open fds on Unix and file handles
on Windows.  These handles are not needed unless the user wants to remap the
memory with ``mapMem`` (or a hypothetical future ``proc resize``).  Adding a
new bool param ``allowRemap=false`` to ``memfiles.open`` solves this cleanly
in a "mostly" backward compatible way.  This is only "mostly" because the
default ``false`` case does not keep unneeded resources allocated, but that
most sensible default means that any ``mapMem`` callers need to fix all their
open calls to have allowRemap=true, as this PR also does for tmemfiles2.nim.
* Include backwards compatibility note.

3 files changed, 24 insertions, 9 deletions

diff --git a/lib/pure/memfiles.nim b/lib/pure/memfiles.nim
index c6322c7bb..b6154d8de 100644
--- a/lib/pure/memfiles.nim
+++ b/lib/pure/memfiles.nim
@@ -83,7 +83,8 @@ proc unmapMem*(f: var MemFile, p: pointer, size: int) =
 
 
 proc open*(filename: string, mode: FileMode = fmRead,
-           mappedSize = -1, offset = 0, newFileSize = -1): MemFile =
+           mappedSize = -1, offset = 0, newFileSize = -1,
+           allowRemap = false): MemFile =
   ## opens a memory mapped file. If this fails, ``EOS`` is raised.
   ##
   ## ``newFileSize`` can only be set if the file does not exist and is opened
@@ -95,6 +96,9 @@ proc open*(filename: string, mode: FileMode = fmRead,
   ## ``offset`` must be multiples of the PAGE SIZE of your OS
   ## (usually 4K or 8K but is unique to your OS)
   ##
+  ## ``allowRemap`` only needs to be true if you want to call ``mapMem`` on
+  ## the resulting MemFile; else file handles are not kept open.
+  ##
   ## Example:
   ##
   ## .. code-block:: nim
@@ -189,11 +193,14 @@ proc open*(filename: string, mode: FileMode = fmRead,
       else: result.size = fileSize.int
 
     result.wasOpened = true
+    if not allowRemap and result.fHandle != INVALID_HANDLE_VALUE:
+      if closeHandle(result.fHandle) == 0:
+        result.fHandle = INVALID_HANDLE_VALUE
 
   else:
     template fail(errCode: OSErrorCode, msg: expr) =
       rollback()
-      if result.handle != 0: discard close(result.handle)
+      if result.handle != -1: discard close(result.handle)
       raiseOSError(errCode)
 
     var flags = if readonly: O_RDONLY else: O_RDWR
@@ -236,6 +243,10 @@ proc open*(filename: string, mode: FileMode = fmRead,
     if result.mem == cast[pointer](MAP_FAILED):
       fail(osLastError(), "file mapping failed")
 
+    if not allowRemap and result.handle != -1:
+      if close(result.handle) == 0:
+        result.handle = -1
+
 proc close*(f: var MemFile) =
   ## closes the memory mapped file `f`. All changes are written back to the
   ## file system, if `f` was opened with write access.
@@ -244,15 +255,16 @@ proc close*(f: var MemFile) =
   var lastErr: OSErrorCode
 
   when defined(windows):
-    if f.fHandle != INVALID_HANDLE_VALUE and f.wasOpened:
+    if f.wasOpened:
       error = unmapViewOfFile(f.mem) == 0
       lastErr = osLastError()
       error = (closeHandle(f.mapHandle) == 0) or error
-      error = (closeHandle(f.fHandle) == 0) or error
+      if f.fHandle != INVALID_HANDLE_VALUE:
+        error = (closeHandle(f.fHandle) == 0) or error
   else:
-    if f.handle != 0:
-      error = munmap(f.mem, f.size) != 0
-      lastErr = osLastError()
+    error = munmap(f.mem, f.size) != 0
+    lastErr = osLastError()
+    if f.handle != -1:
       error = (close(f.handle) != 0) or error
 
   f.size = 0
@@ -263,7 +275,7 @@ proc close*(f: var MemFile) =
     f.mapHandle = 0
     f.wasOpened = false
   else:
-    f.handle = 0
+    f.handle = -1
 
   if error: raiseOSError(lastErr)
 
diff --git a/tests/stdlib/tmemfiles2.nim b/tests/stdlib/tmemfiles2.nim
index 026443e93..665e92e8a 100644
--- a/tests/stdlib/tmemfiles2.nim
+++ b/tests/stdlib/tmemfiles2.nim
@@ -18,7 +18,7 @@ mm = memfiles.open(fn, mode = fmReadWrite, newFileSize = 20)
 mm.close()
 
 # read, change
-mm_full = memfiles.open(fn, mode = fmWrite, mappedSize = -1)
+mm_full = memfiles.open(fn, mode = fmWrite, mappedSize = -1, allowRemap = true)
 echo "Full read size: ",mm_full.size
 p = mm_full.mapMem(fmReadWrite, 20, 0)
 var p2 = cast[cstring](p)
diff --git a/web/news/e031_version_0_16_2.rst b/web/news/e031_version_0_16_2.rst
index 3f111b503..802478090 100644
--- a/web/news/e031_version_0_16_2.rst
+++ b/web/news/e031_version_0_16_2.rst
@@ -23,6 +23,9 @@ Changes affecting backwards compatibility
   pointer. Now the hash is calculated from the contents of the string, assuming
   ``cstring`` is a null-terminated string. Equal ``string`` and ``cstring``
   values produce an equal hash value.
+- ``memfiles.open`` now closes file handleds/fds by default.  Passing
+  ``allowRemap=true`` to ``memfiles.open`` recovers the old behavior.  The old
+  behavior is only needed to call ``mapMem`` on the resulting ``MemFile``.
 
 Library Additions
 -----------------