Sigintsaviour ci worked #315
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Oof 😂 |
This gets very close to avoiding any possible hangs to do with tty
locking and SIGINT handling minus a special case that will be detailed
below.
Summary of implementation changes:
- convert `_mk_pdb()` -> `with _open_pdb() as pdb:` which implicitly
handles the `bdb.BdbQuit` case such that debugger teardown hooks are
always called.
- rename the handler to `shield_sigint()` and handle a variety of new
cases:
* the root is in debug but hasn't been cancelled -> call
`Actor.cancel_soon()`
* the root is in debug but *has* been called (`Actor.cancel_soon()`
already called) -> raise KBI
* a child is in debug *and* has a task locking the debugger -> ignore
SIGINT in child *and* the root actor.
- if the debugger instance is provided to the handler at acquire time,
on SIGINT handling completion re-print the last pdb++ REPL output so
that the user realizes they are still actively in debug.
- ignore the unlock case where a race condition of "no task" holding the
lock causes the `RuntimeError` normally associated with the "wrong
task" doing so (not sure if this is a `trio` bug?).
- change debug logs to runtime level.
Unhandled case(s):
- a child is maybe in debug mode but does not itself have any task using
the debugger.
* ToDo: we need a way to decide what to do with
"intermediate" child actors who themselves either are not in
`debug_mode=True` but have children who *are* such that a SIGINT
won't cause cancellation of that child-as-parent-of-another-child
**iff** any of their children are in in debug mode.
Using either of `@pdb.hideframe` or `__tracebackhide__` on stdlib methods doesn't seem to work either.. This all seems to have something to do with async generator usage I think ?
None of it worked (you still will see `.__exit__()` frames on debugger entry - you'd think this would have been solved by now but, shrug) so instead wrap the debugger entry-point in a `try:` and put the SIGINT handler restoration inside `MultiActorPdb` teardown hooks. This seems to restore the UX as it was prior but with also giving the desired SIGINT override handler behaviour.
Finally! I think this may be the root issue we've been seeing in production in a client project. No idea yet why this is happening but the fault-causing sequence seems to be: - `.open_context()` in a child actor - enter the debugger via `tractor.breakpoint()` - continue from that entry via `c` command in REPL - raise an error just after inside the context task's body Looking at logging it appears as though the child thinks it has the tty but no input is accepted on the REPL and a further `ctrl-c` results in some teardown but also a further hang where both parent and child become unresponsive..
There's a bug that's triggered in the stdlib without latest `pdb++` installed; add a note for that. Further inside `wait_for_parent_stdin_hijack()` don't `.started()` until the interactor stream has been opened to avoid races when debugging this `._debug.py` module (at the least) since we usually don't want the spawning (parent) task to resume until we know for sure the tty lock has been acquired. Also, drop the random checkpoint we had inside `_breakpoint()`, not sure it was actually adding anything useful since we're (mostly) carefully shielded throughout this func.
There's no point in sending a cancel message to the remote linked task and especially no reason to block waiting on a result from that task if the transport layer is detected to be disconnected. We expect that the transport shouldn't go down at the layer of the message loop (reconnection logic should be handled in the transport layer itself) so if we detect the channel is not connected we don't bother requesting cancels nor waiting on a final result message. Why? - if the connection goes down in error the caller side won't have a way to know "how long" it should block to wait for a cancel ack or result and causes a potential hang that may require an additional ctrl-c from the user especially if using the debugger or if the traceback is not seen on console. - obviously there's no point in waiting for messages when there's no transport to deliver them XD Further, add some more detailed cancel logging detailing the task and actor ids.
The method now returns a `bool` which flags whether the transport died to the caller and allows for reporting a disconnect in the channel-transport handler task. This is something a user will normally want to know about on the caller side especially after seeing a traceback from the peer (if in tree) on console.
A hopefully significant fix here is to always avoid suppressing a SIGINT when the root actor can not detect an active IPC connections (via a connected channel) to the supposed debug lock holding actor. In that case it is most likely that the actor has either terminated or has lost its connection for debugger control and there is no way the root can verify the lock is in use; thus we choose to allow KBI cancellation. Drop the (by comment) `try`-`finally` block in `_hijoack_stdin_for_child()` around the `_acquire_debug_lock()` call since all that logic should now be handled internal to that locking manager. Try to catch a weird error around the `.do_longlist()` method call that seems to sometimes break on py3.10 and latest `pdbpp`.
Ensure that even when `pdb` resumption methods are called during a crash where `trio`'s runtime has already terminated (eg. `Event.set()` will raise) we always revert our sigint handler to the original. Further inside the handler if we hit a case where a child is in debug and (thinks it) has the global pdb lock, if it has no IPC connection to a parent, simply presume tty sync-coordination is now lost and cancel the child immediately.
goodboy
force-pushed
the
sigintsaviour_ci_worked
branch
from
cc18c84
to
7dd72e0
Compare
|
Legacy testing branch. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Purely to run CI and see what the diff is with #165 history...