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.

Sign up for GitHub

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

Jump to bottom

Improve Swift concurrency safety #1376

Merged
merged 8 commits into from
Mar 5, 2024
Merged

Improve Swift concurrency safety #1376

merged 8 commits into from
Mar 5, 2024

Commits on Mar 4, 2024

  1. Add a unit test to reproduce OpRepo crash

    @nan-li
    nan-li committed Mar 4, 2024
    Configuration menu
    Copy the full SHA
    a554e30 View commit details
    Browse the repository at this point in the history

  2. Add a Lock class

    @nan-li
    nan-li committed Mar 4, 2024
    Configuration menu
    Copy the full SHA
    d42c494 View commit details
    Browse the repository at this point in the history

  3. Add lock around accessing OpRepo delta queue 

    * Add lock around appending to the queue
* Add lock around entire logic of flushing the delta queue, including enqueuing and flushing in the executors
    @nan-li
    nan-li committed Mar 4, 2024
    Configuration menu
    Copy the full SHA
    96b1ad7 View commit details
    Browse the repository at this point in the history

  4. Synchronize the background tasks dictionary 

    There was a report of a crash related to this dictionary and while I have not been able to reproduce by hammering many threads to call methods manipulating the tasks dictionary, access to them should be synchronized.
    @nan-li
    nan-li committed Mar 4, 2024
    Configuration menu
    Copy the full SHA
    93435a6 View commit details
    Browse the repository at this point in the history

  5. Properties model: add locking to tags dictionary 

    I was able to create crashes by hammering threads to add tags concurrently. Adding the lock prevented crashes, and behavior seems as expected.
Unfortunately, the crashes were very flaky when trying to reproduce in tests, they would crash in much less than 50% of runs. Producing the crashes in the example app was much more consistent. Hence why there is no unit test to reproduce this.
    @nan-li
    nan-li committed Mar 4, 2024
    Configuration menu
    Copy the full SHA
    a21404f View commit details
    Browse the repository at this point in the history

  6. Identity model: add locking to aliases dictionary 

    I was able to create crashes by hammering threads to add aliases and get aliases concurrently. Adding the lock prevented crashes, and behavior seems as expected. I also had to lock the getters for onesignalId and externalId

Unfortunately, the crashes were very flaky when trying to reproduce in tests, they would crash in much less than 50% of runs. Producing the crashes in the example app was much more consistent. Hence why there is no unit test to reproduce this.
    @nan-li
    nan-li committed Mar 4, 2024
    Configuration menu
    Copy the full SHA
    3183463 View commit details
    Browse the repository at this point in the history

  7. Property Executor: Add private dispatch queue 

    * Reproduced crashes by creating multiple async threads that added and removed tags concurrently.
* Added a private dispatch queue to synchronize access to the delta queue and request queue.
* Crashes no longer happened after this change.
* It is possible for the executor to be flushing while a client response is received and modify the request queue.
* Additionally, there are some code paths that enqueue and update request but does not go through the operation repo, such as updating session count at the start of a new session.
    @nan-li
    nan-li committed Mar 4, 2024
    Configuration menu
    Copy the full SHA
    cb88aa6 View commit details
    Browse the repository at this point in the history

  8. OpRepo: Use private dispatch queue instead of lock 

    * In a previous commit, an unfair lock was used to synchronize access to the delta queue and synchronize flushing behavior
* A dispatch queue seems more appropriate for the Operation Repo to use considering it already polls and flushes on a global queue.
* Without the lock or dispatch queue, I reproduced crashes by creating multiple async threads that either added tags or called to flush the operation repo.
* With the dispatch queue, those crashes do no happen and behavior seems as expected.
    @nan-li
    nan-li committed Mar 4, 2024
    Configuration menu
    Copy the full SHA
    c45c081 View commit details
    Browse the repository at this point in the history