Issue #1173 - Default behavior for catch-all blocks

src/plugin/PluginDebug.cpp

Index: src/plugin/PluginDebug.cpp
===================================================================
--- a/src/plugin/PluginDebug.cpp
+++ b/src/plugin/PluginDebug.cpp
@@ -16,11 +16,53 @@
  */
 
 #include "PluginStdAfx.h"
-
 #include "PluginDebug.h"
+#include "Exception.h"
 #include "PluginMutex.h"
 #include "PluginSettings.h"
 
+#if (defined ENABLE_DEBUG_INFO)
+struct EntryPointHandlers
+{
+  static void Unknown(std::string name)
+  {
+    CPluginDebug::Debug("Unknown exception in catch-all for entry point " + name);
+  }
+  static void Exception(std::exception& ex, std::string name)
+  {
+    CPluginDebug::DebugOrdinaryException(ex,
+      PLUGIN_ERROR_ENTRY_POINT, PLUGIN_ERROR_ENTRY_POINT_CATCHALL_EXCEPTION,
+      "std::exception in catch-all for entry point " + name);
+  }
+  static void LogicError(std::logic_error& ex, std::string name)
+  {
+    CPluginDebug::DebugOrdinaryException(ex,
+      PLUGIN_ERROR_ENTRY_POINT, PLUGIN_ERROR_ENTRY_POINT_CATCHALL_EXCEPTION,
+      "std::logic_error in catch-all for entry point " + name);
+  }
+  static void RuntimeError(std::runtime_error& ex, std::string name)
+  {
+    CPluginDebug::DebugOrdinaryException(ex,
+      PLUGIN_ERROR_ENTRY_POINT, PLUGIN_ERROR_ENTRY_POINT_CATCHALL_EXCEPTION,
+      "std::runtime_error in catch-all for entry point " + name);
+  }
+  static void SystemError(std::system_error& ex, std::string name)
+  {
+    CPluginDebug::DebugSystemException(ex,
+      PLUGIN_ERROR_ENTRY_POINT, PLUGIN_ERROR_ENTRY_POINT_CATCHALL_EXCEPTION,
+      "std::system_error in catch-all for entry point " + name);
+  }
+};
+#endif
+
+void EntryPointExceptionDefault(std::string name)

[Oleksandr, 2015/03/05 07:18:55]

I'd prefer 'DefaultExceptionHander' instead. This is not limited in any way to
only entry points.

[Eric, 2015/03/05 14:58:04]

As a small point of language, there's no need to use "Handler" (or "Behavior",
my previous name) as a component of these names. It's completely clear from
context what they're doing, and the extra name doesn't add anything.

I had named this function with "EntryPoint" because entry points are special and
deserve special behavior. They sit at the root of the call stack and must not
rethrow. The policy I'd like to see is that exceptions should be handled
explicitly before they reach the catch-all exception handler of an entry point,
and thus that any exception that _is_ caught there is evidence of missing logic
elsewhere. At the start, this could be as simple as a magic string in the debug
log, but there should be some distinction. Using a separate function also
provides a distinct location to place a breakpoint.

To address your point, I'll add an unadorned 'ExceptionDefault' that can be used
elsewhere. Its log message won't have a special logging annotation. Personally,
I think that the default for non-root exceptions should be to rethrow, in
addition to logging, rather than only logging and thus suppressing the
exception. I've not done that in the present change set, since it's slightly
premature. We can simply add a 'throw' statement with no operand after
'ExceptionDefault' for now to get this behavior.

+{
+#if (defined ENABLE_DEBUG_INFO)
+  CatchAllVoid<EntryPointHandlers>::Handler(name);
+#endif
+}
+
+
 class CPluginDebugLock : public CPluginMutex
 {
 
@@ -49,6 +91,14 @@
   DEBUG_ERROR_LOG(ex.code().value(), errorId, errorSubid, message);
 }
 
+void CPluginDebug::DebugOrdinaryException(const std::exception& ex, int errorId, int errorSubid, const std::string& description)
+{
+  std::string message = description + ", " + ex.what();
+  DEBUG_ERROR_LOG(0, errorId, errorSubid, message);
+}
+
+
+
 #ifdef ENABLE_DEBUG_INFO
 
 void DebugLegacy(const CString& text, DWORD dwProcessId, DWORD dwThreadId)