Reorder detach cleanup and detach callback

[svaarala]

This should make it easier to allow a detached callback to reattach immediately without leaving the debugger state inconsistent.

Tasks:

• Check that the detached -> attach sequence works in practice (multiple times)
• Document that the detached callback may reattach immediately
• Delay detached callback to message handling loop so that it's called between two full messages (this avoids accidentally e.g. skipping to EOM or writing to the new connection in handling code dealing with data from the previous connection)
• Releases entry

[fatcerberus]

Wait... I do this right now in minisphere. In what way can the state be left inconsistent? Could I segfault?

[svaarala]

I think the best answer to that is in the diff. Based on the previous code I think at least heap->dbg_udata would be overwritten as NULL if a non-NULL value was given in the attach call.

[fatcerberus]

It also sets dbg_detached_cb to NULL - ouch. Glad you caught this, that would have been a nasty bug to diagnose, figuring out why my detach callback wouldn't fire the second time. :)

[svaarala]

I'll verify that detached callback theory before I merge.

[svaarala]

There was one more bug, a rather trivial one: the heap->dbg_udata was NULLed before being used in the delayed detached callback. Fixed, with manual testing multiple reattaches now work cleanly.

[svaarala]

One more small issue: when the Detach has been processed, the message processing helper skips to EOM before returning to the message loop. This is incorrect for Detach when the user code reattaches inside the detach callback: we'd skip to EOM in the new connection which causes requests and responses to be out of sync. This caused some funny issues in the web UI, like local variables appearing in the breakpoint list :)

[svaarala]

Hmm, just occurred to me that if a transport is lost inside some message handling function due to an internal reason (like a parse error in the debug message stream) we'd call detach, there would be a reattach, and we'd then still be in the middle of message processing and potentially incorrectly scan to EOM on the new connection.

To avoid this DUK__SET_CONN_BROKEN() should only set a flag indicating the connection is broken and the detached callback would need to be called in the message loop, cleanly between handling two messages.

Then again the transport must be marked bad immediately - so basically DUK__SET_CONN_BROKEN() can and probably should do all processing except the detached callback call, and leaving the debug udata intact until the detach call.

[fatcerberus]

Yeah, that's the kind of thing I was worried about with #262, and why I suggested an API to "clear the table" as it were.

[svaarala]

There's no problem in the debug client side I think - as long as debug transport connection are modelled as separate objects. However, inside Duktape the state keeping is rather primitive, and there's just the heap-wide state for the current transport and it'd probably be overkill to support multiple active connections so that one could be handling a message from one debug connection while doing a handshake on another :)

[svaarala]

I think I have a working solution for that problem now -- detach callback is postponed to the message loop so that it always happens cleanly between messages.

[fatcerberus]

Yeah, I knew there was no problem on the client side - I was concerned about the Duktape end too. :)

[svaarala]

Ah, I see :)

The problem with that approach is that even if we wipe the state through an API call, Duktape will still be in the middle of processing some random request - and if we reattach within that request things will go wrong, so one would still need to delay the reattach somehow (or use longjmp-based error handling inside the debugger code throughout).

Here's the delayed detach callback fix BTW: 31d1296

Explicitly requested detach can happen from outside the message loop, concretely from duk_debugger_detach() and from duk_heap_free(). In these cases the detached callback is called directly because there's no message loop to worry about.

[svaarala]

Ok, I think this is merge ready for my part: all scenarios seem to work cleanly. I simulated the "syntax error in debug connection" by injecting an error into Eval command handling, then using Eval from the web UI, and checking that reattach works properly. (Unfortunately these are not covered by any automated regression tests right now.)

@fatcerberus Any chance you'd have time to see if this works in your reattach scenario in the near future? If so, I can wait before merging.

[fatcerberus]

I'll test it out either tonight or in the next couple of days. My debugger connects over loopback (which is special-cased in Windows) though so it might be difficult to simulate a dropped connection.

[svaarala]

Ok, sounds good - there's no particular hurry as this branch won't easily go into conflict.

[fatcerberus]

One scenario that might be worth testing is if the debugger is detached during a call to duk_debug_halt_execution()

[svaarala]

Agree - it's using duk_debug_process_messages() so there shouldn't be much difference.

[svaarala]

Ok, I tried the detach/reattach sequence by enabling "pause on uncaught" and the auto-reattach feature in "duk". Seems to work as expected.

[fatcerberus]

That's good, I'll test it out soon myself. I was going to the other day, but it turns out minisphere is currently programmed to exit on detach (mimicking MSVC's behavior), so I will have to set up a harness first.

[svaarala]

Thanks - no particular hurry, this won't conflict easily.

[svaarala]

@fatcerberus This is now rebased and merge ready (unless you have any tests you might want to run).

[fatcerberus]

Oops, i forgot about this, i tested out the reattach the other day and it worked as expected (after I actually implemented it in my client anyway :)