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

Correct the usage of flowOn #1508

Merged
merged 2 commits into from
Jun 24, 2024

Conversation

simophin
Copy link

@simophin simophin commented Jun 4, 2024

Description

flowOn applies to operators BEFORE not after.

@simophin simophin changed the base branch from master to dev June 4, 2024 00:42
Copy link

@bemusementpark bemusementpark left a comment

Choose a reason for hiding this comment

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

Cool! Alternatively, I think the function we were looking for was collectOn which operates on the downstream. Perhaps we could replace it with that?

either way:

  1. Does using flowOn on the merged flow allow for parallelism?
  2. Did you check for any other usages of flowOn in the app, or is this home screen only?
  3. Did you put a log or breakpoint in the offending function(s) to check they're not on main? I guess we could even call one of those assertNotMainThread() kinda methods if we really wanted. or even if (BuildConfig.DEBUG) assertNotMain() or assertNotMainIfDebug() if we use it frequently and want a shorthand.

@@ -60,12 +58,10 @@ class HomeViewModel @Inject constructor(
observeTypingStatus(),
messageRequests(),
::Data
)
.stateIn(viewModelScope, SharingStarted.Eagerly, null)
).stateIn(viewModelScope, SharingStarted.Eagerly, null)

Choose a reason for hiding this comment

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

🎉

@simophin
Copy link
Author

simophin commented Jun 4, 2024

Cool! Alternatively, I think the function we were looking for was collectOn which operates on the downstream. Perhaps we could replace it with that?

either way:

1. Does using `flowOn` on the merged flow allow for parallelism?

2. Did you check for any other usages of `flowOn` in the app, or is this home screen only?

3. Did you put a log or breakpoint in the offending function(s) to check they're not on main? I guess we could even call one of those `assertNotMainThread()` kinda methods if we really wanted. or even `if (BuildConfig.DEBUG) assertNotMain()` or `assertNotMainIfDebug()` if we use it frequently and want a shorthand.

I was looking for something like collectOn as well but couldn't find such thing.
I did put breakpoint on the db acess AFTER the changes and they all ran on background threads.

@bemusementpark
Copy link

Nice, does it run concurrently with one flowOn after a merge/combine? or should we add appropriate scheduling to each flow?

@simophin
Copy link
Author

simophin commented Jun 4, 2024

Nice, does it run concurrently with one flowOn after a merge/combine? or should we add appropriate scheduling to each flow?

Yes they do, just confirmed in the debugger that they run concurrently in different threads as expected.
My understanding is that Dispatchers.IO is a thread pool facility so the behavior should be along the line of "apply this dispatcher to the operators == apply this thread pool to this operators"

@ThomasSession ThomasSession merged commit 01655b8 into oxen-io:dev Jun 24, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants