From 1356ecc58fa84d6072c1f3a041b9cd6d905202ea Mon Sep 17 00:00:00 2001 From: Jürgen Knödlseder Date: Wed, 29 Jul 2020 09:35:05 +0200 Subject: [PATCH] Make parameter file reading and writing thread safe (#3287) --- ChangeLog | 3 ++- NEWS | 6 ++++-- doc/source/admin/release_history/1.7.1.rst | 4 +++- src/app/GApplicationPars.cpp | 214 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------------------------------------------------------------------------------------------------------- 4 files changed, 120 insertions(+), 107 deletions(-) diff --git a/ChangeLog b/ChangeLog index cdea45e..3002abe 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,8 +1,9 @@ -2020-07-22 +2020-07-29 * Bug fix version 1.7.1 released ================================ + Make parameter file reading and writing thread safe (#3287) Add quotes to sky region names (#3280) Fix broken power law flux and eflux computation bug (#3279) diff --git a/NEWS b/NEWS index a6e4394..71e3dd2 100644 --- a/NEWS +++ b/NEWS @@ -1,6 +1,6 @@ New Features and Important Changes in GammaLib 1.7.1 -22 July 2020 +29 July 2020 1. Introduction @@ -43,7 +43,9 @@ None 4. Application module --------------------- -None +The reading and writing of the parameter file in GApplicationPars::read() +and GApplicationPars::write() was put in an OpenMP critical zone to assure +that the reading and writing is thread safe (#3287). 5. Base module diff --git a/doc/source/admin/release_history/1.7.1.rst b/doc/source/admin/release_history/1.7.1.rst index 53271ed..8bfbf8a 100644 --- a/doc/source/admin/release_history/1.7.1.rst +++ b/doc/source/admin/release_history/1.7.1.rst @@ -1,6 +1,6 @@ .. _1.7.1: -GammaLib 1.7.1 release (22 July 2020) +GammaLib 1.7.1 release (29 July 2020) ===================================== Overview @@ -10,6 +10,8 @@ GammaLib 1.7.1 is a bug fix release for GammaLib release 1.7. The following bugs were fixed: +* [`3287 `_] - + Make parameter file reading and writing thread safe * [`3280 `_] - Add quotes to sky region names * [`3279 `_] - diff --git a/src/app/GApplicationPars.cpp b/src/app/GApplicationPars.cpp index 0f9db00..3f80d3c 100644 --- a/src/app/GApplicationPars.cpp +++ b/src/app/GApplicationPars.cpp @@ -1268,62 +1268,66 @@ std::string GApplicationPars::outpath(const std::string& filename) const ***************************************************************************/ void GApplicationPars::read(const std::string& filename) { - // Allocate line buffer - const int n = 1000; - char line[n]; - - // Trying to get file lock - #if defined(G_LOCK_PARFILE) - struct flock lock; - lock.l_type = F_RDLCK; // Want a read lock - lock.l_whence = SEEK_SET; // Want beginning of file - lock.l_start = 0; // No offset, lock entire file ... - lock.l_len = 0; // ... to the end - lock.l_pid = getpid(); // Current process ID - int fd; - if ((fd = open(filename.c_str(), O_RDONLY)) == -1) { - throw GException::par_file_open_error(G_READ, filename, - "Could not open file for locking."); - } - #if defined(G_CHECK_LOCK_PARFILE) - if (fcntl(fd, F_SETLKW, &lock) == -1) { // F_SETLKW: wait until unlocked - throw GException::par_file_open_error(G_READ, filename, - "Could not get a lock on the file."); - } - #else - fcntl(fd, F_SETLKW, &lock); - #endif - #endif + // Put in OpenMP critical zone + #pragma omp critical(GApplicationsPars_read) + { + // Allocate line buffer + const int n = 1000; + char line[n]; + + // Trying to get file lock + #if defined(G_LOCK_PARFILE) + struct flock lock; + lock.l_type = F_RDLCK; // Want a read lock + lock.l_whence = SEEK_SET; // Want beginning of file + lock.l_start = 0; // No offset, lock entire file ... + lock.l_len = 0; // ... to the end + lock.l_pid = getpid(); // Current process ID + int fd; + if ((fd = open(filename.c_str(), O_RDONLY)) == -1) { + throw GException::par_file_open_error(G_READ, filename, + "Could not open file for locking."); + } + #if defined(G_CHECK_LOCK_PARFILE) + if (fcntl(fd, F_SETLKW, &lock) == -1) { // F_SETLKW: wait until unlocked + throw GException::par_file_open_error(G_READ, filename, + "Could not get a lock on the file."); + } + #else + fcntl(fd, F_SETLKW, &lock); + #endif + #endif - // Open parameter file - FILE* fptr = fopen(filename.c_str(), "r"); - if (fptr == NULL) { - throw GException::par_file_open_error(G_READ, filename); - } + // Open parameter file + FILE* fptr = fopen(filename.c_str(), "r"); + if (fptr == NULL) { + throw GException::par_file_open_error(G_READ, filename); + } - // Read lines - while (fgets(line, n, fptr) != NULL) { - m_parfile.push_back(std::string(line)); - } + // Read lines + while (fgets(line, n, fptr) != NULL) { + m_parfile.push_back(std::string(line)); + } - // Close file - fclose(fptr); + // Close file + fclose(fptr); - // Unlock file - #if defined(G_LOCK_PARFILE) - if (fd != -1) { - lock.l_type = F_UNLCK; - #if defined(G_CHECK_LOCK_PARFILE) - if (fcntl(fd, F_SETLK, &lock) == -1) { - throw GException::par_file_open_error(G_READ, filename, - "Could not unlock the file."); + // Unlock file + #if defined(G_LOCK_PARFILE) + if (fd != -1) { + lock.l_type = F_UNLCK; + #if defined(G_CHECK_LOCK_PARFILE) + if (fcntl(fd, F_SETLK, &lock) == -1) { + throw GException::par_file_open_error(G_READ, filename, + "Could not unlock the file."); + } + #else + fcntl(fd, F_SETLK, &lock); + #endif + close(fd); } - #else - fcntl(fd, F_SETLK, &lock); #endif - close(fd); } - #endif // Return return; @@ -1343,42 +1347,23 @@ void GApplicationPars::read(const std::string& filename) ***************************************************************************/ void GApplicationPars::write(const std::string& filename) const { - // Trying to get file lock. We have to do this after opening the file - // using the fopen function, as the file may not exist, hence it needs - // to be created first. - #if defined(G_LOCK_PARFILE) - struct flock lock; - lock.l_type = F_WRLCK; // Want a write lock - lock.l_whence = SEEK_SET; // Want beginning of file - lock.l_start = 0; // No offset, lock entire file ... - lock.l_len = 0; // ... to the end - lock.l_pid = getpid(); // Current process ID - int fd; - if ((fd = open(filename.c_str(), O_WRONLY)) != -1) { - #if defined(G_CHECK_LOCK_PARFILE) - if (fcntl(fd, F_SETLKW, &lock) == -1) { // F_SETLKW: wait until unlocked - throw GException::par_file_open_error(G_WRITE, filename, - "Could not get a lock on the file."); - } - #else - fcntl(fd, F_SETLKW, &lock); - #endif - } - #endif - - // Open parameter file. - FILE* fptr = fopen(filename.c_str(), "w"); - if (fptr == NULL) { - throw GException::par_file_open_error(G_WRITE, filename); - } - - // If file is not locked then lock it now. - #if defined(G_LOCK_PARFILE) - if (fd == -1) { + // Put in OpenMP critical zone + #pragma omp critical(GApplicationsPars_write) + { + // Trying to get file lock. We have to do this after opening the file + // using the fopen function, as the file may not exist, hence it needs + // to be created first. + #if defined(G_LOCK_PARFILE) + struct flock lock; + lock.l_type = F_WRLCK; // Want a write lock + lock.l_whence = SEEK_SET; // Want beginning of file + lock.l_start = 0; // No offset, lock entire file ... + lock.l_len = 0; // ... to the end + lock.l_pid = getpid(); // Current process ID + int fd; if ((fd = open(filename.c_str(), O_WRONLY)) != -1) { #if defined(G_CHECK_LOCK_PARFILE) if (fcntl(fd, F_SETLKW, &lock) == -1) { // F_SETLKW: wait until unlocked - fclose(fptr); throw GException::par_file_open_error(G_WRITE, filename, "Could not get a lock on the file."); } @@ -1386,33 +1371,56 @@ void GApplicationPars::write(const std::string& filename) const fcntl(fd, F_SETLKW, &lock); #endif } - } - #endif + #endif - // Write lines - for (int i = 0; i < m_parfile.size(); ++i) { - fprintf(fptr, "%s", m_parfile[i].c_str()); - } + // Open parameter file. + FILE* fptr = fopen(filename.c_str(), "w"); + if (fptr == NULL) { + throw GException::par_file_open_error(G_WRITE, filename); + } - // Close file - fclose(fptr); + // If file is not locked then lock it now. + #if defined(G_LOCK_PARFILE) + if (fd == -1) { + if ((fd = open(filename.c_str(), O_WRONLY)) != -1) { + #if defined(G_CHECK_LOCK_PARFILE) + if (fcntl(fd, F_SETLKW, &lock) == -1) { // F_SETLKW: wait until unlocked + fclose(fptr); + throw GException::par_file_open_error(G_WRITE, filename, + "Could not get a lock on the file."); + } + #else + fcntl(fd, F_SETLKW, &lock); + #endif + } + } + #endif - // Unlock file - #if defined(G_LOCK_PARFILE) - if (fd != -1) { - lock.l_type = F_UNLCK; - #if defined(G_CHECK_LOCK_PARFILE) - if (fcntl(fd, F_SETLK, &lock) == -1) { - throw GException::par_file_open_error(G_WRITE, filename, - "Could not unlock the file."); + // Write lines + for (int i = 0; i < m_parfile.size(); ++i) { + fprintf(fptr, "%s", m_parfile[i].c_str()); + } + + // Close file + fclose(fptr); + + // Unlock file + #if defined(G_LOCK_PARFILE) + if (fd != -1) { + lock.l_type = F_UNLCK; + #if defined(G_CHECK_LOCK_PARFILE) + if (fcntl(fd, F_SETLK, &lock) == -1) { + throw GException::par_file_open_error(G_WRITE, filename, + "Could not unlock the file."); + } + #else + fcntl(fd, F_SETLK, &lock); + #endif + close(fd); } - #else - fcntl(fd, F_SETLK, &lock); #endif - close(fd); } - #endif - + // Return return; } -- libgit2 0.21.4