[Compile Warnings As Errors] WordPress Module - Resolve Coroutines Warnings #17210

ParaskP7 · 2022-09-26T14:21:20Z

Parent: #17173
Partially Closes: #17181

This PR resolves all Coroutines related warnings for the WordPress module:

Main Source:
- 3 warnings x This declaration is experimental and its usage should be marked with '@kotlinx.coroutines.ExperimentalCoroutinesApi' or '@OptIn(kotlinx.coroutines.ExperimentalCoroutinesApi::class)'
- 2 warnings x 'XYZ' is deprecated. Implement CoroutineScope interface and cancel all child coroutines in xyz
- 1 warning x @Synchronized annotation is not applicable to suspend functions and lambdas + Question [❓]: see comment @malinajirka
Test Source:
- 8 warnings x This declaration is experimental and its usage should be marked with '@kotlinx.coroutines.ExperimentalCoroutinesApi' or '@OptIn(kotlinx.coroutines.ExperimentalCoroutinesApi::class)'
- 3 warnings x 'Xyz' is deprecated. Deprecated in the favour of 'xyz'. Please note that the provided replacement does not rethrow channel's close cause as 'poll' did, for the precise replacement please refer to the 'poll' documentation

Warnings Resolution List:

Warnings Suppression List:

Suppress synchronized on suspend coroutines warning.

Refactor List:

Documentation List:

Add scope, dispatcher and other group sections to thread module.

PS: @AjeshRPai I added you as the main reviewer, that is, in addition to @wordpress-mobile/apps-infrastructure team itself, but randomly, since I want someone from the WordPress Android team to primarily sign-off on that change. 🥇

FYI: I am going to randomly add more of you in those PRs that will follow, just so you become more aware of this change and how close we are on enabling allWarningsAsErrors by default everywhere. 🎉

To test:

There is nothing much to test here.
Verifying that all the CI checks are successful should be enough.
However, if you want to be thorough about reviewing this change, you could quickly smoke test both, the WordPress and Jetpack apps, and see if they are both working as expected.

PS: Also note the additional testing instructions as specified in this comment here.

Regression Notes

Potential unintended areas of impact

N/A

What I did to test those areas of impact (or what existing automated tests I relied on)

See To test section above.

What automated tests I added (or what prevented me from doing so)

N/A

PR submission checklist:

I have completed the Regression Notes.
I have considered adding accessibility improvements for my changes.
I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

1st Warning Messages: "This declaration is experimental and its usage should be marked with '@kotlinx.coroutines.ExperimentalCoroutinesApi' or '@OptIn(kotlinx.coroutines.ExperimentalCoroutinesApi::class)'" Adding the '@OptIn(ExperimentalCoroutinesApi::class)' annotation on these functions is the recommended action to resolve this 'GlobalScope' warnings. However, this results to the below '@OptIn' warnings: 2nd Warning Message: "This annotation should be used with the compiler argument '-Xopt-in=kotlin.RequiresOptIn'" Adding the '-Xopt-in=kotlin.RequiresOptIn' free compiler argument is the recommended action to resolve this '@OptIn' warning.

Warning Messages: "'UI/DEFAULT_SCOPE: String' is deprecated. Implement CoroutineScope interface and cancel all child coroutines in onCleared/onDestroy/.."ª It seems that both, the deprecated named 'UI_SCOPE' and 'DEFAULT_SCOPE' coroutine scopes, that are being provided by the [ThreadModule] module, are no longer being utilized within this codebase and thus, they can be now safely removed. There was no need for any migration either.

@synchronized

Warning Messages: "@synchronized annotation is not applicable to suspend functions and lambdas" This coroutines warning is suppressed, that is, instead of it being resolved, since it seems that: 1. Adding the '@synchronized' annotation was done deliberately, and 2. Changing that is a bit complicated as it would need a bit more thought to properly fix this, using [Mutex] or otherwise. For more info see: - Fix crash in UploadStarter on Samsung devices with Android 5 #10878 (Introduced In): https://github.com/wordpress-mobile/WordPress-Android/ pull/10878 - Forbid @synchronized annotation on suspend functions (Kotlin Issue): https://youtrack.jetbrains.com/issue/KT-27333/ Forbid-Synchronized-annotation-on-suspend-functions - Coroutines and Java Synchronization Don't Mix (Blog Post: https://blog.danlew.net/2020/01/28/ coroutines-and-java-synchronization-dont-mix/

Warning Messages: "This declaration is experimental and its usage should be marked with '@kotlinx.coroutines.ExperimentalCoroutinesApi' or '@OptIn(kotlinx.coroutines.ExperimentalCoroutinesApi::class)'"

