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

Use a direct dispatcher in Flow -> Rx conversions. #592

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

psteiger
Copy link
Contributor

@psteiger psteiger commented May 25, 2023

A direct dispatcher mimics the Rx always synchronous-by-default behavior (unless subscribeOn/observeOn is used), which is different to the Dispatchers.Unconfined behavior (default used by Flow.asObservable) which may form event loops for nested coroutines (asynchronous).

See Kotlin/kotlinx.coroutines#3760

@psteiger psteiger force-pushed the directdispatcher branch 2 times, most recently from 1025f48 to 311ae10 Compare May 26, 2023 01:29
* 3. [Coroutines/Flow vs add/remove listener (synchronous execution) #3506](https://github.com/Kotlin/kotlinx.coroutines/issues/3506)
* <!-- spotless:on -->
*/
internal object DirectDispatcher : CoroutineDispatcher() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for adding! wondering if this should be moved into another module (e.g. libraries/rib-coroutines)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@psteiger friendly ping here ^

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I missed this comment.

I think it's best to have it in "core", because I believe the tendency is for rib-coroutine to cease to exist once we migrate fully to coroutines.

@tyvsmith in case you wanna confirm or deny ^

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

np! I see, thanks for clarifying

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets move it to ribs-coroutines until we're ready to move all of it to core. Otherwise we're in a worse fragmented state.

@FranAguilera
Copy link
Contributor

What do you think if we update WorkerBinder default param value with Unconfined with this new DirectDispatcher? https://github.com/uber/RIBs/blob/main/android/libraries/rib-base/src/main/kotlin/com/uber/rib/core/WorkerBinder.kt#L78

@FranAguilera FranAguilera added the Android Android related tickets label Jun 15, 2023
@psteiger
Copy link
Contributor Author

@FranAguilera that's a good question

I think we would be wise to do some benchmarks to measure performance impact of this change and also experiment internally on the safety (stackoverflow risk etc)

@FranAguilera
Copy link
Contributor

@FranAguilera that's a good question

I think we would be wise to do some benchmarks to measure performance impact of this change and also experiment internally on the safety (stackoverflow risk etc)

@psteiger Sorry missed this comment earlier. I totally agree, we can put internal doc for steps on measuring impact of this change within our internal repo

A direct dispatcher mimics the Rx always synchronous-by-default behavior (unless `subscribeOn`/`observeOn` is used), which is different to the `Dispatchers.Unconfined` behavior (default used by `Flow.asObservable`) which may form event loops for nested coroutines (asynchronous).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Android Android related tickets
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants