From fa40bcdc5cf9e6fc6f46236df258d3fa536ccdc7 Mon Sep 17 00:00:00 2001 From: David Steele Date: Mon, 11 Apr 2022 14:08:16 -0400 Subject: [PATCH] Throw error when unable to read lock process. Previously the process id was skipped if it did not exist. Instead, throw an error and handle the errors in downstream code. This was probably ignored at some point to provide backward-compatibility, but that is no longer required, if it ever was. --- doc/xml/release.xml | 9 +++-- src/command/control/stop.c | 19 +++++----- src/common/lock.c | 50 +++++++++++++++++---------- test/src/module/command/controlTest.c | 23 +++++++++--- 4 files changed, 65 insertions(+), 36 deletions(-) diff --git a/doc/xml/release.xml b/doc/xml/release.xml index 1223495ac..298fde25b 100644 --- a/doc/xml/release.xml +++ b/doc/xml/release.xml @@ -158,14 +158,19 @@ - + + + + + + -

Add lockRead*() functions for reading locks from another process.

+

Lock module refactoring.

 diff --git a/src/command/control/stop.c b/src/command/control/stop.c index 270aeba17..a0d254cdc 100644 --- a/src/command/control/stop.c +++ b/src/command/control/stop.c @@ -69,21 +69,18 @@ cmdStop(void) lockFile = strNewFmt("%s/%s", strZ(lockPath), strZ(lockFile)); LockReadResult lockRead = lockReadFileP(lockFile, .remove = true); - // If we cannot open the lock file for any reason then warn and continue to next file - if (lockRead.status != lockReadStatusValid && lockRead.status != lockReadStatusUnlocked) + // If we cannot read the lock file for any reason then warn and continue to next file + if (lockRead.status != lockReadStatusValid) { - LOG_WARN_FMT("unable to open lock file %s", strZ(lockFile)); + LOG_WARN_FMT("unable to read lock file %s", strZ(lockFile)); continue; } - // The file is locked so that means there is a running process -- send a term signal to the process if valid - if (lockRead.data.processId != 0) - { - if (kill(lockRead.data.processId, SIGTERM) != 0) - LOG_WARN_FMT("unable to send term signal to process %d", lockRead.data.processId); - else - LOG_INFO_FMT("sent term signal to process %d", lockRead.data.processId); - } + // The lock file is valid so that means there is a running process -- send a term signal to the process + if (kill(lockRead.data.processId, SIGTERM) != 0) + LOG_WARN_FMT("unable to send term signal to process %d", lockRead.data.processId); + else + LOG_INFO_FMT("sent term signal to process %d", lockRead.data.processId); } } } diff --git a/src/common/lock.c b/src/common/lock.c index 2cc7ccbd5..17f55262b 100644 --- a/src/common/lock.c +++ b/src/common/lock.c @@ -92,29 +92,36 @@ lockReadFileData(const String *const lockFile, const int fd) LockData result = {0}; - MEM_CONTEXT_TEMP_BEGIN() + TRY_BEGIN() { - // Read contents of file - Buffer *const buffer = bufNew(LOCK_BUFFER_SIZE); - IoWrite *const write = ioBufferWriteNewOpen(buffer); - - ioCopyP(ioFdReadNewOpen(lockFile, fd, 0), write); - ioWriteClose(write); - - // Parse the file - const StringList *const parse = strLstNewSplitZ(strNewBuf(buffer), LF_Z); - - MEM_CONTEXT_PRIOR_BEGIN() + MEM_CONTEXT_TEMP_BEGIN() { - if (!strEmpty(strTrim(strLstGet(parse, 0)))) - result.processId = cvtZToInt(strZ(strLstGet(parse, 0))); + // Read contents of file + Buffer *const buffer = bufNew(LOCK_BUFFER_SIZE); + IoWrite *const write = ioBufferWriteNewOpen(buffer); - if (strLstSize(parse) == 3) - result.execId = strDup(strLstGet(parse, 1)); + ioCopyP(ioFdReadNewOpen(lockFile, fd, 0), write); + ioWriteClose(write); + + // Parse the file + const StringList *const parse = strLstNewSplitZ(strNewBuf(buffer), LF_Z); + + MEM_CONTEXT_PRIOR_BEGIN() + { + if (strLstSize(parse) == 3) + result.execId = strDup(strLstGet(parse, 1)); + + result.processId = cvtZToInt(strZ(strLstGet(parse, 0))); + } + MEM_CONTEXT_PRIOR_END(); } - MEM_CONTEXT_PRIOR_END(); + MEM_CONTEXT_TEMP_END(); } - MEM_CONTEXT_TEMP_END(); + CATCH_ANY() + { + THROWP_FMT(errorType(), "unable to read lock file '%s': %s", strZ(lockFile), errorMessage()); + } + TRY_END(); FUNCTION_LOG_RETURN_STRUCT(result); } @@ -286,6 +293,13 @@ lockAcquireFile(const String *const lockFile, const TimeMSec lockTimeout, const { execId = lockReadFileData(lockFile, result).execId; } + CATCH_ANY() + { + // Any errors will be reported as unable to acquire a lock. If a process is trying to get a lock but is not + // synchronized with the process holding the actual lock, it is possible that it could see a short read or + // have some other problem reading. Reporting the error will likely be misleading when the actual problem is + // that another process owns the lock file. + } FINALLY() { close(result); diff --git a/test/src/module/command/controlTest.c b/test/src/module/command/controlTest.c index 6da70a5ee..96ac70f80 100644 --- a/test/src/module/command/controlTest.c +++ b/test/src/module/command/controlTest.c @@ -136,14 +136,14 @@ testRun(void) TEST_STORAGE_EXISTS(hrnStorage, "lock/db" STOP_FILE_EXT, .remove = true, .comment = "stanza stop file created, remove"); // ------------------------------------------------------------------------------------------------------------------------- - TEST_TITLE("unable to open lock file"); + TEST_TITLE("unable to read lock file"); HRN_STORAGE_PUT_EMPTY( hrnStorage, "lock/db-archive" LOCK_FILE_EXT, .modeFile = 0222, .comment = "create a lock file that cannot be opened"); - TEST_RESULT_VOID(cmdStop(), "stanza, create stop file but unable to open lock file"); + TEST_RESULT_VOID(cmdStop(), "stanza, create stop file but unable to read lock file"); TEST_STORAGE_EXISTS(hrnStorage, "lock/db" STOP_FILE_EXT, .comment = "stanza stop file created"); - TEST_RESULT_LOG("P00 WARN: unable to open lock file " HRN_PATH "/lock/db-archive" LOCK_FILE_EXT); + TEST_RESULT_LOG("P00 WARN: unable to read lock file " HRN_PATH "/lock/db-archive" LOCK_FILE_EXT); HRN_STORAGE_PATH_REMOVE(hrnStorage, "lock", .recurse = true, .errorOnMissing = true, .comment = "remove the lock path"); // ------------------------------------------------------------------------------------------------------------------------- @@ -155,6 +155,8 @@ testRun(void) hrnStorage, "lock", "db" STOP_FILE_EXT "\n", .comment = "stanza stop file created, no other process lock, lock file was removed"); + TEST_RESULT_LOG_FMT("P00 WARN: unable to read lock file " HRN_PATH "/lock/db-backup.lock"); + // ------------------------------------------------------------------------------------------------------------------------- TEST_TITLE("empty lock file with another process lock, processId == NULL"); @@ -192,6 +194,8 @@ testRun(void) // Notify child to release lock HRN_FORK_PARENT_NOTIFY_PUT(0); + + TEST_RESULT_LOG_FMT("P00 WARN: unable to read lock file " HRN_PATH "/lock/db-backup.lock"); } HRN_FORK_PARENT_END(); } @@ -234,6 +238,8 @@ testRun(void) // Notify child to release lock HRN_FORK_PARENT_NOTIFY_PUT(0); + + TEST_RESULT_LOG_FMT("P00 WARN: unable to read lock file " HRN_PATH "/lock/db-backup.lock"); } HRN_FORK_PARENT_END(); } @@ -336,9 +342,13 @@ testRun(void) TEST_STORAGE_EXISTS(hrnStorage, "lock/all" STOP_FILE_EXT, .comment = "stanza stop file created"); TEST_STORAGE_LIST( hrnStorage, "lock", "all" STOP_FILE_EXT "\n" "db-junk.txt\n", .comment = "stop file created, all lock files processed"); - TEST_RESULT_LOG(""); HRN_STORAGE_PATH_REMOVE(hrnStorage, "lock", .recurse = true, .errorOnMissing = true, .comment = "remove the lock path"); + TEST_RESULT_LOG_FMT( + "P00 WARN: unable to read lock file " HRN_PATH "/lock/db-archive.lock\n" + "P00 WARN: unable to read lock file " HRN_PATH "/lock/db-backup.lock\n" + "P00 WARN: unable to read lock file " HRN_PATH "/lock/db1-backup.lock"); + // ------------------------------------------------------------------------------------------------------------------------- TEST_TITLE("stanza, force stop = process only stanza lock files, ignore other stanza lock files and other files"); @@ -355,8 +365,11 @@ testRun(void) TEST_STORAGE_LIST( hrnStorage, "lock", "db-junk.txt\ndb" STOP_FILE_EXT "\n" "db1-backup" LOCK_FILE_EXT "\n", .comment = "stop file created, stanza lock file was removed, other stanza lock and other files remain"); - TEST_RESULT_LOG(""); HRN_STORAGE_PATH_REMOVE(hrnStorage, "lock", .recurse = true, .errorOnMissing = true, .comment = "remove the lock path"); + + TEST_RESULT_LOG_FMT( + "P00 WARN: unable to read lock file " HRN_PATH "/lock/db-archive.lock\n" + "P00 WARN: unable to read lock file " HRN_PATH "/lock/db-backup.lock"); } FUNCTION_HARNESS_RETURN_VOID();