Warning Messages: "'poll(): E?' is deprecated. Deprecated in the favour of 'tryReceive'. Please note that the provided replacement does not rethrow channel's close cause as 'poll' did, for the precise replacement please refer to the 'poll' documentation"

@ExperimentalCoroutinesApi

This is done so that the '@ExperimentalCoroutinesApi' annotation is only presented itself once per test suite and avoid having to think and add this boilerplate configuration multiple times, per test configuration or function, whenever and if it is needed.

wpmobilebot · 2022-09-26T14:35:49Z

📲 You can test these changes on WordPress by downloading wordpress-installable-build-pr17210-43949f1.apk

	💡 Scan this QR code with your Android phone to download and install the APK directly on it.
	App	`WordPress`
	Build Flavor	`Jalapeno`
	Build Type	`Debug`
	Commit	`43949f1`

Note: This installable build uses the JalapenoDebug build flavor, and does not support Google Login.

wpmobilebot · 2022-09-26T14:40:21Z

📲 You can test these changes on Jetpack by downloading jetpack-installable-build-pr17210-43949f1.apk

	💡 Scan this QR code with your Android phone to download and install the APK directly on it.
	App	`Jetpack`
	Build Flavor	`Jalapeno`
	Build Type	`Debug`
	Commit	`43949f1`

Note: This installable build uses the JalapenoDebug build flavor, and does not support Google Login.

ParaskP7 · 2022-09-26T15:09:31Z

👋 @AjeshRPai !

FYI: After talking to Jirka about this comment I left for him here, I decided to go ahead and resolve this @Synchronized annotation related warning after all, that is, instead of suppressing it like I did (see here).

As such, I added an additional label to this PR, the [Status] Not Ready For Merge label. However, feel free to proceed with the code review as you would have anyway, don't let this stop you from reviewing the overall solution here.

Then, and when I'll have my finally solution to this problem ready (probably tomorrow), I'll push it here and inform you about the change so you can take a final look at it (plus test it). 🧑‍🏭

@synchronized

Warning Messages: "@synchronized annotation is not applicable to suspend functions and lambdas" It turns out that this '@synchronized' solution were already replacing the previous (d5b9948) 'Mutex' solution, and this, due to the fact that the existing (back then) 'Mutex' solution were causing crashes on Samsung devices running Android 5 (API Level 21). However, this repo, and thus both, the WordPress and Jetpack apps, are no longer supporting these devices as the current 'minSdkVersion' is '24' (Android 7). Thus, this '@synchronized' annotation can be now safely removed and the 'Mutex' solution reverting back. For more info see: - Fix crash in UploadStarter on Samsung devices with Android 5 #10877 (Introduced In): https://github.com/wordpress-mobile/WordPress-Android/ pull/10877/ - ExceptionInInitializerError #10827 (Crash Report): #10827 - NoSuchFieldException with AtomicReferenceFieldUpdater on Samsung Android 5.0.x devices #490 (Coroutines Issue): Kotlin/kotlinx.coroutines#490

ParaskP7 · 2022-09-27T09:54:42Z

👋 @AjeshRPai @malinajirka !

FYI: This is now done, I have resolved the @Synchronized annotation related warning by reverting the previous (d5b9948) commit via this new commit here (43949f1). I have also tested this, both, the before and after the change, by following the instructions in this PR here, everything worked as expected. For your convenience, I am copy-pasting the testing instructions here as well:

To test:
I don't have a Samsung phone running Android 5. But we should at least
verify it still works as expected on other phones.

1. Turn off the internet connection
2. Create a post
3. Publish it
4. Turn on internet connection + pull to refresh
5. Make sure the post gets uploaded

PS: You will find more info about the reasoning behind this change, along with a couple associated links, within the commit message itself.

AjeshRPai

Thanks for providing me with all the details for the review in commit messages as well as in the description. As always, it’s a pleasure to review your PR since I will get the context of the change/links of the references in your PR description and commit messages themselves. 🙌

I have

tested the PR by going through the changes in every commit and reading the reference links mentioned in the commit messages and everything LGTM 👍🏼
done a smoke testing of the WordPress and jetpack app and everything looks fine 👍🏼
tested the synchronized to mutex changes. Published a post on offline mode with the following steps. It works as expected 🟢 👍🏼

1. Turn off the internet connection
2. Create a post
3. Publish it
4. Turn on internet connection + pull to refresh
5. Make sure the post gets uploaded

Am going to approve but not merge the PR so that anyone from @wordpress-mobile/apps-infrastructure or @Jirka can have a look. Please feel free to merge the PR at your convenience. Nice work 🙌🏼

ParaskP7 · 2022-09-27T11:17:43Z

Thank you so much for reviewing and testing this @AjeshRPai , you are awesome! 🙇

