Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should flatMapLatest be buffered by default? #2546

Closed
aasitnikov opened this issue Feb 20, 2021 · 3 comments
Closed

Should flatMapLatest be buffered by default? #2546

aasitnikov opened this issue Feb 20, 2021 · 3 comments

Comments

@aasitnikov
Copy link

aasitnikov commented Feb 20, 2021

For me it was really unexpected, because first it's not clear from the operator name that it's buffered, and second there's no other operators, that are buffered by default (not counting flatMapLatest and others, as they use transformLatest themselves).

flow {
    emit((1..9).toList())
    delay(1500)
    emit(listOf(10))
}
.flatMapLatest { it.asFlow() }
// .buffer(0) // we can remove buffer from flatMapLatest
.distinctUntilChanged()
.collectLatest { int ->
    doSomeResourceConsumingWork(int) // for simplicity can be replaced with delay(1000)
}

Here I was expecting it to collect 1, then 2, then cancel and receive the last 10 emit. But because flatMapLatest is buffered, there was 10 collects without cancellation.

So the main question is - should transformLatest be buffered by default, or is it better to change it to be non-buffered to avoid confusion?

@qwwdfsad
Copy link
Collaborator

Hi, we actually have a consistent policy regarding default buffering: if an operator can be buffered by design, then it always buffered and this fact is reflected in the documentation.

It has both practical and historical reasons:

  • Other reactive frameworks stick to this policy as well (with a minor difference that all those operators have overload with buffer size instead of orthogonal buffer() operator. E.g. you can take a look at switchMap) and people are generally happy with that.
  • In our experience regarding real-world applications, people tend to use such operators in concurrent/multithreaded environments. In such cases, buffering by default is typically both faster and more convenient and users don't even have to think about it or tune it.

As an unfortunate side-effect, single-threaded flows (ones that emit and collect in e.g. runBlocking) expose buffering behaviour

@aasitnikov
Copy link
Author

Yeah, I can understand the thinking behind making Observable\Flowable operators concatMap\switchMap\flatMap buffered, because as you said "people tend to use such operators in concurrent/multithreaded environments". Flow has three similar operators flatMapConcat\flatMapLatest\flatMapMerge, but they all have different buffering policy: flatMapContat isn't buffered, flatMapLatest is buffered and it's documented, and flatMapMerge is buffered, but docs does not reflect that fact. It feels inconsistent, maybe all of those operators should be buffered by default and have a note in docs about that fact, similar to flatMapLatest?

@qwwdfsad
Copy link
Collaborator

Thanks for spotting, documentation to flatMapMerge was incomplete due to its non-trivial contract. Addressed it in #2583.

flatMapConcat\flatMapLatest\flatMapMerge

We also have mapLatest :)

I've updated the doc and now mapLatest, flatMapLatest and flatMapMerge all mention their buffering policy.

Regarding flatMapContact/flattenConcat -- they are fully sequential and produce elements one-by-one, so there cannot be any default buffering (also, neither concatMap in Rx has, but there you can control a prefetch).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants