diff --git a/NEWS.md b/NEWS.md index b013146..a6cb576 100644 --- a/NEWS.md +++ b/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 + ``: Permission denied." (#30). + # filelock 1.0.3 * No user visible changes. 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 0ecfc66..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); } } @@ -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 { diff --git a/src/filelock-windows.c b/src/filelock-windows.c index 5467df5..8718126 100644 --- a/src/filelock-windows.c +++ b/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) { 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 31d434f..2d109f3 100644 --- a/tests/testthat/test.R +++ b/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, "", 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) +})