-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Fix stats widget InterruptedException family (Sentry JETPACK-ANDROID-1ATH and others) #22857
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
Merged
Merged
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
0450870
Fix Sentry: swallow InterruptedException in stats widget VMs
nbradbury af78a56
Merge remote-tracking branch 'origin/trunk' into fix/sentry-widget-in…
nbradbury 3158ddf
Restore interrupt flag, drop unused detekt suppression
nbradbury 6851c17
Updated comment
nbradbury 684d1d4
Drop noisy exception message interpolation from widget log lines
nbradbury 0136104
Clarify widget runBlocking helper scope and restore interrupt flag on…
nbradbury 6b6fca6
Merge remote-tracking branch 'origin/trunk' into fix/sentry-widget-in…
nbradbury 7c7bbee
Merge branch 'trunk' into fix/sentry-widget-interruptedexception
nbradbury File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
28 changes: 28 additions & 0 deletions
28
...n/java/org/wordpress/android/ui/stats/refresh/lists/widget/utils/WidgetCoroutineHelper.kt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| package org.wordpress.android.ui.stats.refresh.lists.widget.utils | ||
|
|
||
| import kotlinx.coroutines.CancellationException | ||
| import kotlinx.coroutines.runBlocking | ||
| import org.wordpress.android.util.AppLog | ||
|
|
||
| /** | ||
| * Runs [block] via [runBlocking] from inside a `RemoteViewsService.onDataSetChanged()` | ||
| * call on a stats widget. Swallows [InterruptedException] / [CancellationException] | ||
| * thrown when the widget host kills the service mid-fetch — the VM can still fall | ||
| * through to its cached read and render whatever data is already on disk. | ||
| * | ||
| * Only safe because this is a root [runBlocking] on the widget worker thread with no | ||
| * parent [kotlinx.coroutines.Job] above it — there is nothing upstream that needs to | ||
| * observe the cancellation. Do NOT call this from inside an existing coroutine: there | ||
| * the swallow would hide a cancellation signal from a surviving parent. | ||
| */ | ||
| internal fun runBlockingForWidget(block: suspend () -> Unit) { | ||
| try { | ||
| runBlocking { block() } | ||
| } catch (_: InterruptedException) { | ||
| AppLog.w(AppLog.T.STATS, "Widget data fetch interrupted") | ||
| Thread.currentThread().interrupt() | ||
| } catch (_: CancellationException) { | ||
| AppLog.w(AppLog.T.STATS, "Widget data fetch cancelled") | ||
| Thread.currentThread().interrupt() | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
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.
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.
mmm I'm not sure that swallowing
CancellationExceptionis a good idea because then the cancel propagation is stopped and upper jobs keep doing their thing.We should limit calling this function to very specific scenarios where we are sure it has no side effects. (Which I think is the case, but still I'm concerned)
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.
We could get rid of this and just catch the
InterruptedException, since that's the main source of the crashes. WDYT?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'm wondering what would happen then when throwing the thread interrupt. But I guess it could work.
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 asked for Claude's input on this discussion:
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.
Sounds reasonable.