From 5bbf39d094efccadfd9a927c5f7052cde541e730 Mon Sep 17 00:00:00 2001 From: Greg Hewgill Date: Sat, 5 Mar 2011 16:43:22 +1300 Subject: [PATCH] Refactor ThreadExecutor::check() to handle child failures more gracefully --- Makefile | 2 +- cli/threadexecutor.cpp | 127 ++++++++++++++++++++++++++++------------- cli/threadexecutor.h | 8 ++- 3 files changed, 93 insertions(+), 44 deletions(-) diff --git a/Makefile b/Makefile index ca835744d..c0e5675cc 100644 --- a/Makefile +++ b/Makefile @@ -330,7 +330,7 @@ test/teststl.o: test/teststl.cpp lib/tokenize.h lib/checkstl.h lib/check.h lib/t test/testsuite.o: test/testsuite.cpp test/testsuite.h lib/errorlogger.h lib/settings.h test/redirect.h test/options.h $(CXX) $(CPPFLAGS) $(CXXFLAGS) ${INCLUDE_FOR_TEST} -c -o test/testsuite.o test/testsuite.cpp -test/testsuppressions.o: test/testsuppressions.cpp lib/cppcheck.h lib/settings.h lib/errorlogger.h lib/checkunusedfunctions.h lib/check.h lib/token.h lib/tokenize.h test/testsuite.h test/redirect.h +test/testsuppressions.o: test/testsuppressions.cpp lib/cppcheck.h lib/settings.h lib/errorlogger.h lib/checkunusedfunctions.h lib/check.h cli/threadexecutor.h lib/token.h lib/tokenize.h test/testsuite.h test/redirect.h $(CXX) $(CPPFLAGS) $(CXXFLAGS) ${INCLUDE_FOR_TEST} -c -o test/testsuppressions.o test/testsuppressions.cpp test/testsymboldatabase.o: test/testsymboldatabase.cpp test/testsuite.h lib/errorlogger.h lib/settings.h test/redirect.h test/testutils.h lib/tokenize.h lib/token.h lib/symboldatabase.h diff --git a/cli/threadexecutor.cpp b/cli/threadexecutor.cpp index bce5fd1d0..f9ae2459e 100644 --- a/cli/threadexecutor.cpp +++ b/cli/threadexecutor.cpp @@ -35,7 +35,7 @@ ThreadExecutor::ThreadExecutor(const std::vector &filenames, Settin : _filenames(filenames), _settings(settings), _errorLogger(errorLogger), _fileCount(0) { #ifdef THREADING_MODEL_FORK - _pipe[0] = _pipe[1] = 0; + _wpipe = 0; #endif } @@ -55,10 +55,10 @@ void ThreadExecutor::addFileContent(const std::string &path, const std::string & #ifdef THREADING_MODEL_FORK -int ThreadExecutor::handleRead(unsigned int &result) +int ThreadExecutor::handleRead(int rpipe, unsigned int &result) { char type = 0; - if (read(_pipe[0], &type, 1) <= 0) + if (read(rpipe, &type, 1) <= 0) { if (errno == EAGAIN) return 0; @@ -73,14 +73,14 @@ int ThreadExecutor::handleRead(unsigned int &result) } unsigned int len = 0; - if (read(_pipe[0], &len, sizeof(len)) <= 0) + if (read(rpipe, &len, sizeof(len)) <= 0) { std::cerr << "#### You found a bug from cppcheck.\nThreadExecutor::handleRead error, type was:" << type << std::endl; exit(0); } char *buf = new char[len]; - if (read(_pipe[0], buf, len) <= 0) + if (read(rpipe, buf, len) <= 0) { std::cerr << "#### You found a bug from cppcheck.\nThreadExecutor::handleRead error, type was:" << type << std::endl; exit(0); @@ -134,32 +134,35 @@ unsigned int ThreadExecutor::check() { _fileCount = 0; unsigned int result = 0; - if (pipe(_pipe) == -1) - { - perror("pipe"); - exit(1); - } - int flags = 0; - if ((flags = fcntl(_pipe[0], F_GETFL, 0)) < 0) - { - perror("fcntl"); - exit(1); - } - - if (fcntl(_pipe[0], F_SETFL, flags | O_NONBLOCK) < 0) - { - perror("fcntl"); - exit(1); - } - - unsigned int childCount = 0; + std::list rpipes; + std::map childFile; unsigned int i = 0; while (true) { // Start a new child - if (i < _filenames.size() && childCount < _settings._jobs) + if (i < _filenames.size() && rpipes.size() < _settings._jobs) { + int pipes[2]; + if (pipe(pipes) == -1) + { + perror("pipe"); + exit(1); + } + + int flags = 0; + if ((flags = fcntl(pipes[0], F_GETFL, 0)) < 0) + { + perror("fcntl"); + exit(1); + } + + if (fcntl(pipes[0], F_SETFL, flags | O_NONBLOCK) < 0) + { + perror("fcntl"); + exit(1); + } + pid_t pid = fork(); if (pid < 0) { @@ -169,6 +172,9 @@ unsigned int ThreadExecutor::check() } else if (pid == 0) { + close(pipes[0]); + _wpipe = pipes[1]; + CppCheck fileChecker(*this, false); fileChecker.settings(_settings); @@ -190,31 +196,70 @@ unsigned int ThreadExecutor::check() exit(0); } - ++childCount; + close(pipes[1]); + rpipes.push_back(pipes[0]); + childFile[pid] = _filenames[i]; + ++i; } - else if (childCount > 0) + else if (!rpipes.empty()) { - // Wait for child to quit before stating new processes - while (true) + fd_set rfds; + FD_ZERO(&rfds); + for (std::list::const_iterator rp = rpipes.begin(); rp != rpipes.end(); ++rp) + FD_SET(*rp, &rfds); + + int r = select(*std::max_element(rpipes.begin(), rpipes.end()) + 1, &rfds, NULL, NULL, NULL); + + if (r > 0) { - int readRes = handleRead(result); - if (readRes == -1) - break; - else if (readRes == 0) + std::list::iterator rp = rpipes.begin(); + while (rp != rpipes.end()) { - struct timespec duration; - duration.tv_sec = 0; - duration.tv_nsec = 5 * 1000 * 1000; // 5 ms - nanosleep(&duration, NULL); + if (FD_ISSET(*rp, &rfds)) + { + int readRes = handleRead(*rp, result); + if (readRes == -1) + { + close(*rp); + rp = rpipes.erase(rp); + } + else + ++rp; + } + else + ++rp; } } int stat = 0; - waitpid(0, &stat, 0); - --childCount; + pid_t child = waitpid(0, &stat, WNOHANG); + if (child > 0) + { + std::string childname; + std::map::iterator c = childFile.find(child); + if (c != childFile.end()) + { + childname = c->second; + childFile.erase(c); + } + + if (WIFSIGNALED(stat)) + { + std::ostringstream oss; + oss << "Internal error: Child process crashed with signal " << WTERMSIG(stat); + + std::list locations; + locations.push_back(ErrorLogger::ErrorMessage::FileLocation(childname, 0)); + const ErrorLogger::ErrorMessage errmsg(locations, + Severity::error, + oss.str(), + "cppcheckError"); + _errorLogger.reportErr(errmsg); + } + } } - else if (childCount == 0) + else { // All done break; @@ -232,7 +277,7 @@ void ThreadExecutor::writeToPipe(char type, const std::string &data) out[0] = type; std::memcpy(&(out[1]), &len, sizeof(len)); std::memcpy(&(out[1+sizeof(len)]), data.c_str(), len); - if (write(_pipe[1], out, len + 1 + sizeof(len)) <= 0) + if (write(_wpipe, out, len + 1 + sizeof(len)) <= 0) { delete [] out; out = 0; diff --git a/cli/threadexecutor.h b/cli/threadexecutor.h index 78cb9a5fe..bbbabe076 100644 --- a/cli/threadexecutor.h +++ b/cli/threadexecutor.h @@ -71,9 +71,13 @@ private: * 0 if there is nothing in the pipe to be read * 1 if we did read something */ - int handleRead(unsigned int &result); + int handleRead(int rpipe, unsigned int &result); void writeToPipe(char type, const std::string &data); - int _pipe[2]; + /** + * Write end of status pipe, different for each child. + * Not used in master process. + */ + int _wpipe; std::list _errorList; public: /**