-
Notifications
You must be signed in to change notification settings - Fork 50
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
Check scope active for awaiting player #578
Conversation
Signed-off-by: brocollie08 <[email protected]>
Signed-off-by: brocollie08 <[email protected]>
/canary |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #578 +/- ##
=======================================
Coverage 89.76% 89.76%
=======================================
Files 331 331
Lines 19840 19840
Branches 1949 1949
=======================================
Hits 17809 17809
Misses 2017 2017
Partials 14 14 ☔ View full report in Codecov by Sentry. |
This reverts commit fcbdba5.
@@ -78,7 +80,7 @@ public open class PlayerViewModel(flows: AsyncFlowIterator) : ViewModel(), Andro | |||
deferredPlayer.getCompleted() | |||
} else { | |||
runBlocking { | |||
deferredPlayer.await() | |||
if (viewModelScope.isActive) deferredPlayer.await() else throw IllegalAccessError("Accessing Player instance when ViewModelScope is no longer active") |
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.
-
Can we do the check outside the
runBlocking
to avoid the perf overhead of creating coroutines and blocking the thread on that coroutine context? -
If the scope that
deferredPlayer
executes in changes, this check would need to be updated to. It'd probably be better to dodeferredPlayer.isActive
if that's possible to check "activeness" explicitly against theDeferred
we're trying to access.
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.
moved to release so no longer needed to check and return or throw here
try { | ||
release() | ||
} catch (e: IllegalAccessError) { | ||
Log.e("AndroidPlayer", e.message ?: "IllegalAccessError in AndroidPlayer") | ||
} |
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.
Do you think this might swallow real IllegalAccessError
s? Can/should we be more targeted with what we're wrapping in try/catch, or maybe even not throw anything at all? i.e.:
public fun release() {
if (deferredPlayer.isCompleted) {
player.release()
}
}
Signed-off-by: brocollie08 <[email protected]>
Change Type (required)
Indicate the type of change your pull request is:
patch
minor
major
N/A
Does your PR have any documentation updates?
📦 Published PR as canary version:
0.10.2--canary.578.19394
Try this version out locally by upgrading relevant packages to 0.10.2--canary.578.19394