Reworked handling of fatal errors:

	Such errors are no longer written immediately to error stream but instead simply throw. Only uncaught exceptions are printed to the error stream through a new terminate handler.
	The terminate handler also closes the error stream file (if present) so that fatal errors will be written to disk.
Chimbuko now gracefully handles instances where a counter event is detected without a corresponding counter name string in the map and no longer aborts

Author: giltirn

app/driver.cpp

@@ -1,4 +1,3 @@
-// #include "chimbuko/AD.hpp"
 #include "chimbuko/chimbuko.hpp"
 #include "chimbuko/verbose.hpp"
 #include "chimbuko/util/string.hpp"

include/chimbuko/util/error.hpp

@@ -28,10 +28,16 @@ namespace chimbuko{
     void recoverable(const std::string &msg, const std::string &func, const std::string &file,  const unsigned long line);
 
     /**
-     * @brief Signal a fatal error
+     * @brief Signal a fatal error. Not written to output stream unless either uncaught (which will also call abort) or manually flushed
      */
     void fatal(const std::string &msg, const std::string &func, const std::string &file,  const unsigned long line);
 
+    /**
+     * @brief Manually flush an exception to the output stream
+     */    
+    void flushError(std::exception_ptr e);
+    void flushError(const std::exception &e);
+
   private:
     std::string getErrStr(const std::string &type, const std::string &msg, const std::string &func, const std::string &file,  const unsigned long line) const;
 
@@ -40,7 +46,17 @@ namespace chimbuko{
     std::mutex m_mutex;
   };
 
-  extern ErrorWriter g_error; /**< Global instance of ErrorWriter*/
+  /**
+   * @brief The global error writer instance
+   */
+  ErrorWriter & Error();
+   
+  /**
+   * @brief For fatal errors we delay writing the error to the output stream in case it is caught. This terminate handler ensures it is written
+   *
+   * After flushing the error the handler calls the terminateHandlerAbortAction above
+   */
+  void writeErrorTerminateHandler();
 
   /**
    * @brief Set the error output of the global error writer to a stream and specify the rank
@@ -57,12 +73,12 @@ namespace chimbuko{
    * @brief Signal a recoverable error
    */
 #define recoverable_error(MSG) \
-  { g_error.recoverable(MSG, __func__, __FILE__, __LINE__); }
+  { chimbuko::Error().recoverable(MSG, __func__, __FILE__, __LINE__); }
 
   /**
    * @brief Signal a fatal error
    */
 #define fatal_error(MSG) \
-  { g_error.fatal(MSG, __func__, __FILE__, __LINE__); }
+  { chimbuko::Error().fatal(MSG, __func__, __FILE__, __LINE__); }
 
 }

src/chimbuko.cpp

@@ -367,7 +367,11 @@ void Chimbuko::extractCounters(int rank, int step){
 	       EventDataType::COUNT,
 	       c,
 	       eventID(rank, step, c));    
-    m_counter->addCounter(ev);
+    try{
+      m_counter->addCounter(ev);
+    }catch(const std::exception &e){
+      recoverable_error(std::string("extractCounters failed to register counter event :\"") + ev.get_json().dump() + "\"");
+    }
   }
   m_perf.add("ad_extract_counters_get_register_ms", timer.elapsed_ms());
 } 

src/util/error.cpp

@@ -5,7 +5,8 @@
 #include <iostream>
 #include <sstream>
 #include <fstream>
-
+#include <exception>
+#include <memory>
 
 namespace chimbuko{
 
@@ -17,8 +18,6 @@ namespace chimbuko{
   }
   void ErrorWriter::fatal(const std::string &msg, const std::string &func, const std::string &file,  const unsigned long line){
     std::string err = getErrStr("FATAL", msg, func, file, line);      
-    std::lock_guard<std::mutex> _(m_mutex);
-    *m_ostream << err;
     throw std::runtime_error(err);
   }
 
@@ -31,21 +30,57 @@ namespace chimbuko{
     return ss.str();
   }
 
-  ErrorWriter g_error;
+  void ErrorWriter::flushError(std::exception_ptr e){
+    if(e){
+      try{ //this is necessary to get the exception type
+	rethrow_exception(e); 
+      }catch(const std::exception &ev){
+	flushError(ev);
+      }
+    }
+  }
+  void ErrorWriter::flushError(const std::exception &e){
+    std::lock_guard<std::mutex> _(m_mutex);
+    (*m_ostream) << e.what();
+  }
+
+  ErrorWriter & Error(){
+    static bool first = true;
+    static ErrorWriter e;
+    
+    //Register the terminate handler
+    if(first){
+      std::set_terminate(writeErrorTerminateHandler);
+      first = false;
+    }
+    return e;
+  }
 
   void set_error_output_stream(const int rank, std::ostream *strm){
-    g_error.setRank(rank);
-    g_error.setStream(strm);
+    Error().setRank(rank);
+    Error().setStream(strm);
   }
 
+  std::unique_ptr<std::ofstream> & error_output_file(){ static std::unique_ptr<std::ofstream> u; return u; }
+
   void set_error_output_file(const int rank, const std::string &file_stub){
     std::string filename = stringize("%s.%d",file_stub.c_str(),rank);
-    static std::ofstream f(filename);
-    if(f.fail()) throw std::runtime_error("set_error_output_file could not open file " + filename);
-    g_error.setRank(rank);
-    g_error.setStream(&f);
+    error_output_file().reset(new std::ofstream(filename));   
+    if(error_output_file()->fail()) throw std::runtime_error("set_error_output_file could not open file " + filename);
+    Error().setRank(rank);
+    Error().setStream(error_output_file().get());
   }
 
-
+  void writeErrorTerminateHandler(){
+    if( auto exc = std::current_exception() ) { 
+      Error().flushError(exc);
+    }
+    
+    //Make sure to close the error output file first because abort does not destruct static objects
+    if(error_output_file()) error_output_file()->close();
+    
+    std::abort();
+  }
+  
 
 }

test/unit_tests/ad/ADCounter.cpp

@@ -61,4 +61,15 @@ TEST(ADCounterTest, CounterAddedCorrectly){
   EXPECT_EQ(idctrs.size(), 1);
   EXPECT_EQ(*idctrs.begin(), clist.begin());
 
+  //Try adding a counter event not in the counter list and ensure it throws an error
+  Event_t ev2 = createCounterEvent_t(pid, rid, tid, 101, value, ts);
+  bool caught_error = false;
+  try{
+    counters.addCounter(ev2);
+  }catch(const std::exception &e){
+    std::cout << "Caught expected exception: " << e.what() << std::endl;
+    caught_error = true;
+  }
+  EXPECT_TRUE(caught_error);
+
 }

test/unit_tests/util/error.cpp

@@ -9,14 +9,15 @@ std::string removeDateTime(const std::string &from){
   if(pos == std::string::npos) return from;
   return from.substr(pos+2);
 }
-  
+
+void donothing(){}  
 
 TEST(TestError, runTests){
   //Test with default rank
   {
     std::stringstream ss;
-    g_error.setStream(&ss);
-    g_error.recoverable("hello", "test_func", "this_file", 129);
+    Error().setStream(&ss);
+    Error().recoverable("hello", "test_func", "this_file", 129);
 
     std::string e = removeDateTime(ss.str());
     EXPECT_EQ(e, "Error (recoverable) : test_func (this_file:129) : hello\n");
@@ -26,7 +27,7 @@ TEST(TestError, runTests){
   {
     std::stringstream ss;
     set_error_output_stream(22, &ss);
-    g_error.recoverable("hello", "test_func", "this_file", 129);
+    Error().recoverable("hello", "test_func", "this_file", 129);
 
     std::string e = removeDateTime(ss.str());
     EXPECT_EQ(e, "Error (recoverable) rank 22 : test_func (this_file:129) : hello\n");
@@ -53,31 +54,36 @@ TEST(TestError, runTests){
     std::stringstream ss;
     set_error_output_stream(22, &ss);
     bool caught = false;
-    std::string thrown_err;
-    unsigned long line = __LINE__ + 2;
+    unsigned long line = __LINE__ + 3;
+    std::runtime_error thrown_err("pooh");
     try{
       fatal_error("hello");
-    }catch(std::exception &e){
-      thrown_err = removeDateTime(e.what());
+    }catch(const std::runtime_error &e){
+      std::cout << "Caught " << e.what() << std::endl;
+      thrown_err = e;
       caught = true;
     }
 
     EXPECT_EQ(caught, true);
 
-    std::string e = removeDateTime(ss.str());
-
     std::stringstream expect;
     expect << "Error (FATAL) rank 22 : " << __func__ << " (" << __FILE__ << ":" << line <<") : hello\n";
-    
-    std::cout << e << std::endl;
-    std::cout << thrown_err << std::endl;
-
-    EXPECT_EQ(e, expect.str());
-    EXPECT_EQ(thrown_err, expect.str());
-  }
 
+    std::string thrown_err_str = removeDateTime(thrown_err.what());
 
+    std::cout << thrown_err_str << std::endl;
+    EXPECT_EQ(thrown_err_str, expect.str());
+   
+    //Normally fatal errors are only written to the error stream if they are uncaught
+    //As we caught the error above it will not be in the error stream unless we do an explicit flush
+    EXPECT_EQ(ss.str().size(), 0);
 
+    Error().flushError(thrown_err);
 
+    std::string e = removeDateTime(ss.str());
+   
+    std::cout << e << std::endl;
 
+    EXPECT_EQ(e, expect.str());
+  }
 }