Include lock file path in locking error messages

NEWS.md

@@ -1,5 +1,9 @@
 # filelock (development version)
 
+* Error messages now include the path of the lock file when locking fails,
+  and follow the tidyverse error style, e.g. "Can't open lock file
+  `<path>`: Permission denied." (#30).
+
 # filelock 1.0.3
 
 * No user visible changes.

R/package.R

@@ -149,12 +149,14 @@ lock <- function(path, exclusive = TRUE, timeout = Inf) {
   stopifnot(is_timeout(timeout))
 
   ## Inf if encoded as -1 in our C code
-  if (timeout == Inf) timeout <- -1L
+  if (timeout == Inf) {
+    timeout <- -1L
+  }
 
   dn <- dirname(path)
   ndn <- normalizePath(dn)
   if (!file.exists(ndn)) {
-    stop("Directory of lock file does not exist: '", dn, "'")
+    stop("Directory of lock file does not exist: `", dn, "`.")
   }
   path <- file.path(ndn, basename(path))
 

src/filelock-unix.c

@@ -78,7 +78,7 @@ int filelock__interruptible(int filedes, struct flock *lck,
        If -1, but not EINTR, then a real error happened. */
     if (ret != -1) { ret = 0; break; }
     if (ret == -1 && errno != EINTR) {
-      error("Cannot lock file: '%s': %s", c_path, strerror(errno));
+      error("Can't lock file `%s`: %s.", c_path, strerror(errno));
     }
 
     /* Otherwise, need to wait, check for interrupts, and start over */
@@ -103,9 +103,9 @@ SEXP filelock_lock(SEXP path, SEXP exclusive, SEXP timeout) {
 	(!c_exclusive && !node->exclusive)) {
       return filelock__make_lock_handle(node);
     } else if (c_exclusive) {
-      error("File already has a shared lock");
+      error("File `%s` already has a shared lock.", c_path);
     } else {
-      error("File already has an exclusive lock");
+      error("File `%s` already has an exclusive lock.", c_path);
     }
   }
 
@@ -115,7 +115,9 @@ SEXP filelock_lock(SEXP path, SEXP exclusive, SEXP timeout) {
   lck.l_len = 0;
 
   filedes = open(c_path, O_RDWR | O_CREAT, S_IRUSR | S_IWUSR);
-  if (filedes == -1) error("Cannot open lock file: %s", strerror(errno));
+  if (filedes == -1) {
+    error("Can't open lock file `%s`: %s.", c_path, strerror(errno));
+  }
 
   /* One shot only? Do not block if cannot lock */
   if (c_timeout == 0) {
@@ -124,7 +126,7 @@ SEXP filelock_lock(SEXP path, SEXP exclusive, SEXP timeout) {
       if (errno == EAGAIN || errno == EACCES) {
 	return R_NilValue;
       }
-      error("Cannot lock file: '%s': %s", c_path, strerror(errno));
+      error("Can't lock file `%s`: %s.", c_path, strerror(errno));
     }
 
   } else {

src/filelock-windows.c

@@ -18,9 +18,10 @@ int filelock__is_interrupt_pending(void) {
   return !(R_ToplevelExec(filelock__check_interrupt_fn, NULL));
 }
 
-void filelock__error(const char *str, DWORD errorcode) {
+void filelock__error(const char *str, const char *path, DWORD errorcode) {
   LPVOID lpMsgBuf;
   char *msg;
+  size_t len;
 
   FormatMessage(
     FORMAT_MESSAGE_ALLOCATE_BUFFER |
@@ -36,7 +37,15 @@ void filelock__error(const char *str, DWORD errorcode) {
   strcpy(msg, lpMsgBuf);
   LocalFree(lpMsgBuf);
 
-  error("Filelock error (%d), %s %s", (int) errorcode, str, msg);
+  /* Strip the trailing whitespace (CRLF) of the system error message */
+  len = strlen(msg);
+  while (len > 0 && (msg[len - 1] == '\r' || msg[len - 1] == '\n' ||
+		     msg[len - 1] == ' ')) {
+    msg[--len] = '\0';
+  }
+
+  error("Can't %s `%s`: %s (Windows error %d).",
+	str, path, msg, (int) errorcode);
 }
 
 void filelock__finalizer(SEXP x) {
@@ -74,7 +83,7 @@ int filelock__lock_now(HANDLE file, int exclusive, int *locked) {
   }
 }
 
-int filelock__lock_wait(HANDLE file, int exclusive) {
+int filelock__lock_wait(HANDLE file, int exclusive, const char *path) {
   DWORD dwFlags = exclusive ? LOCKFILE_EXCLUSIVE_LOCK : 0;
   BOOL res;
   OVERLAPPED ov = { 0 };
@@ -86,7 +95,9 @@ int filelock__lock_wait(HANDLE file, int exclusive) {
     DWORD err = GetLastError();
     while (1) {
       DWORD wres;
-      if (err != ERROR_IO_PENDING) filelock__error("Locking file: ", err);
+      if (err != ERROR_IO_PENDING) {
+	filelock__error("lock file", path, err);
+      }
 
       wres = WaitForSingleObject(ov.hEvent, FILELOCK_INTERRUPT_INTERVAL);
       if (wres == WAIT_TIMEOUT) {
@@ -97,7 +108,7 @@ int filelock__lock_wait(HANDLE file, int exclusive) {
       } else if (wres == WAIT_FAILED) {
 	CancelIo(file);
 	CloseHandle(ov.hEvent);
-	filelock__error("Locking file (timeout): ", GetLastError());
+	filelock__error("lock file (timeout)", path, GetLastError());
       }
 
       /* Check for interrupt and try again */
@@ -106,7 +117,7 @@ int filelock__lock_wait(HANDLE file, int exclusive) {
 	CloseHandle(ov.hEvent);
 	UnlockFileEx(file, 0, 1, 0, &ov); /* ignore errors */
 	CloseHandle(file);		  /* ignore errors */
-	error("Locking interrupted");
+	error("Locking interrupted for file `%s`.", path);
       }
     }
   }
@@ -115,7 +126,8 @@ int filelock__lock_wait(HANDLE file, int exclusive) {
   return 0;
 }
 
-int filelock__lock_timeout(HANDLE file, int exclusive, int timeout, int *locked) {
+int filelock__lock_timeout(HANDLE file, int exclusive, int timeout,
+			   int *locked, const char *path) {
 
   DWORD dwFlags = exclusive ? LOCKFILE_EXCLUSIVE_LOCK : 0;
   BOOL res;
@@ -134,7 +146,9 @@ int filelock__lock_timeout(HANDLE file, int exclusive, int timeout, int *locked)
       DWORD wres;
       int waitnow;
 
-      if (err != ERROR_IO_PENDING) filelock__error("Locking file: ", err);
+      if (err != ERROR_IO_PENDING) {
+	filelock__error("lock file", path, err);
+      }
 
       waitnow = timeleft < FILELOCK_INTERRUPT_INTERVAL ? timeleft :
 	FILELOCK_INTERRUPT_INTERVAL;
@@ -147,7 +161,7 @@ int filelock__lock_timeout(HANDLE file, int exclusive, int timeout, int *locked)
       } else {
 	CancelIo(file);
 	CloseHandle(ov.hEvent);
-	filelock__error("Locking file (timeout): ", GetLastError());
+	filelock__error("lock file (timeout)", path, GetLastError());
       }
 
       /* Check for interrupt and try again */
@@ -156,7 +170,7 @@ int filelock__lock_timeout(HANDLE file, int exclusive, int timeout, int *locked)
 	CloseHandle(ov.hEvent);
 	UnlockFileEx(file, 0, 1, 0, &ov); /* ignore errors */
 	CloseHandle(file);		  /* ignore errors */
-	error("Locking interrupted");
+	error("Locking interrupted for file `%s`.", path);
       }
       timeleft -= FILELOCK_INTERRUPT_INTERVAL;
     }
@@ -184,9 +198,9 @@ SEXP filelock_lock(SEXP path, SEXP exclusive, SEXP timeout) {
 	(!c_exclusive && !node->exclusive)) {
       return filelock__make_lock_handle(node);
     } else if (c_exclusive) {
-      error("File already has a shared lock");
+      error("File `%s` already has a shared lock.", c_path);
     } else {
-      error("File already has an exclusive lock");
+      error("File `%s` already has an exclusive lock.", c_path);
     }
   }
 
@@ -202,7 +216,7 @@ SEXP filelock_lock(SEXP path, SEXP exclusive, SEXP timeout) {
     /* hTemplateFile = */         NULL);
 
   if (file == INVALID_HANDLE_VALUE) {
-    filelock__error("Opening file: ", GetLastError());
+    filelock__error("open lock file", c_path, GetLastError());
   }
 
   /* Give it a try, fail immediately */
@@ -211,16 +225,16 @@ SEXP filelock_lock(SEXP path, SEXP exclusive, SEXP timeout) {
 
   /* Wait indefintely */
   } else if (c_timeout == -1) {
-    ret = filelock__lock_wait(file, c_exclusive);
+    ret = filelock__lock_wait(file, c_exclusive, c_path);
 
   /* Finite timeout */
   } else {
     ret = filelock__lock_timeout(file, c_exclusive,
-				 c_timeout, &locked);
+				 c_timeout, &locked, c_path);
   }
 
   if (ret) {
-    filelock__error("Lock file: ", ret);
+    filelock__error("lock file", c_path, ret);
   }
 
   if (!locked) {

src/utf8.c

@@ -4,7 +4,7 @@
 
 #include "filelock.h"
 
-void filelock__error(const char *str, DWORD errorcode);
+void filelock__error(const char *str, const char *path, DWORD errorcode);
 
 int filelock__utf8_to_utf16_alloc(const char* s, WCHAR** ws_ptr) {
   int ws_len, r;
@@ -19,7 +19,7 @@ int filelock__utf8_to_utf16_alloc(const char* s, WCHAR** ws_ptr) {
     /* cchWideChar =    */ 0);
 
   if (ws_len <= 0) {
-    filelock__error("Cannot convert UTF8 file name to wide chararacter",
+    filelock__error("convert UTF-8 file name to wide character", s,
 		    GetLastError());
   }
 
@@ -34,7 +34,7 @@ int filelock__utf8_to_utf16_alloc(const char* s, WCHAR** ws_ptr) {
     /* cchWideChar =    */ ws_len);
 
   if (r != ws_len) {
-    error("filelock error interpreting UTF8 filename");
+    error("Can't interpret UTF-8 file name `%s`.", s);
   }
 
   *ws_ptr = ws;

tests/testthat/_snaps/test.md

@@ -4,15 +4,15 @@
       lock(tmp, exclusive = FALSE)
     Condition
       Error in `lock()`:
-      ! File already has an exclusive lock
+      ! File `<path>` already has an exclusive lock.
 
 ---
 
     Code
       lock(tmp, exclusive = TRUE)
     Condition
       Error in `lock()`:
-      ! File already has a shared lock
+      ! File `<path>` already has a shared lock.
 
 # unlock() needs lock object
 

tests/testthat/test.R

@@ -292,12 +292,26 @@ test_that("Relocking does not affect unlocked locks", {
 
 test_that("Multiple, incompatible lock types", {
   tmp <- tempfile()
+  # The error message contains the lock file path, scrub it for stability.
+  # lock() builds the path as file.path(normalizePath(dirname), basename),
+  # which mixes path separators on Windows.
+  msg_path <- file.path(normalizePath(dirname(tmp)), basename(tmp))
+  scrub_path <- function(x) gsub(msg_path, "<path>", x, fixed = TRUE)
+
   lck <- lock(tmp, exclusive = TRUE)
-  expect_snapshot(error = TRUE, lock(tmp, exclusive = FALSE))
+  expect_snapshot(
+    error = TRUE,
+    lock(tmp, exclusive = FALSE),
+    transform = scrub_path
+  )
   unlock(lck)
 
   lck <- lock(tmp, exclusive = FALSE)
-  expect_snapshot(error = TRUE, lock(tmp, exclusive = TRUE))
+  expect_snapshot(
+    error = TRUE,
+    lock(tmp, exclusive = TRUE),
+    transform = scrub_path
+  )
   unlock(lck)
 })
 
@@ -308,13 +322,17 @@ test_that("UTF-8 filenames", {
   good <- tryCatch(
     {
       cat("hello\n", file = tmp)
-      if (readLines(tmp) != "hello") stop("Not good")
+      if (readLines(tmp) != "hello") {
+        stop("Not good")
+      }
       unlink(tmp)
       TRUE
     },
     error = function(e) FALSE
   )
-  if (identical(good, FALSE)) skip("FS does not support Unicode file names")
+  if (identical(good, FALSE)) {
+    skip("FS does not support Unicode file names")
+  }
 
   expect_silent(l <- lock(tmp))
   expect_equal(Encoding(l[[2]]), "UTF-8")
@@ -332,3 +350,17 @@ test_that("non-exclusive lock with timeout", {
 test_that("unlock() needs lock object", {
   expect_snapshot(error = TRUE, unlock(1))
 })
+
+test_that("lock file path is included in the error message", {
+  # Directory permissions do not reliably restrict file creation on Windows
+  skip_on_os("windows")
+
+  dir <- tempfile()
+  dir.create(dir)
+  on.exit(unlink(dir, recursive = TRUE), add = TRUE)
+  Sys.chmod(dir, mode = "0555")
+  on.exit(Sys.chmod(dir, mode = "0755"), add = TRUE)
+
+  lck <- file.path(dir, "test.lock")
+  expect_error(lock(lck), normalizePath(dir), fixed = TRUE)
+})