Open phr34k opened 1 year ago
I have not grokked the code yet, but unwinding sounds like correct and expected behavior.
When you cancel a stream subscription from a stream of an async*
function, the effect is that all later yield
statements behave as a return
, going through every finally
on the way out of the method body. That's deliberate, to allow the function to release resources that it no longer needs.
The Future
returned by StreamSubscription.cancel
is completed when the method body is exited.
If you don't have an oppurtunity to gracefully cancel resources, I'd argue that the order of events is incorrect. Isn't the whole point of having an oncancel callback, so you can make whatever state changes that will set-it up to break out of existing computations, and then clean-up resources.
var f = StreamSubscriber.cancel()
method body unwinding
StreamController.oncancel()
await f;
In this case it means that because of the method body unwinding, the websocket is unsubsribed (since running in asyncExpand) which causes it not process close when issued later when oncancel is called. I feel like it's more intuetive that oncancel should actually be first to be called- because from what I can tell in my situation of the async computation isn't unwinded first the close would have worked as intended.
@lrhn eventually I made this (https://gist.github.com/phr34k/620c9af014e763e6be5591eeb1d36868#file-websocket_test-dart-L78) variation of the code, to guarentee that ondisconnect callback happens in the right semantical ordering, throwing the finalizer body into it's own deferred microtask.
But the only way to correctly get the status codes, is throwing an delay of 5 seconds (websocket implementation detail). When there's no listener anymore (stack unwinding does this) it's not processing any data more incl. websocket frames in response to termination issued, instead it uses a (5s) timer to defer setting the closeReason/closeCode.
I would argue that most is just simple cause and effect from the fact it simply cannot attempt an gracefull shutdown first.
Can you point to the part that you think is not working as it should.
The entire system is a very complicated interleaving of multiple streams, some async*
, some from controllers, some affected by stream transformers or asyncExpand
.
The behavior of an async*
function is already described. Cancelling its subscription makes it exit the body in an orderly manner, the finally
s of the body doing the equivalent of the onCancel
of a stream controller to clean up, and complete the returned future when the clean-up is complete.
The behavior of asyncExpand
should be that when its subscription is cancelled, it cancels the current inner stream subscription, if one is active, and when that has completed, it cancels the outer stream subscription (which is paused while there is an inner stream subscription), and when that's done, its own cancel
call future is completed.
Both of these seem to be working as intended.
I'm not familiar enough with web-sockets or WebSocketTransformation
to know what they are supposed to do.
@lrhn i believe in my previous posts I've already touched on my observations that the current system does not allow for an gracefull shutdown of the socket
facts:
iterator.cancel
causes an active subscription to be cancelled, and propegates until the source (async* function).The websocket stream [3] get's ubsubscribed before oncancel [5] is executed, so there's no way possible to let it attempt an gracefull shutdown and the correct shutdown state of the websocket has to be obtained with implementation detail trickery [6].
Just because the invidual parts are working as intended and considered sound doesn't mean the sum of them their parts must be sound also. Evidently in this case you'd actually want the onCancel
control the canceling of the inner streams (websocket), and after onCancel's future returns terminate the stack.
So the problem is with WebSocket
?
StreamIterator.cancel
immediately cancels its subscription using StreamSubscription.cancel
, and returns the result future. That's reasonable. (I recommend using StreamQueue
over StreamIterator
, because it's nicer, but StreamIterator
does work as intended, when it's used correctly, and it is here.)
The StreamIterator
is used on the stream of a synchronous StreamController
, so calling the StreamIterator.cancel
requests a cancel from the stream controller.
The StreamController
is most likely executing an addStream
of an asyncExpand
.
The stream controller cancels by first cancelling the addStream
, and then calling its own onCancel
callback to return a future saying that cancel is done.
The asyncExpand
is internally using another stream controller and an addStream
, so cancelling the expand-stream cancels the inner stream (if one is active), then cancels the outer stream, and then it's done.
The inner stream of asyncExpand
is a web socket emitted as event by the stream created by _generate
, so that gets cancelled.
Then the outer stream is cancelled, which is the one of _generate
, paused at the yield ret;
statement. Cancelling an async*
stream subscription makes all current and following yield
statements act like a return
.
As you noticed, that means that the code immediately following that yield
is not executed.
If you need the code to be run even after the stream is cancelled, you should put the code into a finally
block too, and you should expect every yield
to be able to return.
I think everything here is working as intended.
Sort of, websockets is a (manifestation of the) problem but not convinced it is the source of the problem.
Normally with communication standards (websockets incl.) it isn't uncommon that you coordinate your shutdown before finally closing the underlying sockets.
That isnt an immediate action and can take some time. What happens is this process seems to get initiated, but lacking a subscription responses stop being processed.
If you read the websocket code you'll see all kinds of weird twists with timers inheritly because of disattached stream subscriber.
Don't get me wrong: I admit what you said isnt unreasonable. Individually each these element behaves as you'd expect.
So you tell me your thoughts if I did something unreasonable- if its a specific bug in websocket that need solving or if async/stream mechanism has some shortcomings.
On Sun, Feb 5, 2023, 22:33 Lasse R.H. Nielsen @.***> wrote:
So the problem is with WebSocket?
StreamIterator.cancel immediately cancels its subscription using StreamSubscription.cancel, and returns the result future. That's reasonable. (I recommend using StreamQueue over StreamIterator, because it's nicer, but StreamIterator does work as intended, when it's used correctly, and it is here.)
The StreamIterator is used on the stream of a synchronous StreamController, so calling the StreamIterator.cancel requests a cancel from the stream controller.
The StreamController is most likely executing an addStream of an asyncExpand. The stream controller cancels by first cancelling the addStream, and then calling its own onCancel callback to return a future saying that cancel is done.
The asyncExpand is internally using another stream controller and an addStream, so cancelling the expand-stream cancels the inner stream (if one is active), then cancels the outer stream, and then it's done.
The inner stream of asyncExpand is a web socket emitted as event by the stream created by _generate, so that gets cancelled. Then the outer stream is cancelled, which is the one of _generate, paused at the yield ret; statement. Cancelling an async* stream subscription makes all current and following yield statements act like a return. As you noticed, that means that the code immediately following that yield is not executed. If you need the code to be run even after the stream is cancelled, you should put the code into a finally block too, and you should expect every yield to be able to return.
I think everything here is working as intended.
— Reply to this email directly, view it on GitHub https://github.com/dart-lang/sdk/issues/51241#issuecomment-1418049500, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJCQ4YNT5U4SNKMRA46QSDWV7B4FANCNFSM6AAAAAAUPTVMWE . You are receiving this because you authored the thread.Message ID: @.***>
Using a stream might have been the wrong approach. If you can't pause the source, and you get events that must be reacted to, then a stream is not a good choice, because calling pause
and then cancel
can leave some events unhandled in the subscription buffer.
The source should then buffer those events while the subscription is paused, so it knows to properly dispose them if the subscription cancels.
I don't know the websocket code well enough to say whether that's even the problem, or how easy it is to fix.
Let's rope in some dart:io
people.
Steps to Reproduce
Run the sample code I provided below, it's a small unit test and the code just uses basic dependencies.
To give a brief description. What I basically did was create an generator function that generates a infinite stream of websockets, when the socket is closed, it'll reconnect while respecting randomized timeouts. I then use
asyncExpand
function, to expand all of the websockets data and expose into a singleStream<dynamic>
so that the caller will just have to listen once, and then get some notifications when a socket is disconnected or opened.Expected results:
In the code above I expect two connects and two disconnects to happen. The second (last) disconnect should be because the StreamIterator closes, and it'll close the websocket normally, logically I'd expect it close normally with 1005.
Actual results:
However the actual results, seem that the moment I call
iterator.cancel()
, the onCancel function is the last function to get called, instead it seems like it first terminates first the active_generate()
computation which is the source. What I observe is it'll break out of loop, unwinding the stack executing any remainingfinally
statements. The best analogy I think is as-if an invisible exception is thrown, and only after that it'll actually call the websocket.close().The problem that this gives is that
_generate
gets unwinded,websocket.close()
hasn't actually been called yet and the errorCode and errorStatus properties of the websocket are still null. It feels like these ordering of events are counter intuative- at least to me.moved from here: https://github.com/flutter/flutter/issues/119790