From bad8b57da6c55ad40c4a449ab467da2a8a038a1f Mon Sep 17 00:00:00 2001 From: JAJHall Date: Sat, 4 Oct 2025 16:44:18 +0100 Subject: [PATCH 1/3] Added Highs::closeLogFile() and calling this before returning from the command line executable to ensure that the log file is closed --- app/RunHighs.cpp | 40 +++++++++++++++++++++++++++------------- highs/Highs.h | 13 +++++++++++-- highs/lp_data/Highs.cpp | 19 +++++++++++++++++++ 3 files changed, 57 insertions(+), 15 deletions(-) diff --git a/app/RunHighs.cpp b/app/RunHighs.cpp index 0b24fc44c0..840663e8d2 100644 --- a/app/RunHighs.cpp +++ b/app/RunHighs.cpp @@ -8,9 +8,23 @@ /**@file ../app/RunHighs.cpp * @brief HiGHS main */ +#include // For fclose + #include "Highs.h" #include "HighsRuntimeOptions.h" +int runHighsReturn(Highs& highs, const int status) { + // Close any log file explicitly + highs.closeLogFile(); + // Check that the log file has been closed + assert(highs.getOptions().log_options.log_stream == nullptr); + return status; +} + +int runHighsReturn(Highs& highs, const HighsStatus status) { + return runHighsReturn(highs, int(status)); +} + int main(int argc, char** argv) { // Create the Highs instance. Highs highs; @@ -40,31 +54,31 @@ int main(int argc, char** argv) { app.parse(argc, argv); } catch (const CLI::CallForHelp& e) { std::cout << app.help() << std::endl; - return 0; + return runHighsReturn(highs, 0); } catch (const CLI::CallForAllHelp& e) { std::cout << app.help(); - return 0; + return runHighsReturn(highs, 0); } catch (const CLI::RequiredError& e) { std::cout << "Please specify filename in .mps|.lp|.ems format." << std::endl; - return (int)HighsStatus::kError; + return runHighsReturn(highs, HighsStatus::kError); } catch (const CLI::ExtrasError& e) { std::cout << e.what() << std::endl; std::cout << "Multiple files not supported." << std::endl; - return (int)HighsStatus::kError; + return runHighsReturn(highs, HighsStatus::kError); } catch (const CLI::ArgumentMismatch& e) { std::cout << e.what() << std::endl; std::cout << "Too many arguments provided. Please provide only one." << std::endl; - return (int)HighsStatus::kError; + return runHighsReturn(highs, HighsStatus::kError); } catch (const CLI::ParseError& e) { std::cout << e.what() << std::endl; // app.exit() should be called from main. - return app.exit(e); + return runHighsReturn(highs, app.exit(e)); } if (!loadOptions(app, log_options, cmd_options, loaded_options)) - return (int)HighsStatus::kError; + return runHighsReturn(highs, HighsStatus::kError); // Open the app log file - unless output_flag is false, to avoid // creating an empty file. It does nothing if its name is "". @@ -86,13 +100,13 @@ int main(int argc, char** argv) { HighsStatus read_status = highs.readModel(cmd_options.model_file); if (read_status == HighsStatus::kError) { highsLogUser(log_options, HighsLogType::kInfo, "Error loading file\n"); - return (int)read_status; + return runHighsReturn(highs, read_status); } if (options.write_presolved_model_file != "") { // Run presolve and write the presolved model to a file HighsStatus status = highs.presolve(); - if (status == HighsStatus::kError) return int(status); + if (status == HighsStatus::kError) return runHighsReturn(highs, status); HighsPresolveStatus model_presolve_status = highs.getModelPresolveStatus(); const bool ok_to_write = model_presolve_status == HighsPresolveStatus::kNotReduced || @@ -102,18 +116,18 @@ int main(int argc, char** argv) { if (!ok_to_write) { highsLogUser(log_options, HighsLogType::kInfo, "No presolved model to write to file\n"); - return int(status); + return runHighsReturn(highs, status); } status = highs.writePresolvedModel(options.write_presolved_model_file); - return int(status); + return runHighsReturn(highs, status); } // Solve the model HighsStatus run_status = highs.run(); - if (run_status == HighsStatus::kError) return int(run_status); + if (run_status == HighsStatus::kError) runHighsReturn(highs, run_status); // Shut down task executor for explicit release of memory. // Valgrind still reachable otherwise. highs.resetGlobalScheduler(true); - return (int)run_status; + return runHighsReturn(highs, run_status); } diff --git a/highs/Highs.h b/highs/Highs.h index 2beaa37948..7545a602f6 100644 --- a/highs/Highs.h +++ b/highs/Highs.h @@ -43,10 +43,14 @@ class Highs { public: Highs(); virtual ~Highs() { - FILE* log_stream = options_.log_options.log_stream; + FILE* log_stream = this->options_.log_options.log_stream; if (log_stream != nullptr) { assert(log_stream != stdout); - fclose(log_stream); + // Was this, but only passing a copy of the pointer, so not + // closing the true log stream? + // + // fclose(log_stream); + fclose(this->options_.log_options.log_stream); } } @@ -1216,6 +1220,11 @@ class Highs { */ HighsStatus openLogFile(const std::string& log_file = ""); + /** + * @brief Close any open log file + */ + HighsStatus closeLogFile(); + /** * @brief Interpret common qualifiers to string values */ diff --git a/highs/lp_data/Highs.cpp b/highs/lp_data/Highs.cpp index fdaa1080a2..2fb0d995ca 100644 --- a/highs/lp_data/Highs.cpp +++ b/highs/lp_data/Highs.cpp @@ -4762,6 +4762,25 @@ HighsStatus Highs::openLogFile(const std::string& log_file) { return HighsStatus::kOk; } +HighsStatus Highs::closeLogFile() { + FILE* log_stream = this->options_.log_options.log_stream; + if (log_stream != nullptr) { + assert(log_stream != stdout); + // Using this, as in the original destructor of Highs, only + // passing a copy of the pointer, so not closing the true log + // stream? + // + // fclose(log_stream); + fclose(this->options_.log_options.log_stream); + // Set log_stream to nullptr to give a test whether the it has + // been closed (and avoid trying to close it again which causes an + // error). + this->options_.log_options.log_stream = nullptr; + assert(this->options_.log_options.log_stream == nullptr); + } + return HighsStatus::kOk; +} + void Highs::resetGlobalScheduler(bool blocking) { HighsTaskExecutor::shutdown(blocking); } From f65b89e1bd2a0cbf2e0ab4d3218cf974b9328c26 Mon Sep 17 00:00:00 2001 From: JAJHall Date: Sat, 4 Oct 2025 17:15:42 +0100 Subject: [PATCH 2/3] Use Highs::closeLogFile in destructor --- highs/Highs.h | 8 +++----- highs/lp_data/Highs.cpp | 16 ++++++---------- highs/lp_data/HighsOptions.cpp | 19 +++++++++++-------- 3 files changed, 20 insertions(+), 23 deletions(-) diff --git a/highs/Highs.h b/highs/Highs.h index 7545a602f6..2a171fb52f 100644 --- a/highs/Highs.h +++ b/highs/Highs.h @@ -46,11 +46,9 @@ class Highs { FILE* log_stream = this->options_.log_options.log_stream; if (log_stream != nullptr) { assert(log_stream != stdout); - // Was this, but only passing a copy of the pointer, so not - // closing the true log stream? - // - // fclose(log_stream); - fclose(this->options_.log_options.log_stream); + // Flush and close log_stream + fflush(log_stream); + fclose(log_stream); } } diff --git a/highs/lp_data/Highs.cpp b/highs/lp_data/Highs.cpp index 2fb0d995ca..0f730db2ac 100644 --- a/highs/lp_data/Highs.cpp +++ b/highs/lp_data/Highs.cpp @@ -4763,20 +4763,16 @@ HighsStatus Highs::openLogFile(const std::string& log_file) { } HighsStatus Highs::closeLogFile() { + // Work with a copy of the pointer for brevity FILE* log_stream = this->options_.log_options.log_stream; if (log_stream != nullptr) { assert(log_stream != stdout); - // Using this, as in the original destructor of Highs, only - // passing a copy of the pointer, so not closing the true log - // stream? - // - // fclose(log_stream); - fclose(this->options_.log_options.log_stream); - // Set log_stream to nullptr to give a test whether the it has - // been closed (and avoid trying to close it again which causes an - // error). + fflush(log_stream); + fclose(log_stream); + // Set the true log_stream to nullptr to give a test whether it + // has been closed (and avoid trying to close it again which + // causes an error) this->options_.log_options.log_stream = nullptr; - assert(this->options_.log_options.log_stream == nullptr); } return HighsStatus::kOk; } diff --git a/highs/lp_data/HighsOptions.cpp b/highs/lp_data/HighsOptions.cpp index 13ec2abdcb..393720d671 100644 --- a/highs/lp_data/HighsOptions.cpp +++ b/highs/lp_data/HighsOptions.cpp @@ -25,19 +25,22 @@ void highsOpenLogFile(HighsLogOptions& log_options, OptionStatus status = getOptionIndex(log_options, "log_file", option_records, index); assert(status == OptionStatus::kOk); - if (log_options.log_stream != NULL) { + if (log_options.log_stream != nullptr) { // Current log file stream is not null, so flush and close it fflush(log_options.log_stream); fclose(log_options.log_stream); + // Set the stream to null to give a test whether it has been + // closed (and avoid trying to close it again which causes an + // error) + log_options.log_stream = nullptr; } - if (log_file.compare("")) { - // New log file name is not empty, so open it, appending if - // possible + assert(!log_options.log_stream); + // If new log file name is not empty, open it, appending if possible + // + // If fopen fails then it returns nullptr, so log_file is open for + // writing or nullptr + if (log_file.compare("")) log_options.log_stream = fopen(log_file.c_str(), "a"); - } else { - // New log file name is empty, so set the stream to null - log_options.log_stream = NULL; - } OptionRecordString& option = *(OptionRecordString*)option_records[index]; option.assignvalue(log_file); } From 2e5d7eb595416e74e61c77718d9dbd0382af256d Mon Sep 17 00:00:00 2001 From: JAJHall Date: Sat, 4 Oct 2025 17:21:17 +0100 Subject: [PATCH 3/3] Now using Highs::closeLogFile() in ~Highs() --- highs/Highs.h | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/highs/Highs.h b/highs/Highs.h index 2a171fb52f..7d2c21a538 100644 --- a/highs/Highs.h +++ b/highs/Highs.h @@ -42,15 +42,7 @@ const char* highsGithash(); class Highs { public: Highs(); - virtual ~Highs() { - FILE* log_stream = this->options_.log_options.log_stream; - if (log_stream != nullptr) { - assert(log_stream != stdout); - // Flush and close log_stream - fflush(log_stream); - fclose(log_stream); - } - } + virtual ~Highs() { this->closeLogFile(); } /** * @brief Return the version as a string