Fix closeHandle bug, add setFileSize, make resize work on Windows (#21375)

* Add general purpose `setFileSize` (unexported for now).  Use to simplify
`memfiles.open` as well as make robust (via hard allocation, not merely
`ftruncate` address space allocation) on systems with `posix_fallocate`.

As part of this, fix a bad `closeHandle` return check bug on Windows and
add `MemFile.resize` for Windows now that setFileSize makes that easier.

* Adapt existing test to exercise newly portable `MemFile.resize`.

* Since Apple has never provided `posix_fallocate`, provide a fallback.
This is presently written in terms of `ftruncate`, but it can be
improved to use `F_PREALLOCATE` instead, as mentioned in a comment.

lib/posix/posix.nim

@@ -195,7 +195,13 @@ proc open*(a1: cstring, a2: cint, mode: Mode | cint = 0.Mode): cint {.inline.} =
 
 proc posix_fadvise*(a1: cint, a2, a3: Off, a4: cint): cint {.
   importc, header: "<fcntl.h>".}
-proc posix_fallocate*(a1: cint, a2, a3: Off): cint {.
+
+proc ftruncate*(a1: cint, a2: Off): cint {.importc, header: "<unistd.h>".}
+when defined(osx):              # 2001 POSIX evidently does not concern Apple
+  proc posix_fallocate*(a1: cint, a2, a3: Off): cint =
+    ftruncate(a1, a2 + a3)      # Set size to off + len, max offset
+else:                           # TODO: Use fcntl(fd, F_PREALLOCATE, ..) above
+  proc posix_fallocate*(a1: cint, a2, a3: Off): cint {.
     importc, header: "<fcntl.h>".}
 
 when not defined(haiku) and not defined(openbsd):
@@ -511,7 +517,6 @@ proc fpathconf*(a1, a2: cint): int {.importc, header: "<unistd.h>".}
 proc fsync*(a1: cint): cint {.importc, header: "<unistd.h>".}
  ## synchronize a file's buffer cache to the storage device
 
-proc ftruncate*(a1: cint, a2: Off): cint {.importc, header: "<unistd.h>".}
 proc getcwd*(a1: cstring, a2: int): cstring {.importc, header: "<unistd.h>", sideEffect.}
 proc getuid*(): Uid {.importc, header: "<unistd.h>", sideEffect.}
  ## returns the real user ID of the calling process

lib/pure/memfiles.nim

@@ -35,6 +35,31 @@ proc newEIO(msg: string): ref IOError =
   new(result)
   result.msg = msg
 
+proc setFileSize(fh: FileHandle, newFileSize = -1): OSErrorCode =
+  ## Set the size of open file pointed to by `fh` to `newFileSize` if != -1.
+  ## Space is only allocated if that is cheaper than writing to the file.  This
+  ## routine returns the last OSErrorCode found rather than raising to support
+  ## old rollback/clean-up code style. [ Should maybe move to std/osfiles. ]
+  if newFileSize == -1:
+    return
+  when defined(windows):
+    var sizeHigh = int32(newFileSize shr 32)
+    let sizeLow = int32(newFileSize and 0xffffffff)
+    let status = setFilePointer(fh, sizeLow, addr(sizeHigh), FILE_BEGIN)
+    let lastErr = osLastError()
+    if (status == INVALID_SET_FILE_POINTER and lastErr.int32 != NO_ERROR) or
+        setEndOfFile(fh) == 0:
+      result = lastErr
+  else:
+    var e: cint # posix_fallocate truncates up when needed.
+    when declared(posix_fallocate):
+      while (e = posix_fallocate(fh, 0, newFileSize); e == EINTR):
+        discard
+    if e in [EINVAL, EOPNOTSUPP] and ftruncate(fh, newFileSize) == -1:
+      result = osLastError() # fallback arguable; Most portable, but allows SEGV
+    elif e != 0:
+      result = osLastError()
+
 type
   MemFile* = object      ## represents a memory mapped file
     mem*: pointer        ## a pointer to the memory mapped file. The pointer
@@ -182,17 +207,8 @@ proc open*(filename: string, mode: FileMode = fmRead,
     if result.fHandle == INVALID_HANDLE_VALUE:
       fail(osLastError(), "error opening file")
 
-    if newFileSize != -1:
+    if (let e = setFileSize(result.fHandle.FileHandle, newFileSize);
-      var
+        e != 0.OSErrorCode): fail(e, "error setting file size")
-        sizeHigh = int32(newFileSize shr 32)
-        sizeLow = int32(newFileSize and 0xffffffff)
-
-      var status = setFilePointer(result.fHandle, sizeLow, addr(sizeHigh),
-                                  FILE_BEGIN)
-      let lastErr = osLastError()
-      if (status == INVALID_SET_FILE_POINTER and lastErr.int32 != NO_ERROR) or
-          (setEndOfFile(result.fHandle) == 0):
-        fail(lastErr, "error setting file size")
 
     # since the strings are always 'nil', we simply always call
     # CreateFileMappingW which should be slightly faster anyway:
@@ -226,7 +242,7 @@ proc open*(filename: string, mode: FileMode = fmRead,
 
     result.wasOpened = true
     if not allowRemap and result.fHandle != INVALID_HANDLE_VALUE:
-      if closeHandle(result.fHandle) == 0:
+      if closeHandle(result.fHandle) != 0:
         result.fHandle = INVALID_HANDLE_VALUE
 
   else:
@@ -249,9 +265,8 @@ proc open*(filename: string, mode: FileMode = fmRead,
       # Is there an exception that wraps it?
       fail(osLastError(), "error opening file")
 
-    if newFileSize != -1:
+    if (let e = setFileSize(result.handle.FileHandle, newFileSize);
-      if ftruncate(result.handle, newFileSize) == -1:
+        e != 0.OSErrorCode): fail(e, "error setting file size")
-        fail(osLastError(), "error setting file size")
 
     if mappedSize != -1:
       result.size = mappedSize
@@ -306,23 +321,47 @@ proc flush*(f: var MemFile; attempts: Natural = 3) =
       if lastErr != EBUSY.OSErrorCode:
         raiseOSError(lastErr, "error flushing mapping")
 
-when defined(posix) or defined(nimdoc):
+proc resize*(f: var MemFile, newFileSize: int) {.raises: [IOError, OSError].} =
-  proc resize*(f: var MemFile, newFileSize: int) {.raises: [IOError, OSError].} =
+  ## Resize & re-map the file underlying an `allowRemap MemFile`.  If the OS/FS
-    ## resize and re-map the file underlying an `allowRemap MemFile`.
+  ## supports it, file space is reserved to ensure room for new virtual pages.
-    ## **Note**: this assumes the entire file is mapped read-write at offset zero.
+  ## Caller should wait often enough for `flush` to finish to limit use of
+  ## system RAM for write buffering, perhaps just prior to this call.
+  ## **Note**: this assumes the entire file is mapped read-write at offset 0.
   ## Also, the value of `.mem` will probably change.
-    ## **Note**: This is not (yet) available on Windows.
+  if newFileSize < 1: # Q: include system/bitmasks & use PageSize ?
-    when defined(posix):
+    raise newException(IOError, "Cannot resize MemFile to < 1 byte")
+  when defined(windows):
+    if not f.wasOpened:
+      raise newException(IOError, "Cannot resize unopened MemFile")
+    if f.fHandle == INVALID_HANDLE_VALUE:
+      raise newException(IOError,
+                         "Cannot resize MemFile opened with allowRemap=false")
+    if unmapViewOfFile(f.mem) == 0 or closeHandle(f.mapHandle) == 0: # Un-do map
+      raiseOSError(osLastError())
+    if newFileSize != f.size: # Seek to size & `setEndOfFile` => allocated.
+      if (let e = setFileSize(f.fHandle.FileHandle, newFileSize);
+          e != 0.OSErrorCode): raiseOSError(e)
+    f.mapHandle = createFileMappingW(f.fHandle, nil, PAGE_READWRITE, 0,0,nil)
+    if f.mapHandle == 0:                                             # Re-do map
+      raiseOSError(osLastError())
+    if (let m = mapViewOfFileEx(f.mapHandle, FILE_MAP_READ or FILE_MAP_WRITE,
+                                0, 0, WinSizeT(newFileSize), nil); m != nil):
+      f.mem  = m
+      f.size = newFileSize
+    else:
+      raiseOSError(osLastError())
+  elif defined(posix):
     if f.handle == -1:
       raise newException(IOError,
                          "Cannot resize MemFile opened with allowRemap=false")
-      if ftruncate(f.handle, newFileSize) == -1:
+    if newFileSize != f.size:
-        raiseOSError(osLastError())
+      if (let e = setFileSize(f.handle.FileHandle, newFileSize);
+          e != 0.OSErrorCode): raiseOSError(e)
     when defined(linux): #Maybe NetBSD, too?
-        #On Linux this can be over 100 times faster than a munmap,mmap cycle.
+      # On Linux this can be over 100 times faster than a munmap,mmap cycle.
       proc mremap(old: pointer; oldSize, newSize: csize_t; flags: cint):
           pointer {.importc: "mremap", header: "<sys/mman.h>".}
-        let newAddr = mremap(f.mem, csize_t(f.size), csize_t(newFileSize), cint(1))
+      let newAddr = mremap(f.mem, csize_t(f.size), csize_t(newFileSize), 1.cint)
       if newAddr == cast[pointer](MAP_FAILED):
         raiseOSError(osLastError())
     else:

tests/stdlib/tmemfiles2.nim

@@ -15,8 +15,9 @@ var
 
 if fileExists(fn): removeFile(fn)
 
-# Create a new file, data all zeros
+# Create a new file, data all zeros, starting at size 10
-mm = memfiles.open(fn, mode = fmReadWrite, newFileSize = 20)
+mm = memfiles.open(fn, mode = fmReadWrite, newFileSize = 10, allowRemap=true)
+mm.resize 20  # resize up to 20
 mm.close()
 
 # read, change