Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reorder detach cleanup and detach callback #399

Merged
merged 6 commits into from Nov 3, 2015

Conversation

@svaarala
Copy link
Owner

@svaarala svaarala commented Oct 12, 2015

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
@svaarala svaarala added this to the v1.4.0 milestone Oct 12, 2015
@fatcerberus
Copy link
Contributor

@fatcerberus fatcerberus commented Oct 12, 2015

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

@svaarala
Copy link
Owner Author

@svaarala svaarala commented Oct 12, 2015

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
Copy link
Contributor

@fatcerberus fatcerberus commented Oct 12, 2015

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
Copy link
Owner Author

@svaarala svaarala commented Oct 12, 2015

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

@svaarala
Copy link
Owner Author

@svaarala svaarala commented Oct 27, 2015

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
Copy link
Owner Author

@svaarala svaarala commented Oct 27, 2015

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 svaarala force-pushed the debugger-detach-reorder-callback branch from 982fa10 Oct 27, 2015
@svaarala
Copy link
Owner Author

@svaarala svaarala commented Oct 27, 2015

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
Copy link
Contributor

@fatcerberus fatcerberus commented Oct 27, 2015

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
Copy link
Owner Author

@svaarala svaarala commented Oct 27, 2015

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
Copy link
Owner Author

@svaarala svaarala commented Oct 27, 2015

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
Copy link
Contributor

@fatcerberus fatcerberus commented Oct 27, 2015

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

@svaarala svaarala force-pushed the debugger-detach-reorder-callback branch 2 times, most recently Oct 27, 2015
@svaarala
Copy link
Owner Author

@svaarala svaarala commented Oct 27, 2015

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 svaarala force-pushed the debugger-detach-reorder-callback branch to b10bb56 Oct 27, 2015
@svaarala
Copy link
Owner Author

@svaarala svaarala commented Oct 27, 2015

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.

@svaarala svaarala added the mergeready label Oct 27, 2015
@fatcerberus
Copy link
Contributor

@fatcerberus fatcerberus commented Oct 27, 2015

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
Copy link
Owner Author

@svaarala svaarala commented Oct 27, 2015

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

@fatcerberus
Copy link
Contributor

@fatcerberus fatcerberus commented Oct 28, 2015

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

@svaarala
Copy link
Owner Author

@svaarala svaarala commented Oct 28, 2015

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

@svaarala
Copy link
Owner Author

@svaarala svaarala commented Oct 29, 2015

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

@svaarala svaarala closed this Oct 29, 2015
@svaarala svaarala reopened this Oct 29, 2015
@fatcerberus
Copy link
Contributor

@fatcerberus fatcerberus commented Oct 29, 2015

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
Copy link
Owner Author

@svaarala svaarala commented Oct 29, 2015

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

svaarala added 6 commits Sep 16, 2015
This should make it easier to allow a detached callback to reattach
immediately without leaving the debugger state inconsistent.

Requires careful handling of the detach/reattach sequence:

- Both the detached callback and its udata must be stashed, state cleared,
  and only then the detached callback can be called.

- When the Detach command has been handled, must avoid skipping to EOM
  because that would happen in a potentially new connection which would
  cause that connection to be out of sync.  Returning to the message loop
  is fine, and since duk_debugger_attach() marks the state dirty, a Status
  will be immediately sent (followed by the handshake line, sent inline by
  duk_debugger_attach()).
Without this it's possible for the following to happen:

- The debug transport gets broken by e.g. a syntax error during message
  handling.

- The detached_cb is called when marking transport bad.  The detached
  callback immediately reattaches.

- The debug command processing continues, potentially reading from and
  writing to the *new connection* which causes the connection to go
  out of sync.

Delaying the detached_cb call to the message loop, right after one message
has been fully processed, avoids this issue and keeps the connections
cleanly separated.

The solution is a bit awkward: duk_debug_do_detach() is split into two.
Outside callers (duk_debugger_detach(), duk_heap_free()) still have a single
function helper to hide the split.
@svaarala svaarala force-pushed the debugger-detach-reorder-callback branch from b10bb56 to c36d48a Nov 3, 2015
@svaarala
Copy link
Owner Author

@svaarala svaarala commented Nov 3, 2015

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

@fatcerberus
Copy link
Contributor

@fatcerberus fatcerberus commented Nov 3, 2015

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 :)

svaarala added a commit that referenced this pull request Nov 3, 2015
Reorder detach cleanup and detach callback
@svaarala svaarala merged commit f8619e1 into master Nov 3, 2015
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@svaarala svaarala deleted the debugger-detach-reorder-callback branch Nov 3, 2015
@svaarala svaarala removed the mergeready label Nov 3, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.