Am going to approve but not merge the PR so that anyone from https://github.com/orgs/wordpress-mobile/teams/apps-infrastructure or @Jirka can have a look. Please feel free to merge the PR at your convenience. Nice work 🙌🏼

This sounds like a good plan! 🙏

PS: As always, I am humbled by your kind words, thank you! 😊

ParaskP7 · 2022-10-04T10:15:15Z

👋 @malinajirka !

Am going to approve but not merge the PR so that anyone from https://github.com/orgs/wordpress-mobile/teams/apps-infrastructure or @Jirka can have a look. Please feel free to merge the PR at your convenience. Nice work 🙌🏼

Any last thoughts on it before I go merging this to trunk. PS: Thanks a lot for your help? 🙏

malinajirka · 2022-10-04T10:24:25Z

I haven't tested it but the changes LGTM! ⭐

ParaskP7 · 2022-10-04T10:31:22Z

Coolio, thank you for the confirmation @malinajirka , that's all I needed! 🙇

ParaskP7 added 10 commits September 26, 2022 15:39

Refactor: Move ui dispatcher down to group scope and dispatcher funs

d1f74fd

Docs: Add scope, dispatcher and other group sections to thread module

e3242a5

Refactor: Rename provide background to bg dispatcher

e03d208

Analysis: Resolve opt-in experimental coroutines api warnings for tests

e6471d4

Warning Messages: "This declaration is experimental and its usage should be marked with '@kotlinx.coroutines.ExperimentalCoroutinesApi' or '@OptIn(kotlinx.coroutines.ExperimentalCoroutinesApi::class)'"

Test: Remove unnecessary experimental coroutines api annotation

2fd0fc8

ParaskP7 added Tooling Tech Debt labels Sep 26, 2022

ParaskP7 added this to the 20.9 milestone Sep 26, 2022

ParaskP7 requested review from a team and AjeshRPai September 26, 2022 14:21

ParaskP7 self-assigned this Sep 26, 2022

This was referenced Sep 26, 2022

Fix crash in UploadStarter on Samsung devices with Android 5 #10878

Merged

Compiler Warnings as Errors - WordPress Module #17181

Closed

Compiler Warnings as Errors - Resolve/Suppress All Warnings #17173

Closed

ParaskP7 added the [Status] Not Ready for Merge label Sep 26, 2022

ParaskP7 removed the [Status] Not Ready for Merge label Sep 27, 2022

AjeshRPai approved these changes Sep 27, 2022

View reviewed changes

AliSoftware modified the milestones: 20.9, 21.0 Oct 3, 2022

ParaskP7 merged commit 6092a72 into trunk Oct 4, 2022

ParaskP7 deleted the analysis/wordpress-coroutines-warnings branch October 4, 2022 10:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Compile Warnings As Errors] WordPress Module - Resolve Coroutines Warnings #17210

[Compile Warnings As Errors] WordPress Module - Resolve Coroutines Warnings #17210

Uh oh!

ParaskP7 commented Sep 26, 2022 •

edited

Loading

Uh oh!

wpmobilebot commented Sep 26, 2022 •

edited

Loading

Uh oh!

wpmobilebot commented Sep 26, 2022 •

edited

Loading

Uh oh!

ParaskP7 commented Sep 26, 2022

Uh oh!

ParaskP7 commented Sep 27, 2022 •

edited

Loading

Uh oh!

AjeshRPai left a comment

Uh oh!

ParaskP7 commented Sep 27, 2022

Uh oh!

ParaskP7 commented Oct 4, 2022

Uh oh!

malinajirka commented Oct 4, 2022

Uh oh!

ParaskP7 commented Oct 4, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

[Compile Warnings As Errors] WordPress Module - Resolve Coroutines Warnings #17210

[Compile Warnings As Errors] WordPress Module - Resolve Coroutines Warnings #17210

Uh oh!

Conversation

ParaskP7 commented Sep 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Regression Notes

Uh oh!

wpmobilebot commented Sep 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wpmobilebot commented Sep 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ParaskP7 commented Sep 26, 2022

Uh oh!

ParaskP7 commented Sep 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AjeshRPai left a comment

Choose a reason for hiding this comment

Uh oh!

ParaskP7 commented Sep 27, 2022

Uh oh!

ParaskP7 commented Oct 4, 2022

Uh oh!

malinajirka commented Oct 4, 2022

Uh oh!

ParaskP7 commented Oct 4, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ParaskP7 commented Sep 26, 2022 •

edited

Loading

wpmobilebot commented Sep 26, 2022 •

edited

Loading

wpmobilebot commented Sep 26, 2022 •

edited

Loading

ParaskP7 commented Sep 27, 2022 •

edited

Loading