File locks now work on Windows (#480)

* File locks now work on Windows

Uses LockFileEx() and UnlockFileEx(). Fixes HDFFV-10191 (partial).

* Committing clang-format changes

* Committing clang-format changes

* Fixes commenting in h5repack

* Reworks H5Fis_accessible()

H5Fis_accessible() created a new file handle and attempted to read
through it, which will fail when a file has been opened with an
exclusive lock on operating systems that have mandatory locks.

This change uses the same scheme we use in H5Fopen() to check if
the file is already open and only tries to read the file signature
if the file has not already been opened.

Also adds a test for this behavior.

* Committing clang-format changes

* Trivial change to force github to run actions

Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>

release_docs/RELEASE.txt

@@ -384,6 +384,23 @@ New Features
 
     Library:
     --------
+    - File locking now works on Windows
+
+      Since version 1.10.0, the HDF5 library has used a file locking scheme
+      to help enforce one reader at a time accessing an HDF5 file, which can
+      be helpful when setting up readers and writers to use the single-
+      writer/multiple-readers (SWMR) access pattern.
+
+      In the past, this was only functional on POSIX systems where flock() or
+      fcntl() were present. Windows used a no-op stub that always succeeded.
+
+      HDF5 now uses LockFileEx() and UnlockFileEx() to lock the file using the
+      same scheme as POSIX systems. We lock the entire file when we set up the
+      locks (by passing DWORDMAX as both size parameters to LockFileEx()).
+
+        (DER - 2021/03/19, HDFFV-10191)
+
+
     - H5Epush_ret() now requires a trailing semicolon
 
         H5Epush_ret() is a function-like macro that has been changed to

src/H5Fint.c

@@ -1056,9 +1056,10 @@ done:
 htri_t
 H5F__is_hdf5(const char *name, hid_t fapl_id)
 {
-    H5FD_t *file      = NULL;        /* Low-level file struct                    */
+    H5FD_t *      file      = NULL;        /* Low-level file struct            */
-    haddr_t sig_addr  = HADDR_UNDEF; /* Addess of hdf5 file signature            */
+    H5F_shared_t *shared    = NULL;        /* Shared part of file              */
-    htri_t  ret_value = FAIL;        /* Return value                             */
+    haddr_t       sig_addr  = HADDR_UNDEF; /* Addess of hdf5 file signature    */
+    htri_t        ret_value = FAIL;        /* Return value                     */
 
     FUNC_ENTER_PACKAGE
 
@@ -1069,10 +1070,20 @@ H5F__is_hdf5(const char *name, hid_t fapl_id)
     if (NULL == (file = H5FD_open(name, H5F_ACC_RDONLY, fapl_id, HADDR_UNDEF)))
         HGOTO_ERROR(H5E_FILE, H5E_CANTINIT, FAIL, "unable to open file")
 
-    /* The file is an hdf5 file if the hdf5 file signature can be found */
+    /* If the file is already open, it's an HDF5 file
-    if (H5FD_locate_signature(file, &sig_addr) < 0)
+     *
-        HGOTO_ERROR(H5E_FILE, H5E_NOTHDF5, FAIL, "error while trying to locate file signature")
+     * If the file is open with an exclusive lock on an operating system that enforces
-    ret_value = (HADDR_UNDEF != sig_addr);
+     * mandatory file locks (like Windows), creating a new file handle and attempting
+     * to read through it will fail so we have to try this first.
+     */
+    if ((shared = H5F__sfile_search(file)) != NULL)
+        ret_value = TRUE;
+    else {
+        /* The file is an HDF5 file if the HDF5 file signature can be found */
+        if (H5FD_locate_signature(file, &sig_addr) < 0)
+            HGOTO_ERROR(H5E_FILE, H5E_NOTHDF5, FAIL, "error while trying to locate file signature")
+        ret_value = (HADDR_UNDEF != sig_addr);
+    }
 
 done:
     /* Close the file */
@@ -1782,7 +1793,7 @@ H5F_open(const char *name, unsigned flags, hid_t fcpl_id, hid_t fapl_id)
     FUNC_ENTER_NOAPI(NULL)
 
     /*
-     * If the driver has a `cmp' method then the driver is capable of
+     * If the driver has a 'cmp' method then the driver is capable of
      * determining when two file handles refer to the same file and the
      * library can insure that when the application opens a file twice
      * that the two handles coordinate their operations appropriately.
@@ -3718,10 +3729,17 @@ H5F__start_swmr_write(H5F_t *f)
     setup = TRUE;
 
     /* Place an advisory lock on the file */
-    if (H5F_USE_FILE_LOCKING(f))
+    if (H5F_USE_FILE_LOCKING(f)) {
+        /* Have to unlock on Windows as Win32 doesn't support changing the lock
+         * type (exclusive vs shared) with a second call.
+         */
+        if (H5FD_unlock(f->shared->lf) < 0) {
+            HGOTO_ERROR(H5E_FILE, H5E_CANTUNLOCKFILE, FAIL, "unable to unlock the file")
+        }
         if (H5FD_lock(f->shared->lf, TRUE) < 0) {
             HGOTO_ERROR(H5E_FILE, H5E_CANTLOCKFILE, FAIL, "unable to lock the file")
         }
+    }
 
     /* Mark superblock as dirty */
     if (H5F_super_dirty(f) < 0)

src/H5system.c

@@ -618,40 +618,43 @@ c99_vsnprintf(char *str, size_t size, const char *format, va_list ap)
  *-------------------------------------------------------------------------
  */
 int
-Wflock(int H5_ATTR_UNUSED fd, int H5_ATTR_UNUSED operation)
+Wflock(int fd, int operation)
 {
 
-/* This is a no-op while we implement a Win32 VFD */
+    HANDLE hFile;
-#if 0
+    DWORD  dwFlags    = LOCKFILE_FAIL_IMMEDIATELY;
-int
+    DWORD  dwReserved = 0;
-Wflock(int fd, int operation) {
+    /* MAXDWORD locks the entire file */
-
+    DWORD nNumberOfBytesToLockLow  = MAXDWORD;
-    HANDLE          hFile;
+    DWORD nNumberOfBytesToLockHigh = MAXDWORD;
-    DWORD           dwFlags = LOCKFILE_FAIL_IMMEDIATELY;
+    /* Must initialize OVERLAPPED struct */
-    DWORD           dwReserved = 0;
+    OVERLAPPED overlapped = {0};
-                    /* MAXDWORD for entire file */
-    DWORD           nNumberOfBytesToLockLow = MAXDWORD;
-    DWORD           nNumberOfBytesToLockHigh = MAXDWORD;
-                    /* Must initialize OVERLAPPED struct */
-    OVERLAPPED      overlapped = {0};
 
     /* Get Windows HANDLE */
-    hFile = _get_osfhandle(fd);
+    if (INVALID_HANDLE_VALUE == (hFile = (HANDLE)_get_osfhandle(fd)))
+        return -1;
 
     /* Convert to Windows flags */
-    if(operation & LOCK_EX)
+    if (operation & LOCK_EX)
         dwFlags |= LOCKFILE_EXCLUSIVE_LOCK;
 
     /* Lock or unlock */
-    if(operation & LOCK_UN)
+    if (operation & LOCK_UN) {
-        if(0 == UnlockFileEx(hFile, dwReserved, nNumberOfBytesToLockLow,
+        if (0 ==
-                            nNumberOfBytesToLockHigh, &overlapped))
+            UnlockFileEx(hFile, dwReserved, nNumberOfBytesToLockLow, nNumberOfBytesToLockHigh, &overlapped)) {
+            /* Attempting to unlock an already unlocked file will fail and this can happen
+             * in H5Fstart_swmr_write(). For now, just ignore the "error" (error code: 0x9e / 158).
+             */
+            if (GetLastError() != 158)
+                return -1;
+        }
+    }
+    else {
+        if (0 == LockFileEx(hFile, dwFlags, dwReserved, nNumberOfBytesToLockLow, nNumberOfBytesToLockHigh,
+                            &overlapped))
             return -1;
-    else
+    }
-        if(0 == LockFileEx(hFile, dwFlags, dwReserved, nNumberOfBytesToLockLow,
+
-                            nNumberOfBytesToLockHigh, &overlapped))
-            return -1;
-#endif /* 0 */
     return 0;
 } /* end Wflock() */
 

test/tfile.c

@@ -1666,6 +1666,29 @@ test_file_is_accessible(const char *env_h5_drvr)
     is_hdf5 = H5Fis_accessible(filename, fapl_id);
     VERIFY(is_hdf5, TRUE, "H5Fis_accessible");
 
+    /*****************************************/
+    /* Newly created file that is still open */
+    /*****************************************/
+
+    /* On Windows, file locking is mandatory so this check ensures that
+     * H5Fis_accessible() works on files that have an exclusive lock.
+     * Previous versions of this API call created an additional file handle
+     * and attempted to read through it, which will not work when locks
+     * are enforced by the OS.
+     */
+
+    /* Create a file and hold it open */
+    fid = H5Fcreate(filename, H5F_ACC_TRUNC, H5P_DEFAULT, fapl_id);
+    CHECK(fid, H5I_INVALID_HID, "H5Fcreate");
+
+    /* Verify that the file is an HDF5 file */
+    is_hdf5 = H5Fis_accessible(filename, fapl_id);
+    VERIFY(is_hdf5, TRUE, "H5Fis_accessible");
+
+    /* Close file */
+    ret = H5Fclose(fid);
+    CHECK(ret, FAIL, "H5Fclose");
+
     /*******************************/
     /* Non-default user block size */
     /*******************************/

tools/src/h5repack/h5repack_copy.c

@@ -304,14 +304,6 @@ copy_objects(const char *fnamein, const char *fnameout, pack_opt_t *options)
     if ((fidout = H5Fcreate(fnameout, H5F_ACC_TRUNC, fcpl, options->fout_fapl)) < 0)
         H5TOOLS_GOTO_ERROR((-1), "H5Fcreate could not create file <%s>:", fnameout);
 
-    /*-------------------------------------------------------------------------
-     * write a new user block if requested
-     *-------------------------------------------------------------------------
-     */
-    if (options->ublock_size > 0)
-        if (copy_user_block(options->ublock_filename, fnameout, options->ublock_size) < 0)
-            H5TOOLS_GOTO_ERROR((-1), "Could not copy user block. Exiting...");
-
     /*-------------------------------------------------------------------------
      * get list of objects
      *-------------------------------------------------------------------------
@@ -346,27 +338,60 @@ copy_objects(const char *fnamein, const char *fnameout, pack_opt_t *options)
     }
 
     /*-------------------------------------------------------------------------
-     * write only the input file user block if there is no user block file input
+     * Close the file and everything in it so the lock is removed
+     *-------------------------------------------------------------------------
+     */
+    if (H5Pclose(fcpl) < 0)
+        H5TOOLS_GOTO_ERROR((-1), "could not close fcpl");
+    if (H5Pclose(options->fout_fapl) < 0)
+        H5TOOLS_GOTO_ERROR((-1), "could not close fcpl");
+    options->fout_fapl = H5P_DEFAULT;
+    if (H5Pclose(gcpl_in) < 0)
+        H5TOOLS_GOTO_ERROR((-1), "could not close fcpl");
+    if (H5Gclose(grp_in) < 0)
+        H5TOOLS_GOTO_ERROR((-1), "could not close fcpl");
+    if (H5Fclose(fidout) < 0)
+        H5TOOLS_GOTO_ERROR((-1), "could not close fcpl");
+    if (H5Fclose(fidin) < 0)
+        H5TOOLS_GOTO_ERROR((-1), "could not close fcpl");
+
+    /*-------------------------------------------------------------------------
+     * NOTE: The userblock MUST be written out AFTER the file is closed or
+     * the file locking will cause failures on Windows, where file locks
+     * are mandatory, not advisory.
      *-------------------------------------------------------------------------
      */
 
-    if (ub_size > 0 && options->ublock_size == 0)
+    /*-------------------------------------------------------------------------
+     * Write a new user block if requested, using the input file user block if
+     * there is no separate user block file input
+     *-------------------------------------------------------------------------
+     */
+
+    if (options->ublock_size > 0) {
+        if (copy_user_block(options->ublock_filename, fnameout, options->ublock_size) < 0)
+            H5TOOLS_GOTO_ERROR((-1), "Could not copy user block. Exiting...");
+    }
+    else if (ub_size > 0 && options->ublock_size == 0) {
         if (copy_user_block(fnamein, fnameout, ub_size) < 0)
             H5TOOLS_GOTO_ERROR((-1), "Could not copy user block. Exiting...");
+    }
 
 done:
-    H5E_BEGIN_TRY
+    if (-1 == ret_value) {
-    {
+        H5E_BEGIN_TRY
-        H5Pclose(fcpl);
+        {
-        H5Pclose(options->fout_fapl);
+            H5Pclose(fcpl);
-        options->fout_fapl = H5P_DEFAULT;
+            H5Pclose(options->fout_fapl);
-        H5Pclose(gcpl_in);
+            options->fout_fapl = H5P_DEFAULT;
-        H5Gclose(grp_in);
+            H5Pclose(gcpl_in);
-        H5Pclose(fcpl_in);
+            H5Gclose(grp_in);
-        H5Fclose(fidout);
+            H5Pclose(fcpl_in);
-        H5Fclose(fidin);
+            H5Fclose(fidout);
+            H5Fclose(fidin);
+        }
+        H5E_END_TRY;
     }
-    H5E_END_TRY;
     if (travt)
         trav_table_free(travt);