search

matrix-org / matrix-rust-sdk

Matrix Client-Server SDK for Rust
Apache License 2.0
1.27k stars 253 forks source link

`ObservableEventHandler` does not buffer past events, only returns the last event. #4297

Open torrybr opened 3 days ago

torrybr commented 3 days ago

ObservableEventHandler currently only returns the most recent event when subscribing to a stream, with no buffering of past events. This means events emitted before the subscription are not processed, which can be limiting for scenarios where all past and new events need to be handled.

Hywan commented 2 days ago

Be event handler or observable events will never emit events that are past the registration. However, the event handler API will return all events, whilst the observable events API will only return the current event at the time it is observed, there is no buffer of previous events.

I think we should clarify that in the documentation as it is a fundamental difference between the two API. Basically:

About your contribution, I believe this is not blocking since I'm not sure we are interested by all previous locations: only the current location is interesting. If the task listening for live locations isn't busy, it will consume all live locations as they come from the sync. Otherwise, if the task is busy, some live locations might be missed, but that's alright I think.

I also think we should implement a SharedBufferedObvervable type inside eyeball. Or maybe we can use a unbounded channel from Tokio instead of an ShareadObservable inside the observable events API, if everyone believe it should provide all values instead of the current one. That's really an easy and quick fix.

pinging @matrix-org/rust for feedbacks.

bnjbvr commented 2 days ago

I guess it depends on the use case of the new API? Live location is likely mostly interested in the last position, but other use cases might want the entire history.

I think it's OK to have a difference in terms of behaviors between the two APIs, but they must be heavily documented and clarified. It sounds like implementing an API to get the full history based on a regular client handler API is kind of trivial, so having a way to observe the latest instance of an event sounds like a useful complement :+1: (But having consistency across the two APIs would be nice too, so I'm a bit torn here.)