Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Replacing some Behavior/Publish Relay usage in core artifacts with coroutines #544
Replacing some Behavior/Publish Relay usage in core artifacts with coroutines #544
Changes from 3 commits
1015f2f
f27391b
036fa95
73ae40a
93dd6e8
56c9d29
be9bd95
6e857f6
80189ef
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prototyped a copy of bindToWorkerLifecycle that has to launch to match events so it doesn't need to convert to observable and can be native coroutine, and we end up in race condtion since a subscribe on main thread is guaranteed to register before the testing code calls it. Need to look into if we should block for registering the lambda but not on the terminal event.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I came up with the following implementation that passes tests from
WorkerBinderTest
. However, some things are still unclear to me:1. The scope of the coroutine / Rx subscription
it's still unclear to me what should be the scope of the coroutine -- I wonder the same about the current implementation by the way -- it feels leaky because the
Disposable
returned by the subscription inbindToWorkerLifecycle
is never used. In the example I just usedMainScope()
, could also work withGlobalScope
(assumingbind
is called from the main thread).2. Multithreading/concurrency
I feel like all of this binding/unbinding should be done on the main thread. But what if the
WorkerUnbinder.unbind()
is called from a worker thread? In the example below,worker.onStop()
would run a non-main thread, and I think it's erroneous. IfWorkerUnbinder.unbind()
is called from a non-main-thread, it would require asynchronicity to runworker.onStop()
on main thread (we'll need to dispatch the operation). The same doubt remains on current Rx implementation. I feel like binding and unbinding from a non-main thread should just be forbidden (throw exception) for the most predictable behavior: always synchronous (by the timeworkerUnbinder.unbind()
finishes,worker.onStop()
has also finished).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some experimentation:
Output:
Note the synchronous behavior here: job starts immediately and runs until collector installed. Also,
finally
blocks runs immediately on cancelation, and it only returns after thefinally block
finishes. I think we want the same behavior withWorkerUnbinder.unbind()
and Worker.onStop()`.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More experimentation: Notice that with current Rx implementation, the issue of calling
unbinder.unbind()
on a worker thread is the same: we'll end up callingworker.onStop
on the caller worker thread:Output:
If we use
observeOn
beforesubscribe
, this fixes it, but introduces unwanted asynchronicity, hence my proposal to forbid unbinding from a worker thread.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding 1, that's what I observed as well, and was thinking we'd need to use create a coroutinecontext on the spot. We can take one in as optional in the params for testing, but the current usage does "feel" leaky since it subscribes directly, but it looks like it ends up getting coupled to the parents lifecycle through the internal logic inside it.
Regarding 2, @FranAguilera has done some research on workers binding on main thread vs background and has a proposal to make an explicit API change. WIth his proposed changes, we'd need both options.