Bi streaming no debugger stuff #219

goodboy · 2021-07-05T16:45:55Z

This is a first draft of a bidirectional streaming api as discussed in #53.

This specific PR is a clone of the original #209 but with debugger changes factored out to see if we can get a clean CI run as well as a smaller patch set for reviewers. There is a follow up PR #220 which includes the debugger improvements that now use and rely on this new api.

Critique and suggestions very welcome from lurkers 😎.

continued task list from #209:

more extensive tests for closing down either side of the stream early
port _debug.py remote tty locking to context api.
tests where the consumer tasks use async for stream while sender is running independently in another trio task
~~[ ] should we add broadcasting support here for We need broadcast channels, stat. #204 ?~~ let's keep it separate.
/api.html#anyio.streams.stapled.StapledObjectStream) (obvs we won't use *Object* since we're msgpack type contrained). Actual code is here.
example in the main readme. i'm thinking that should be a big boon when compared to projects like faust and ray which (i think) are unidirectional only?

the following were deferred to #223

docs on the new apis
possibly a better internal design for tracking bidir vs unidir context usage to avoid hacky if not self._ctx._portal checking inside ReceiveMsgStream.aclose()
from Ems to bidir streaming pikers/piker#190

would be nice if in tractor we can require either a ctx arg, or a named arg with ctx in it and a type annotation of tractor.Context instead of strictly requiring a ctx arg.
move to a split SendMsgStream / ReceiveMsgStream type set and staple them together using a channel/messaging equivalent of trio.StapledStream? I'm thinking we'll make all streams a MsgStream and just don't allow send on receive only (which should be minority use case i'd imagine eventually).
- currently this would allow making the stream arg to @stream funcs a SendMsgStream instead of a Context
- turns out [anyio has something similar: StapledObjectStream](https://anyio.readthedocs.io/en/stable
- - a comment from some discussion there:
The main question was moreso about cancellation race conditions that can arise where the local channel is killed after it's sent the stop and whether or not we should wait / shield the mem chan until the msg is processed later (also presumably this is all before the sub-actor is killed).
- this really has to do with whether or not we want a channel teardown transaction eventually. I personally think right now it's not a requirement (and makes facing 2-general's more immediate). the more sane thing to address first is the naive discovery issues we have as per Multi-root discovery: pragmatic, simple consensus. #216.

outstanding windows hangs CI work, leak-over from #209

~~[ ] comment from #209 suggests it might have been the mutlti-task producer task, but removing that on 83dd8e4 shows it's still not working...~~ it was some wacky logging thing 🙄

goodboy · 2021-07-05T23:18:37Z

@chrizzFTD if you've got a second I would sure love to know why these tests hang 😂

This mostly adds the api described in #53 (comment) The first draft summary: - formalize bidir steaming using the `trio.Channel` style interface which we derive as a `MsgStream` type. - add `Portal.open_context()` which provides a `trio.Nursery.start()` remote task invocation style for setting up and tearing down tasks contexts in remote actors. - add a distinct `'started'` message to the ipc protocol to facilitate `Context.start()` with a first return value. - for our `ReceiveMsgStream` type, don't cancel the remote task in `.aclose()`; this is now done explicitly by the surrounding `Context` usage: `Context.cancel()`. - streams in either direction still use a `'yield'` message keeping the proto mostly symmetric without having to worry about which side is the caller / portal opener. - subtlety: only allow sending a `'stop'` message during a 2-way streaming context from `ReceiveStream.aclose()`, detailed comment with explanation is included. Relates to #53

