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

[Fix] loadSavedOperations IndexOutOfBoundsException (5.1.11 only issue) #2081

Merged
merged 2 commits into from
May 8, 2024

Conversation

jkasten2
Copy link
Member

@jkasten2 jkasten2 commented May 7, 2024

Description

One Line Summary

Fixes IndexOutOfBounds exception thrown from OperationRepo.loadSavedOperations if app was opened offline, some operations done, and then the app is opened again.

Details

Motivation

SDK needs to be stable, even if the device goes offline at any point.

Scope

Only affects OperationRepo.loadSavedOperations used on cold start of the app.

Related

Fixes bug introduced in PR #2068 (released in 5.1.11)

Testing

Unit testing

Manual testing

Tested on an Android 14 emulator.

  1. Clear app storage
  2. Turn on airplane mode
  3. Open the app
  4. Perform operations, such as login and addTag
  5. Close and re-open the app, observe the runtime error no longer happens.

Affected code checklist

  • Notifications
    • Display
    • Open
    • Push Processing
    • Confirm Deliveries
  • Outcomes
  • Sessions
  • In-App Messaging
  • REST API requests
  • Public API changes

Checklist

Overview

  • I have filled out all REQUIRED sections above
  • PR does one thing
    • If it is hard to explain how any codes changes are related to each other then it most likely needs to be more than one PR
  • Any Public API changes are explained in the PR details and conform to existing APIs

Testing

  • I have included test coverage for these changes, or explained why they are not needed
  • All automated tests pass, or I explained why that is not possible
  • I have personally tested this on my device, or explained why that is not possible

Final pass

  • Code is as readable as possible.
    • Simplify with less code, followed by splitting up code into well named functions and variables, followed by adding comments to the code.
  • I have reviewed this PR myself, ensuring it meets each checklist item
    • WIP (Work In Progress) is ok, but explain what is still in progress and what you would like feedback on. Start the PR title with "WIP" to indicate this.

This change is Reviewable

Test proving that loadSavedOperations can throw
indexOutOfBoundsException. Real world scenario is this can happen if a
few operations are added when the device is offline then the app is
restarted.
Copy link
Contributor

@jinliu9508 jinliu9508 left a comment

Choose a reason for hiding this comment

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

The testing unit successfully thrown an IndexOutOfBoundException prior to the code change.
image
The test passed after the second commit.

However, I got a concurrent modification exception from a previous test unit. This is not stably reproducible though. It could be that calling queue.any from two different thread may interrupt each other if they happen to run at the same time.
image

Since things can be added to the queue before loadSavedOperations is
called it has to account for duplicate entries. It did do this, however
it was missing the logic to account for not advancing the index on
duplicates so an out of bounds was possible.

We solved the problem by only incrementing the index if it wasn't a
duplicate, however this implementation has a future landmine. If
something ever removes something from the queue that
loadSavedOperations added and it is still executing index problems can
still happen. This scenario never happens now, but a new feature to
OperationRepo or a refactor could introduce the problem. A fast follow
is recommend so this landmine isn't left here.
@jkasten2 jkasten2 force-pushed the fix/loadSavedOperations-index-out-of-bounds branch from feafcda to 8aa83c5 Compare May 8, 2024 17:03
@jkasten2 jkasten2 changed the title WIP Fix loadSavedOperations IndexOutOfBoundsException [Fix] loadSavedOperations IndexOutOfBoundsException May 8, 2024
@jkasten2 jkasten2 changed the title [Fix] loadSavedOperations IndexOutOfBoundsException [Fix] loadSavedOperations IndexOutOfBoundsException (5.1.11 only issue) May 8, 2024
@jkasten2
Copy link
Member Author

jkasten2 commented May 8, 2024

Good catch! I have address the ConcurrentModificationException and did a git push --force-with-lease to correct the orignal commit.

@jkasten2
Copy link
Member Author

jkasten2 commented May 8, 2024

Tests failed on CI sometimes:

Failure 1

com.onesignal.core.internal.operations.OperationRepoTests > ensure onOperationRepoLoaded is called once loading is completed FAILED
    java.lang.AssertionError: Verification failed: calls are not in verification order

    Matchers: 
    +OperationRepo(#433).subscribe(any()))
    OperationModelStore(#424).loadOperations())
    IOperationRepoLoadedListener(#432).onOperationRepoLoaded())

    Calls:
    1) OperationRepo(#433).addOperationLoadedListener(IOperationRepoLoadedListener(#432))
    2) +OperationRepo(#433).subscribe(IOperationRepoLoadedListener(#432))
    3) OperationRepo(#433).start()
    4) OperationRepo(#433).getHasSubscribers()

Failure 2

com.onesignal.user.internal.migrations.RecoverFromDroppedLoginBugTests > ensure RecoverFromDroppedLoginBug receive onOperationRepoLoaded callback from operationRepo FAILED
    java.lang.AssertionError: Verification failed: call 2 of 5: IdentityModelStore(#917).getModel()) was not called
        at io.mockk.impl.recording.states.VerifyingState.failIfNotPassed(VerifyingState.kt:63)
        at io.mockk.impl.recording.states.VerifyingState.recordingDone(VerifyingState.kt:42)
        at io.mockk.impl.recording.CommonCallRecorder.done(CommonCallRecorder.kt:47)
        at io.mockk.impl.eval.RecordedBlockEvaluator.record(RecordedBlockEvaluator.kt:64)
        at io.mockk.impl.eval.VerifyBlockEvaluator.verify(VerifyBlockEvaluator.kt:30)
        at io.mockk.MockKDsl.internalVerify(API.kt:119)
        at io.mockk.MockKKt.verify(MockK.kt:149)
        at io.mockk.MockKKt.verify$default(MockK.kt:140)
        at com.onesignal.user.internal.migrations.RecoverFromDroppedLoginBugTests$1$1.invokeSuspend(RecoverFromDroppedLoginBugTests.kt:43)
        at com.onesignal.user.internal.migrations.RecoverFromDroppedLoginBugTests$1$1.invoke(RecoverFromDroppedLoginBugTests.kt)
        at com.onesignal.user.internal.migrations.RecoverFromDroppedLoginBugTests$1$1.invoke(RecoverFromDroppedLoginBugTests.kt)

        Caused by:
        java.lang.AssertionError: Verification failed: call 2 of 5: IdentityModelStore(#917).getModel()) was not called
            at io.mockk.impl.recording.states.VerifyingState.failIfNotPassed(VerifyingState.kt:63)
            ... 10 more

These seem to be pre-existing flaky, not related to these changes. (so should be addressed in a different PR)

@jinliu9508 jinliu9508 merged commit b85e36a into main May 8, 2024
2 checks passed
@jinliu9508 jinliu9508 deleted the fix/loadSavedOperations-index-out-of-bounds branch May 8, 2024 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants