apple / swift-async-algorithms

Async Algorithms for Swift
Apache License 2.0
3.04k stars 151 forks source link

A throttled `AsyncSequence` doesn't yield its parent sequence's final value #248

Closed gregcotten closed 1 year ago

gregcotten commented 1 year ago

Not sure if this is expected behavior.

I make an AsyncStream called stream, and a throttled sequence from that stream throttledStream. I yield values into stream for a while, and eventually call finish() on its continuation. All the while in the background I am iterating through throttledStream - when finish() gets called on stream, my iteration through throttledStream finishes, but I don't get the final value that was sent to stream.

Sorry this is kind of a complicated way to explain the issue, but I will make a reproducible example project if this behavior sounds like it is indeed not intended!

gregcotten commented 1 year ago

I should add, I guess it's the "penultimate" value I'm looking for - I realize that nil is the final value in a "finished" AsyncStream

twittemb commented 1 year ago

Is it a throttle with “latest” ? If not then it can be normal that the penultimate element is skipped.

gregcotten commented 1 year ago

Is it a throttle with “latest” ? If not then it can be normal that the penultimate element is skipped.

Yes, latest = true, just calling let throttledStream = stream.throttle(for: .seconds(1), latest: true), for example.

twittemb commented 1 year ago

@gregcotten

Keep in mind that AsyncSequence is a pull mechanism whereas Combine is a push mechanism for instance. It means that the throttling is applied to the pace of the consumer, not the producer.

in your case, let’s say you have a throttling duration of 1s, here is what I suspect is happening at the end of the timeline:

|
| x -> last received element by the consumer, a new throttling start time is set, ending in 1s
| y -> new element is discarded because it is too close to ‘x’ (the time elapsed since the start is < 1s)
|
| z -> last element is discarded because it is too close to ‘x’ (the time elapsed since the start is < 1s)
|
| nil -> the upstream sequence is finished before the theoretical end of this throttling session
|
| ——> theoretical end for the current throttling session (start + 1s)
V

In the end the consumer doesn’t receive the penultimate element but the last that matches a throttling session’s end, that is to say ‘x’.

I hope this helps. @FranzBusch could also chime in to confirm or not (he made the algorithm I think).

gregcotten commented 1 year ago

Yes, this behavior makes sense to me, but I think it might be nice to find a way to always get the penultimate value somehow... Would a PR be welcome for this?

twittemb commented 1 year ago

Yeah I can see how that would be useful. Let’s say you throttle a text field (like a search field), with the current implementation you would not have the last character … which would be wrong search wise.

@FranzBusch what’s your take on that ? Should we switch to another implementation that:

It would be much closer to what throttling does in push based system … but with the drawback of not be based on the consumer pace.

FranzBusch commented 1 year ago

Couldn't we just change the implementation of throttle to check if there is a latest value when the upstream returns nil and then return the latest value. IMO we could also make this the default but would like to get @phausler on this as well.

gregcotten commented 1 year ago

Couldn't we just change the implementation of throttle to check if there is a latest value when the upstream returns nil and then return the latest value. IMO we could also make this the default but would like to get @phausler on this as well.

That makes sense to me. Would the final value still be throttled or debounced at the specified interval, or would we break the contract for the "final" value?

FranzBusch commented 1 year ago

That makes sense to me. Would the final value still be throttled or debounced at the specified interval, or would we break the contract for the "final" value?

Good question. I would like to understand what the current standard in other frameworks is. If we could get a little comparison going here that would be nice to make an informed choice.

gregcotten commented 1 year ago

@FranzBusch My initial thought would be to respect the specified throttle interval when deciding when to emit the final value.

phausler commented 1 year ago

Combine for example used the rule for throttle and debounce: that the iterative output (not the signals to determine failure or finishedness) are the rate limited things. So specifically for throttle; it means that even the last value should not emit any faster than the requested throttling rate.

The intent for folks using throttle is that they don't ever want a rate beyond a given value. Making the last value ignore that would break folks expectations.

twittemb commented 1 year ago

It might be difficult to achieve with the current implementation since we have no way to determine that a value is the final value. We cannot postpone the emitting until the throttling duration is finished.

That's why I suggested to rely on a tick mechanism but it makes things significantly more complex and changes the way the upstream sequence is being iterated over.

FranzBusch commented 1 year ago

Thanks for clarifying the behaviour @phausler. I think that makes sense but it also means we will be dropping the final values in some cases which IMO is correct. If you throttle you have to live with the fact that you might be losing values which you are already due to the rate limiting.

gregcotten commented 1 year ago

@FranzBusch that wasn't how I interpreted @phausler's comment. I took it to mean we should always emit the final value, but only after the specified throttle interval has elapsed.

twittemb commented 1 year ago

@FranzBusch it means that if you throttle a TextField you might loose the last value, which will be an issue when performing a search for instance.

phausler commented 1 year ago

no, I meant that the value is not expected to be emitted earlier than the interval, termination on the other hand is not a participant of that emission so therefore it can happen before the interval.

Per text fields; I think (just like other button presses) debounce is usually the right tool. Sadly we don't have any capacitors handy ;)

gregcotten commented 1 year ago

My issue is that I want to update the progress of a long running task, and would like to not miss the final "I'm at 100% complete" progress update ever, which seems like a fair request.

twittemb commented 1 year ago

Per text fields; I think (just like other button presses) debounce is usually the right tool. Sadly we don't have any capacitors handy ;)

yep sure. I was just trying to illustrate my point with a real-life use case but I still think we should not be able to skip the last element since some use cases will rely on that.

gregcotten commented 1 year ago

no, I meant that the value is not expected to be emitted earlier than the interval, termination on the other hand is not a participant of that emission so therefore it can happen before the interval.

Per text fields; I think (just like other button presses) debounce is usually the right tool. Sadly we don't have any capacitors handy ;)

I'm fairly certain the current debounce implementation will not emit the final value in all cases.

gregcotten commented 1 year ago

Re: textfield search, not all cases you want to debounce. Throttling can be acceptable in certain use cases.

phausler commented 1 year ago

to be clear; it is reasonable to have opt-in behaviors or overloads of algorithms that adjust these fine points. I was just commenting on the behavior implemented for other systems.

FranzBusch commented 1 year ago

We can definitely have a latest: true option here. The implementation should also be quite trivial, if we get nil from the upstream and we have a last value and latest = true then we can do a Task.sleep until the timeout has passed.

I'm fairly certain the current debounce implementation will not emit the final value in all cases.

If that happens and you can reproduce it please open a separate issue here.

FranzBusch commented 1 year ago

@phausler just put up a PR https://github.com/apple/swift-async-algorithms/pull/292 to change the behaviour. We would appreciate if you all could check that it is doing what you expected it to do now.

gregcotten commented 1 year ago

@phausler just put up a PR #292 to change the behaviour. We would appreciate if you all could check that it is doing what you expected it to do now.

Yes, this does exactly what I expect now!