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: Correct threading around user notification log #261

Merged

Conversation

BrandonStalnaker
Copy link
Contributor

Instructions

  1. PR target branch should be against development
  2. PR title name should follow this format: https://github.com/mParticle/mparticle-workflows/blob/main/.github/workflows/pr-title-check.yml
  3. PR branch prefix should follow this format: https://github.com/mParticle/mparticle-workflows/blob/main/.github/workflows/pr-branch-check-name.yml

Summary

  • Fix occasion timing crash around log notification
  1. App goes to background for a long time (not killed)
  2. User receives and taps notification
  3. App opens and at the same, concurrently, time attempts to
    a. on main queue, log user notification with _session property in
    MPMessageBuilder *messageBuilder = [MPMessageBuilder newBuilderWithMessageType:MPMessageTypePushNotification session:_session messageInfo:messageInfo];

    b. on internal message queue, end old session and begin a new one in

The read on main queue in logUserNotification: method is done without synchronisation, resulting in occasional crashes on notification interactions.

Testing Plan

  • Tested using sample app while manually triggering logNotification

Reference Issue (For mParticle employees only. Ignore if you are an outside contributor)

Copy link
Contributor

@einsteinx2 einsteinx2 left a comment

Choose a reason for hiding this comment

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

Looks like a solid solution to the issue.

@BrandonStalnaker BrandonStalnaker force-pushed the fix/6192-unsafe-logging-user-notification branch from b96ce13 to e4b806c Compare March 14, 2024 18:03
@einsteinx2 einsteinx2 force-pushed the fix/6192-unsafe-logging-user-notification branch from e4b806c to dd03859 Compare March 14, 2024 18:05
@einsteinx2 einsteinx2 force-pushed the fix/6192-unsafe-logging-user-notification branch from dd03859 to 04b3b71 Compare March 14, 2024 18:12
@BrandonStalnaker BrandonStalnaker merged commit 456160b into development Mar 14, 2024
11 checks passed
@BrandonStalnaker BrandonStalnaker deleted the fix/6192-unsafe-logging-user-notification branch March 14, 2024 18:35
mparticle-automation added a commit that referenced this pull request Mar 19, 2024
# [8.21.0](v8.20.0...v8.21.0) (2024-03-19)

### Bug Fixes

* Correct threading around user notification log ([#261](#261)) ([456160b](456160b))
* Refactor MPUploadBuilder to attempt to eliminate rare crash in withLocation: method ([#262](#262)) ([60cd0c8](60cd0c8))

### Features

* Improve mParticle reset methods ([#263](#263)) ([cde71a2](cde71a2))
@mparticle-automation
Copy link
Contributor

🎉 This PR is included in version 8.21.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants