Merge pull request #399 from svaarala/debugger-detach-reorder-callback

Reorder detach cleanup and detach callback

RELEASES.rst

@@ -1145,13 +1145,16 @@ Planned
   intended mainly for minimal compatibility with existing code using "const"
   (GH-360)
 
-* Add a debugger Throw notify for errors about to be thrown, and an option
-  to automatically pause before an uncaught error is thrown (GH-286, GH-347)
-
 * Add a human readable summary of object/key for rejected property operations
   to make error messages more useful for operations like "null.foo = 123;"
   (GH-210, GH-405)
 
+* Add a debugger Throw notify for errors about to be thrown, and an option
+  to automatically pause before an uncaught error is thrown (GH-286, GH-347)
+
+* Allow debugger detached callback to call duk_debugger_attach(), previously
+  this clobbered some internal state (GH-399)
+
 * Fix "debugger" statement line number off-by-one so that the debugger now
   correctly pauses on the debugger statement rather than after it (GH-347)
 

examples/cmdline/duk_cmdline.c

@@ -593,8 +593,26 @@ static int handle_interactive(duk_context *ctx) {
 
 #ifdef DUK_CMDLINE_DEBUGGER_SUPPORT
 static void debugger_detached(void *udata) {
+	duk_context *ctx = (duk_context *) udata;
+	(void) ctx;
 	fprintf(stderr, "Debugger detached, udata: %p\n", (void *) udata);
 	fflush(stderr);
+
+#if 0  /* For manual auto-reattach test */
+	duk_trans_socket_finish();
+	duk_trans_socket_init();
+	duk_trans_socket_waitconn();
+	fprintf(stderr, "Debugger connected, call duk_debugger_attach() and then execute requested file(s)/eval\n");
+	fflush(stderr);
+	duk_debugger_attach(ctx,
+	                    duk_trans_socket_read_cb,
+	                    duk_trans_socket_write_cb,
+	                    duk_trans_socket_peek_cb,
+	                    duk_trans_socket_read_flush_cb,
+	                    duk_trans_socket_write_flush_cb,
+	                    debugger_detached,
+	                    (void *) ctx);
+#endif
 }
 #endif
 
@@ -706,7 +724,7 @@ static duk_context *create_duktape_heap(int alloc_provider, int debugger, int aj
 		                    duk_trans_socket_read_flush_cb,
 		                    duk_trans_socket_write_flush_cb,
 		                    debugger_detached,
-		                    (void *) 0xbeef1234);
+		                    (void *) ctx);
 #else
 		fprintf(stderr, "Warning: option --debugger ignored, no debugger support\n");
 		fflush(stderr);

examples/debug-trans-socket/duk_trans_socket.c

@@ -29,7 +29,7 @@ static int server_sock = -1;
 static int client_sock = -1;
 
 /*
- *  Transport init
+ *  Transport init and finish
  */
 
 void duk_trans_socket_init(void) {
@@ -71,6 +71,17 @@ void duk_trans_socket_init(void) {
 	}
 }
 
