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

Refactor: Replace mutex.lock() with mutex.withLock() #18067

Merged
merged 2 commits into from
May 2, 2023

Conversation

zwarm
Copy link
Contributor

@zwarm zwarm commented Mar 8, 2023

Parent #17463
(CompletionHandlerException: Exception in resume onCancellation handler for CancellableContinuation(DispatchedContinuation[Dis...)

The goal of this PR is to try and fix the IllegalStateException: Mutex is not locked issue that occurs when uploading local changes.

The intent is to change from using mutex.lock() to mutex.withLocking(). This should ensure that we have access to the locked mutex before trying to unlock it. The theory behind this is that we may be experiencing a race condition since mutex.isLocked() only gives us the "at this moment" answer, another process could have come in before we get to the unlock.

Using withLock should automatically manage the locking/unlocking of the mutex, and in addition, we can call mutex.unlock() when we experience a CancellationException without needing to check the isLocked beforehand.

If anyone thinks this is a bad idea, we can nix it. Figured it was worth a try. :)

To test:
There is nothing specific to test; as we haven't been able to reproduce this crash.
Please take the apps out for a generic spin - create/update some posts, turn networking on/off, etc.

Regression Notes

  1. Potential unintended areas of impact
    N/A

  2. What I did to test those areas of impact (or what existing automated tests I relied on)
    N/A

  3. 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.

@zwarm zwarm requested review from ovitrif and ParaskP7 March 8, 2023 20:22
@zwarm zwarm self-assigned this Mar 8, 2023
@wpmobilebot
Copy link
Contributor

wpmobilebot commented Mar 8, 2023

Jetpack📲 You can test these changes on Jetpack by downloading jetpack-installable-build-pr18067-1b5e1f3.apk
💡 Scan this QR code with your Android phone to download and install the APK directly on it.
AppJetpack
Build FlavorJalapeno
Build TypeDebug
Commit1b5e1f3
Note: This installable build uses the JalapenoDebug build flavor, and does not support Google Login.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Mar 8, 2023

WordPress📲 You can test these changes on WordPress by downloading wordpress-installable-build-pr18067-1b5e1f3.apk
💡 Scan this QR code with your Android phone to download and install the APK directly on it.
AppWordPress
Build FlavorJalapeno
Build TypeDebug
Commit1b5e1f3
Note: This installable build uses the JalapenoDebug build flavor, and does not support Google Login.

@peril-wordpress-mobile
Copy link

Warnings
⚠️ PR is not assigned to a milestone.

Generated by 🚫 dangerJS

@zwarm zwarm marked this pull request as draft March 9, 2023 13:17
@zwarm
Copy link
Contributor Author

zwarm commented Mar 9, 2023

Note: Moved to a draft PR. Although all the current tests pass, there is still some uncertainty as to whether this will make a difference.

@ovitrif
Copy link
Contributor

ovitrif commented Mar 22, 2023

Note: Moved to a draft PR. Although all the current tests pass, there is still some uncertainty as to whether this will make a difference.

I think these days "I feel in better shape" to retry simulating this error via TDD. Will try to find some time this week or next week to dedicate to this 🙇🏻

Thanks for drafting this PR @zwarm !

@ovitrif
Copy link
Contributor

ovitrif commented May 2, 2023

@zwarm Let's try this out, it can't hurt 👍🏻

Plus, GPT subscribes:

CleanShot 2023-05-02 at 17 59 19@2x

Copy link
Contributor

@ovitrif ovitrif left a comment

Choose a reason for hiding this comment

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

LGTM :shipit:

IMHO it can't hurt to try this out, we'll keep monitoring the crash frequency and see if it actually does fix the issue.

@ovitrif ovitrif marked this pull request as ready for review May 2, 2023 16:03
@ovitrif ovitrif added this to the 22.4 milestone May 2, 2023
@ovitrif ovitrif merged commit 0bf9f10 into trunk May 2, 2023
@ovitrif ovitrif deleted the issue/17463-use-mutex-withLock branch May 2, 2023 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants