From 17aa4225a04203bcce79610d4fa54e23e4ce2aac Mon Sep 17 00:00:00 2001 From: Robert Szarek Date: Thu, 17 Sep 2026 13:25:36 -0500 Subject: [PATCH 1/4] Include lock file path in locking error messages Errors raised when opening or locking a lock file fails now name the file, so users can find and remove stale lock files. Closes #30. --- NEWS.md | 3 +++ src/filelock-unix.c | 4 +++- src/filelock-windows.c | 33 +++++++++++++++++++-------------- tests/testthat/test.R | 22 ++++++++++++++++++++-- 4 files changed, 45 insertions(+), 17 deletions(-) diff --git a/NEWS.md b/NEWS.md index b013146..5e5f07c 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,8 @@ # filelock (development version) +* Error messages now include the path of the lock file when locking fails, + e.g. `Cannot open lock file: '': Permission denied` (#30). + # filelock 1.0.3 * No user visible changes. diff --git a/src/filelock-unix.c b/src/filelock-unix.c index 0ecfc66..a318481 100644 --- a/src/filelock-unix.c +++ b/src/filelock-unix.c @@ -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("Cannot open lock file: '%s': %s", c_path, strerror(errno)); + } /* One shot only? Do not block if cannot lock */ if (c_timeout == 0) { diff --git a/src/filelock-windows.c b/src/filelock-windows.c index 5467df5..b7a9828 100644 --- a/src/filelock-windows.c +++ b/src/filelock-windows.c @@ -18,7 +18,7 @@ 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; @@ -36,7 +36,7 @@ void filelock__error(const char *str, DWORD errorcode) { strcpy(msg, lpMsgBuf); LocalFree(lpMsgBuf); - error("Filelock error (%d), %s %s", (int) errorcode, str, msg); + error("Filelock error (%d), %s'%s': %s", (int) errorcode, str, path, msg); } void filelock__finalizer(SEXP x) { @@ -74,7 +74,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 +86,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("Locking file: ", path, err); + } wres = WaitForSingleObject(ov.hEvent, FILELOCK_INTERRUPT_INTERVAL); if (wres == WAIT_TIMEOUT) { @@ -97,7 +99,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("Locking file (timeout): ", path, GetLastError()); } /* Check for interrupt and try again */ @@ -106,7 +108,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 +117,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 +137,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("Locking file: ", path, err); + } waitnow = timeleft < FILELOCK_INTERRUPT_INTERVAL ? timeleft : FILELOCK_INTERRUPT_INTERVAL; @@ -147,7 +152,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("Locking file (timeout): ", path, GetLastError()); } /* Check for interrupt and try again */ @@ -156,7 +161,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; } @@ -202,7 +207,7 @@ SEXP filelock_lock(SEXP path, SEXP exclusive, SEXP timeout) { /* hTemplateFile = */ NULL); if (file == INVALID_HANDLE_VALUE) { - filelock__error("Opening file: ", GetLastError()); + filelock__error("Opening file: ", c_path, GetLastError()); } /* Give it a try, fail immediately */ @@ -211,16 +216,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) { diff --git a/tests/testthat/test.R b/tests/testthat/test.R index 31d434f..2257084 100644 --- a/tests/testthat/test.R +++ b/tests/testthat/test.R @@ -308,13 +308,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 +336,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) +}) From 355810be2797d56d87c8946c56890b210aa5919d Mon Sep 17 00:00:00 2001 From: Robert Szarek Date: Thu, 17 Sep 2026 13:34:47 -0500 Subject: [PATCH 2/4] Nudge locking error messages toward tidyverse style - Use "Can't", backticks around file paths, and trailing full stops. - The 'already has a shared/exclusive lock' errors now name the file. - Fix utf8.c to the new filelock__error() signature; it would not have compiled on Windows otherwise. - Strip the trailing CRLF of Windows system error messages. Ref #30. --- NEWS.md | 3 ++- R/package.R | 6 ++++-- src/filelock-unix.c | 10 +++++----- src/filelock-windows.c | 31 ++++++++++++++++++++----------- src/utf8.c | 6 +++--- tests/testthat/_snaps/test.md | 4 ++-- tests/testthat/test.R | 15 +++++++++++++-- 7 files changed, 49 insertions(+), 26 deletions(-) diff --git a/NEWS.md b/NEWS.md index 5e5f07c..a6cb576 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,7 +1,8 @@ # filelock (development version) * Error messages now include the path of the lock file when locking fails, - e.g. `Cannot open lock file: '': Permission denied` (#30). + and follow the tidyverse error style, e.g. "Can't open lock file + ``: Permission denied." (#30). # filelock 1.0.3 diff --git a/R/package.R b/R/package.R index 58b7570..f4782fe 100644 --- a/R/package.R +++ b/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)) diff --git a/src/filelock-unix.c b/src/filelock-unix.c index a318481..3ac0ea9 100644 --- a/src/filelock-unix.c +++ b/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); } } @@ -116,7 +116,7 @@ SEXP filelock_lock(SEXP path, SEXP exclusive, SEXP timeout) { filedes = open(c_path, O_RDWR | O_CREAT, S_IRUSR | S_IWUSR); if (filedes == -1) { - error("Cannot open lock file: '%s': %s", c_path, strerror(errno)); + error("Can't open lock file `%s`: %s.", c_path, strerror(errno)); } /* One shot only? Do not block if cannot lock */ @@ -126,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 { diff --git a/src/filelock-windows.c b/src/filelock-windows.c index b7a9828..65a0092 100644 --- a/src/filelock-windows.c +++ b/src/filelock-windows.c @@ -21,6 +21,7 @@ int filelock__is_interrupt_pending(void) { 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, const char *path, DWORD errorcode) { strcpy(msg, lpMsgBuf); LocalFree(lpMsgBuf); - error("Filelock error (%d), %s'%s': %s", (int) errorcode, str, path, 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) { @@ -87,7 +96,7 @@ int filelock__lock_wait(HANDLE file, int exclusive, const char *path) { while (1) { DWORD wres; if (err != ERROR_IO_PENDING) { - filelock__error("Locking file: ", path, err); + filelock__error("lock file", path, err); } wres = WaitForSingleObject(ov.hEvent, FILELOCK_INTERRUPT_INTERVAL); @@ -99,7 +108,7 @@ int filelock__lock_wait(HANDLE file, int exclusive, const char *path) { } else if (wres == WAIT_FAILED) { CancelIo(file); CloseHandle(ov.hEvent); - filelock__error("Locking file (timeout): ", path, GetLastError()); + filelock__error("lock file", path, GetLastError()); } /* Check for interrupt and try again */ @@ -108,7 +117,7 @@ int filelock__lock_wait(HANDLE file, int exclusive, const char *path) { CloseHandle(ov.hEvent); UnlockFileEx(file, 0, 1, 0, &ov); /* ignore errors */ CloseHandle(file); /* ignore errors */ - error("Locking interrupted for file: '%s'", path); + error("Locking interrupted for file `%s`.", path); } } } @@ -138,7 +147,7 @@ int filelock__lock_timeout(HANDLE file, int exclusive, int timeout, int waitnow; if (err != ERROR_IO_PENDING) { - filelock__error("Locking file: ", path, err); + filelock__error("lock file", path, err); } waitnow = timeleft < FILELOCK_INTERRUPT_INTERVAL ? timeleft : @@ -152,7 +161,7 @@ int filelock__lock_timeout(HANDLE file, int exclusive, int timeout, } else { CancelIo(file); CloseHandle(ov.hEvent); - filelock__error("Locking file (timeout): ", path, GetLastError()); + filelock__error("lock file", path, GetLastError()); } /* Check for interrupt and try again */ @@ -161,7 +170,7 @@ int filelock__lock_timeout(HANDLE file, int exclusive, int timeout, CloseHandle(ov.hEvent); UnlockFileEx(file, 0, 1, 0, &ov); /* ignore errors */ CloseHandle(file); /* ignore errors */ - error("Locking interrupted for file: '%s'", path); + error("Locking interrupted for file `%s`.", path); } timeleft -= FILELOCK_INTERRUPT_INTERVAL; } @@ -189,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); } } @@ -207,7 +216,7 @@ SEXP filelock_lock(SEXP path, SEXP exclusive, SEXP timeout) { /* hTemplateFile = */ NULL); if (file == INVALID_HANDLE_VALUE) { - filelock__error("Opening file: ", c_path, GetLastError()); + filelock__error("open lock file", c_path, GetLastError()); } /* Give it a try, fail immediately */ @@ -225,7 +234,7 @@ SEXP filelock_lock(SEXP path, SEXP exclusive, SEXP timeout) { } if (ret) { - filelock__error("Lock file: ", c_path, ret); + filelock__error("lock file", c_path, ret); } if (!locked) { diff --git a/src/utf8.c b/src/utf8.c index 47849fb..186bda9 100644 --- a/src/utf8.c +++ b/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; diff --git a/tests/testthat/_snaps/test.md b/tests/testthat/_snaps/test.md index 52cec06..239b5fc 100644 --- a/tests/testthat/_snaps/test.md +++ b/tests/testthat/_snaps/test.md @@ -4,7 +4,7 @@ lock(tmp, exclusive = FALSE) Condition Error in `lock()`: - ! File already has an exclusive lock + ! File `` already has an exclusive lock. --- @@ -12,7 +12,7 @@ lock(tmp, exclusive = TRUE) Condition Error in `lock()`: - ! File already has a shared lock + ! File `` already has a shared lock. # unlock() needs lock object diff --git a/tests/testthat/test.R b/tests/testthat/test.R index 2257084..dad0dbc 100644 --- a/tests/testthat/test.R +++ b/tests/testthat/test.R @@ -292,12 +292,23 @@ 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 + scrub_path <- function(x) gsub(normalizePath(tmp), "", 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) }) From 02a4fa3f24abde2b023dfd31650e3a74f288c30f Mon Sep 17 00:00:00 2001 From: Robert Szarek Date: Thu, 17 Sep 2026 13:40:09 -0500 Subject: [PATCH 3/4] Fix path scrubbing in snapshot test on Windows lock() reports the path as file.path(normalizePath(dirname), basename), which mixes path separators on Windows, so scrub that exact string. Ref #30. --- tests/testthat/test.R | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/tests/testthat/test.R b/tests/testthat/test.R index dad0dbc..2d109f3 100644 --- a/tests/testthat/test.R +++ b/tests/testthat/test.R @@ -292,8 +292,11 @@ 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 - scrub_path <- function(x) gsub(normalizePath(tmp), "", x, fixed = TRUE) + # 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, "", x, fixed = TRUE) lck <- lock(tmp, exclusive = TRUE) expect_snapshot( From 9aea6621dafda2d41a526077f1c2ad3b08cb7c67 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=A1bor=20Cs=C3=A1rdi?= Date: Thu, 24 Sep 2026 17:14:11 +0200 Subject: [PATCH 4/4] Improve two error messages --- src/filelock-windows.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/filelock-windows.c b/src/filelock-windows.c index 65a0092..8718126 100644 --- a/src/filelock-windows.c +++ b/src/filelock-windows.c @@ -108,7 +108,7 @@ int filelock__lock_wait(HANDLE file, int exclusive, const char *path) { } else if (wres == WAIT_FAILED) { CancelIo(file); CloseHandle(ov.hEvent); - filelock__error("lock file", path, GetLastError()); + filelock__error("lock file (timeout)", path, GetLastError()); } /* Check for interrupt and try again */ @@ -161,7 +161,7 @@ int filelock__lock_timeout(HANDLE file, int exclusive, int timeout, } else { CancelIo(file); CloseHandle(ov.hEvent); - filelock__error("lock file", path, GetLastError()); + filelock__error("lock file (timeout)", path, GetLastError()); } /* Check for interrupt and try again */