re-entrant coffeecatch

[ghazel]

Occasionally, it makes sense to wrap a callback with coffeecatch. If that callback happens to be called within an existing coffeecatch block, an error occurs -- the first block to finish calls pthread_setspecific(native_code_thread, NULL), and the second block to finish fails at assert(t != NULL);.

It would be handy if coffeecatch supported nested blocks on the same thread. Presumably the right thing to do is just ignore nested calls, and let them propagate up to the outermost block.

[ghazel]

Here's a swing at it. Maybe it could be easier, but I'm not familiar with the internals:

diff --git a/coffeecatch.c b/coffeecatch.c
index 256a1a0..b8e9b49 100644
--- a/coffeecatch.c
+++ b/coffeecatch.c
@@ -261,6 +134,7 @@ typedef struct native_code_handler_struct {
   /* Restore point context. */
   sigjmp_buf ctx;
   int ctx_is_set;
+  int reenter;

   /* Alternate stack. */
   char *stack_buffer;
@@ -1353,6 +1227,15 @@ void coffeecatch_get_backtrace_info(void (*fun)(void *arg,
   }
 }

+int coffeecatch_inside() {
+  native_code_handler_struct *const t = coffeecatch_get();
+  if (t != NULL && t->reenter > 0) {
+    t->reenter++;
+    return 1;
+  }
+  return 0;
+}
+
 /**
  * Calls coffeecatch_handler_setup(1) to setup a crash handler, mark the
  * context as valid, and return 0 upon success.
@@ -1361,6 +1244,8 @@ int coffeecatch_setup() {
   if (coffeecatch_handler_setup(1) == 0) {
     native_code_handler_struct *const t = coffeecatch_get();
     assert(t != NULL);
+    assert(t->reenter == 0);
+    t->reenter = 1;
     t->ctx_is_set = 1;
     return 0;
   } else {
@@ -1374,8 +1259,12 @@ int coffeecatch_setup() {
 void coffeecatch_cleanup() {
   native_code_handler_struct *const t = coffeecatch_get();
   assert(t != NULL);
-  t->ctx_is_set = 0;
-  coffeecatch_handler_cleanup();
+  assert(t->reenter > 0);
+  t->reenter--;
+  if (t->reenter == 0) {
+    t->ctx_is_set = 0;
+    coffeecatch_handler_cleanup();
+  }
 }

 sigjmp_buf* coffeecatch_get_ctx() {
diff --git a/coffeecatch.h b/coffeecatch.h
index 3aa00dd..1f6d1b7 100644
--- a/coffeecatch.h
+++ b/coffeecatch.h
@@ -208,12 +208,14 @@ extern int coffeecatch_cancel_pending_alarm(void);

 /** Internal functions & definitions, not to be used directly. **/
 #include <setjmp.h>
+extern int coffeecatch_inside(void);
 extern int coffeecatch_setup(void);
 extern sigjmp_buf* coffeecatch_get_ctx(void);
 extern void coffeecatch_cleanup(void);
 #define COFFEE_TRY()                                \
-  if (coffeecatch_setup() == 0                      \
-      && sigsetjmp(*coffeecatch_get_ctx(), 1) == 0)
+  if (coffeecatch_inside() || \
+      (coffeecatch_setup() == 0 \
+       && sigsetjmp(*coffeecatch_get_ctx(), 1) == 0))
 #define COFFEE_CATCH() else
 #define COFFEE_END() coffeecatch_cleanup()
 /** End of internal functions & definitions. **/

[xroche]

Patch looks good - would you mind sending a pull request ?

[xroche]

Thanks.

On a second thought however, there is a very annoying issue here: reentrency means that the native code is at some point calling a Java method. This will lead to have coffeecatch being monitoring the code while executing regular Java bytecode, and this can cause troubles, because the JIT engine will itself produce various signals (page faults to bytecode to be generated for example)

So the real solution is to actually notify coffeecatch that we are about to call Java code, and temporarily disable handlers.

I'll try to find a clean solution for that, but unfortunately the proposed patch is not sufficient :(

[ghazel]

Ah, that makes sense.

From a user perspective, it would probably also make more sense for the inner-most coffeecatch block to catch the error, instead of the outer-most. That would more closely mimic try/catch. Not sure if that makes your solution easier or harder, but maybe the right time to consider it.

[ghazel]

Also, perhaps the jsig library would be of use? http://docs.oracle.com/javase/6/docs/technotes/guides/vm/signal-chaining.html