Merge pull request #2558 from ERGO-Code/fix-2534

Added `Highs::closeLogFile()`, and calling this before returning from the command line executable

Author: jajhall

app/RunHighs.cpp

@@ -8,9 +8,23 @@
 /**@file ../app/RunHighs.cpp
  * @brief HiGHS main
  */
+#include <cstdio>  // 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);
 }

highs/Highs.h

@@ -42,13 +42,7 @@ const char* highsGithash();
 class Highs {
  public:
   Highs();
-  virtual ~Highs() {
-    FILE* log_stream = options_.log_options.log_stream;
-    if (log_stream != nullptr) {
-      assert(log_stream != stdout);
-      fclose(log_stream);
-    }
-  }
+  virtual ~Highs() { this->closeLogFile(); }
 
   /**
    * @brief Return the version as a string
@@ -1216,6 +1210,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
    */

highs/lp_data/Highs.cpp

@@ -4762,6 +4762,21 @@ HighsStatus Highs::openLogFile(const std::string& log_file) {
   return HighsStatus::kOk;
 }
 
+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);
+    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;
+  }
+  return HighsStatus::kOk;
+}
+
 void Highs::resetGlobalScheduler(bool blocking) {
   HighsTaskExecutor::shutdown(blocking);
 }

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);
 }