Merged
Conversation
Owner
Author
|
Ok so same results as #217 so we're approaching some kind of sanity. This somehow means some patch from the bidir streaming, which is causing the windows run to hang, is somehow fixed by some debugger related patch that uses the new context/bidir streaming apis? This is extremely strange since the debugger nor the tests for it should not be getting triggered at all on windows 🤯 Gonna start spelunking in diffs again and likely put up a diff branch to basically |
Owner
Author
|
Yeah I honestly feel like I'm losing my mind..
-> The CI without this patch set hangs indefinitely. This is like the problem of evil for testing.. |
This was referenced Jul 6, 2021
Merged
goodboy
force-pushed
the
bi_streaming_no_debugger_stuff
branch
3 times, most recently
from
fbcd253 to
929b6dc
Compare
goodboy
force-pushed
the
ctx_debugger
branch
2 times, most recently
from
a01d7ff to
b24477f
Compare
goodboy
force-pushed
the
bi_streaming_no_debugger_stuff
branch
from
a35b938 to
240f591
Compare
A context is the natural fit (vs. a receive stream) for locking the root proc's tty usage via it's `.started()` sync point. Simplify the `_breakpoin()` routine to be a simple async func instead of all this "returning a coroutine" stuff from before we decided that `tractor.breakpoint()` must be async. Use `runtime` level for locking logging making it easier to trace.
If the root calls `trio.Process.kill()` on immediate child proc teardown when the child is using pdb, we can get stdstreams clobbering that results in a pdb++ repl where the user can't see what's been typed. Not killing such children on cancellation / error seems to resolve this issue whilst still giving reliable termination. For now, code that special path until a time it becomes a problem for ensuring zombie reaps.
Finally this makes a cancelled root actor nursery not clobber child tasks which request and lock the root's tty for the debugger repl. Using an edge triggered event which is set after all fifo-lock-queued tasks are complete, we can be sure that no lingering child tasks are going to get interrupted during pdb use and tty lock acquisition. Further, even if new tasks do queue up to get the lock, the root will incrementally send cancel msgs to each sub-actor only once the tty is not locked by a (set of) child request task(s). Add shielding around all the critical sections where the child attempts to allocate the lock from the root such that it won't be disrupted from cancel messages from the root after the acquire lock transaction has started.
We may get multiple re-entries to debugger by `bp_forever` sub-actor now since the root will incrementally try to cancel it only when the tty lock is not held.
Closed
goodboy
commented
Aug 1, 2021
| fast fail test with a context. | ||
| ensure the partially initialized sub-actor process | ||
| doesn't cause a hang on error/cancel of the parent | ||
| nrusery. |
goodboy
commented
Aug 1, 2021
tests/test_debugger.py
Outdated
| # NOTE: previously since we did not have clobber prevention | ||
| # in the root actor this final resume could result in the debugger | ||
| # tearing down since both child actors would be cancelled and it was | ||
| # unlikely that `bp_forever` would re-acquire the tty loack again. |
goodboy
commented
Aug 1, 2021
| # spawn both actors | ||
| portal = await n.run_in_actor(key_error) | ||
|
|
||
| # XXX: originally a bug causes by this |
goodboy
commented
Aug 1, 2021
tests/test_debugger.py
Outdated
|
|
||
| # startup time can be iffy | ||
| time.sleep(1) | ||
| # time.sleep(1) |
goodboy
commented
Aug 1, 2021
tractor/_debug.py
Outdated
| async def _acquire_debug_lock( | ||
| uid: Tuple[str, str] | ||
| ) -> AsyncIterator[trio.StrictFIFOLock]: | ||
| '''Acquire a actor local FIFO lock meant to mutex entry to a local |
Owner
Author
There was a problem hiding this comment.
maybe use breakpoint instead of entry point
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Clone of #217 but with commits factored to pair with #219.
Again trying to find the source commits that breaks windows CI...
In theory this should have the same CI results as #217 but who knows any more 😂