+void duk_trans_socket_finish(void) {
+	if (client_sock >= 0) {
+		(void) close(client_sock);
+		client_sock = -1;
+	}
+	if (server_sock >= 0) {
+		(void) close(server_sock);
+		server_sock = -1;
+	}
+}
+
 void duk_trans_socket_waitconn(void) {
 	struct sockaddr_in addr;
 	socklen_t sz;

examples/debug-trans-socket/duk_trans_socket.h

@@ -4,6 +4,7 @@
 #include "duktape.h"
 
 void duk_trans_socket_init(void);
+void duk_trans_socket_finish(void);
 void duk_trans_socket_waitconn(void);
 duk_size_t duk_trans_socket_read_cb(void *udata, char *buffer, duk_size_t length);
 duk_size_t duk_trans_socket_write_cb(void *udata, const char *buffer, duk_size_t length);

src/duk_api_debug.c

@@ -53,6 +53,10 @@ DUK_EXTERNAL void duk_debugger_attach(duk_context *ctx,
 	const char *str;
 	duk_size_t len;
 
+	/* XXX: should there be an error or an automatic detach if
+	 * already attached?
+	 */
+
 	DUK_ASSERT_CTX_VALID(ctx);
 	DUK_ASSERT(read_cb != NULL);
 	DUK_ASSERT(write_cb != NULL);
@@ -104,7 +108,7 @@ DUK_EXTERNAL void duk_debugger_detach(duk_context *ctx) {
 	DUK_ASSERT(thr != NULL);
 	DUK_ASSERT(thr->heap != NULL);
 
-	/* Can be called muliple times with no harm. */
+	/* Can be called multiple times with no harm. */
 	duk_debug_do_detach(thr->heap);
 }
 

src/duk_debugger.c

@@ -25,22 +25,27 @@ typedef union {
 
 #define DUK__SET_CONN_BROKEN(thr) do { \
 		/* For now shared handler is fine. */ \
-		duk_debug_do_detach((thr)->heap); \
+		duk__debug_do_detach1((thr)->heap); \
 	} while (0)
 
-DUK_INTERNAL void duk_debug_do_detach(duk_heap *heap) {
-	/* Can be called muliple times with no harm. */
+DUK_LOCAL void duk__debug_do_detach1(duk_heap *heap) {
+	/* Can be called multiple times with no harm.  Mark the transport
+	 * bad (dbg_read_cb == NULL) and clear state except for the detached
+	 * callback and the udata field.  The detached callback is delayed
+	 * to the message loop so that it can be called between messages;
+	 * this avoids corner cases related to immediate debugger reattach
+	 * inside the detached callback.
+	 */
+
+	DUK_D(DUK_DPRINT("debugger transport detaching, marking transport broken"));
 
 	heap->dbg_read_cb = NULL;
 	heap->dbg_write_cb = NULL;
 	heap->dbg_peek_cb = NULL;
 	heap->dbg_read_flush_cb = NULL;
 	heap->dbg_write_flush_cb = NULL;
-	if (heap->dbg_detached_cb) {
-		heap->dbg_detached_cb(heap->dbg_udata);
-	}
-	heap->dbg_detached_cb = NULL;
-	heap->dbg_udata = NULL;
+	/* heap->dbg_detached_cb: keep */
+	/* heap->dbg_udata: keep */
 	heap->dbg_processing = 0;
 	heap->dbg_paused = 0;
 	heap->dbg_state_dirty = 0;
@@ -61,6 +66,32 @@ DUK_INTERNAL void duk_debug_do_detach(duk_heap *heap) {
 	heap->dbg_breakpoints_active[0] = (duk_breakpoint *) NULL;
 }
 
+DUK_LOCAL void duk__debug_do_detach2(duk_heap *heap) {
+	duk_debug_detached_function detached_cb;
+	void *detached_udata;
+
+	/* Safe to call multiple times. */
+
+	detached_cb = heap->dbg_detached_cb;
+	detached_udata = heap->dbg_udata;
+	heap->dbg_detached_cb = NULL;
+	heap->dbg_udata = NULL;
+
+	if (detached_cb) {
+		/* Careful here: state must be wiped before the call
+		 * so that we can cleanly handle a re-attach from
+		 * inside the callback.
+		 */
+		DUK_D(DUK_DPRINT("detached during message loop, delayed call to detached_cb"));
+		detached_cb(detached_udata);
+	}
+}
+
+DUK_INTERNAL void duk_debug_do_detach(duk_heap *heap) {
+	duk__debug_do_detach1(heap);
+	duk__debug_do_detach2(heap);
+}
+
 /*
  *  Debug connection peek and flush primitives
  */
@@ -1714,6 +1745,10 @@ DUK_LOCAL void duk__debug_process_message(duk_hthread *thr) {
 			break;
 		}
 		case DUK_DBG_CMD_DETACH: {
+			/* The actual detached_cb call is postponed to message loop so
+			 * we don't need any special precautions here (just skip to EOM
+			 * on the already closed connection).
+			 */
 			duk__debug_handle_detach(thr, heap);
 			break;
 		}
@@ -1870,6 +1905,17 @@ DUK_INTERNAL duk_bool_t duk_debug_process_messages(duk_hthread *thr, duk_bool_t
 		}
 
 		duk__debug_process_message(thr);
+
+		if (thr->heap->dbg_read_cb == NULL) {
+			/* Became detached during message handling (perhaps because
+			 * of an error or by an explicit Detach).  Call detached
+			 * callback here, between messages, to avoid confusing the
+			 * broken connection and a possible replacement (which may
+			 * be provided by an instant reattach inside the detached
+			 * callback).
+			 */
+			duk__debug_do_detach2(thr->heap);
+		}
 		if (thr->heap->dbg_state_dirty) {
 			/* Executed something that may have affected status,
 			 * resend.

website/api/duk_debugger_attach.yaml

@@ -21,6 +21,14 @@ summary: |
   <code>read_flush_cb</code> and <code>write_flush_cb</code> callbacks are
   optional.  Unimplemented callbacks are indicated using a <code>NULL</code>.</p>
 
+  <div class="note">
+  Since Duktape 1.4.0 the debugger detached callback is allowed to call
+  <code><a href="#duk_debugger_attach">duk_debugger_attach()</a></code>
+  to immediately reattach the debugger.  In Duktape 1.3.0 and before the
+  immediate reattach potentially caused
+  <a href="https://github.com/svaarala/duktape/pull/399">some issues</a>.
+  </div>
+
 example: |
   duk_debugger_attach(ctx,
                       my_read_cb,