Add clear teardown semantics for `Context` such that the remote side cancellation propagation happens only on error or if client code explicitly requests it (either by exit flag to `Portal.open_context()` or by manually calling `Context.cancel()`). Add `Context.result()` to wait on and capture the final result from a remote context function; any lingering msg sequence will be consumed/discarded. Changes in order to make this possible: - pass the runtime msg loop's feeder receive channel in to the context on the calling (portal opening) side such that a final 'return' msg can be waited upon using `Context.result()` which delivers the final return value from the callee side `@tractor.context` async function. - always await a final result from the target context function in `Portal.open_context()`'s `__aexit__()` if the context has not been (requested to be) cancelled by client code on block exit. - add an internal `Context._cancel_called` for context "cancel requested" tracking (much like `trio`'s cancel scope). - allow flagging a stream as terminated using an internal `._eoc` flag which will mark the stream as stopped for iteration. - drop `StopAsyncIteration` catching in `.receive()`; it does nothing.

Adds a new hard kill routine for the `trio` spawning backend.

goodboy · 2021-07-06T12:46:30Z

Looking at this last run it appears as though it's the discovery tests causing the hang now?

I think we need a manual tester to come in and check it out.

goodboy · 2021-07-06T18:07:47Z

Ok going to increment in from #220 onto the CI_increment_for_windows_bidirstreaming branch to see if we can bisect the solution 😂

goodboy · 2021-07-07T20:02:44Z

As per whatever dark magic is going on in this patch set this should solve windows CI hangs?

goodboy · 2021-07-07T20:33:02Z

Lol true 🤯 right there.

goodboy · 2021-07-08T18:06:13Z

Aight, gonna consider this a clean run since the one flaky test is just in-determinism that'll be removed in #220 😎

Last couple things I'm hoping to get in here:

readme examples of streaming
~~[ ] possibly allow passing in a nursery to .open_context()~~ deferred to Streaming API refinements #223.
~~traceback truncating to remove all our internal frames~~ move to Ctx debugger #220.

goodboy · 2021-07-31T16:24:43Z

tractor/_actor.py

@@ -378,10 +473,24 @@ async def _stream_handler(
        try:
            await self._process_messages(chan)
        finally:
+
+            # channel cleanup sequence


Ahh dang I guess we can drop this?

Oh wait, no this is a reminder comment of sorts.

goodboy force-pushed the bi_streaming_no_debugger_stuff branch from e084925 to d130f2c Compare July 5, 2021 19:04

goodboy mentioned this pull request Jul 5, 2021

Ctx debugger #220

Merged

goodboy force-pushed the bi_streaming_no_debugger_stuff branch from 25938fe to 83dd8e4 Compare July 6, 2021 11:31

goodboy mentioned this pull request Jul 6, 2021

Bidir streaming #209

Closed

9 tasks

goodboy requested review from chrizzFTD, guilledk, ryanhiebert and salotz July 6, 2021 11:46

Base automatically changed from transport_cleaning to master July 6, 2021 12:20

goodboy added 20 commits July 6, 2021 08:23

Expose @context decorator at top level

1411454

Cancel scope on stream consumer completion

4846c6d

Add basic test set

4240efc

Support passing shield at stream contruction

1f8966b

Parametrize with async for style tests

98133a9

Fix typing

08eb6bd

Be more pedantic with error handling

e311430

Add dynamic pubsub test using new bidir stream apis

e5bc07f

Expose msg stream types at top level

6559fb7

Only send stop msg if not received from far end

a2e2f7e

Support no arg to Context.started() like trio

9a4244b

Add error case

5b8b7d3

Only close recv chan if we get a ref

39b9896

Add a multi-task streaming test

3999849

Add a specially handled ContextCancelled error

eb3662f

Expose streaming components at top level

409f7f0

Specially raise a ContextCancelled for a task-context rpc

73302d9

Fix exception typing

1a69727

goodboy added 7 commits July 6, 2021 08:23

Always hard kill sub-procs on teardown

ef725c5

Adds a new hard kill routine for the `trio` spawning backend.

Don't shield on root cancel it can causes hangs

b1cd7fd

Move join event trigger to direct exit path

98bbf8e

Don't enter debug on closed resource errors

12f9875

Drop trip kwarg

7f86d63

Avoid mutate on iterate race

9ddb654

Go back to only logging crashes if no pdb gets engaged

2513c65

goodboy force-pushed the bi_streaming_no_debugger_stuff branch from f5af130 to 4157d98 Compare July 6, 2021 16:54

Skip debugger tests on windows at module level

929b6dc

goodboy force-pushed the bi_streaming_no_debugger_stuff branch 2 times, most recently from fbcd253 to 929b6dc Compare July 6, 2021 17:54

goodboy mentioned this pull request Jul 6, 2021

Ci increment for windows bidirstreaming #221

Closed

goodboy added 2 commits July 7, 2021 14:31

Flip "trace" level to "transport" level logging

31590e8

Change trace to transport level

8c927d7

goodboy added 4 commits July 8, 2021 12:48

Mypy fixes

fde52d2

Define explicit adapter level methods for mypy

25779d4

Use "pdb" level logging in debug mode

443ebea

Install test deps and py3.9 for type check job

69bbf6a

goodboy mentioned this pull request Jul 31, 2021

Streaming API refinements #223

Open

4 tasks

goodboy force-pushed the bi_streaming_no_debugger_stuff branch from 47b0d32 to a35b938 Compare July 31, 2021 15:28

Add 2-way streaming example to readme and scripts

240f591

goodboy force-pushed the bi_streaming_no_debugger_stuff branch from a35b938 to 240f591 Compare July 31, 2021 16:10

goodboy commented Jul 31, 2021

View reviewed changes

goodboy merged commit 54d8c93 into master Jul 31, 2021

goodboy deleted the bi_streaming_no_debugger_stuff branch July 31, 2021 16:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bi streaming no debugger stuff #219

Bi streaming no debugger stuff #219

goodboy commented Jul 5, 2021 •

edited

goodboy commented Jul 5, 2021

goodboy commented Jul 6, 2021

goodboy commented Jul 6, 2021

goodboy commented Jul 7, 2021

goodboy commented Jul 7, 2021

goodboy commented Jul 8, 2021 •

edited

goodboy Jul 31, 2021

goodboy Jul 31, 2021

Bi streaming no debugger stuff #219

Bi streaming no debugger stuff #219

Conversation

goodboy commented Jul 5, 2021 • edited

continued task list from #209:

the following were deferred to #223

outstanding windows hangs CI work, leak-over from #209

goodboy commented Jul 5, 2021

goodboy commented Jul 6, 2021

goodboy commented Jul 6, 2021

goodboy commented Jul 7, 2021

goodboy commented Jul 7, 2021

goodboy commented Jul 8, 2021 • edited

goodboy Jul 31, 2021

Choose a reason for hiding this comment

goodboy Jul 31, 2021

Choose a reason for hiding this comment

goodboy commented Jul 5, 2021 •

edited

goodboy commented Jul 8, 2021 •

edited