fix(AWSPinpoint): Fixing a deadlock when submitting events from multiple threads

[ruisebas]

Issue #, if available:

• possible deadlock with AWSPinpointAnalyticsClient submitEvents #2419
• buildEndpointRequestPayload EXC_BAD_ACCESS_KERN_INVALID_ADDRESS #4551

Description of changes:

When submitEvents is called, part of the process is to grab the currentEndpointProfile because the PutEvents call to Pinpoint requires it as an input.

Inside AWSPinpointTargetingClient.currentEndpointProfile, there's a call to AWSPinpointNotificationManager.isNotificationEnabled, which is necessary to determine if the endpoint is currently opted out of notifications or not.

isNotificationEnabled is internally calling UIApplication.isRegisteredForRemoteNotifications, but because the latter needs to be called from the main queue, it's doing a dispatch_sync(dispatch_get_main_queue()) if the current thread is not the main one.

This in isolation is not a problem, but it causes trouble when integrated to the whole event submission process, since we do a @synchronized(self.lock) right at the beginning. This will result in a deadlock:

• A background thread calls submitEvents and gets the lock
• The main queue calls submitEvents and waits to get the lock
• The background thread reaches the isNotificationEnabled code, so it attempts to dispatch_sync into the main queue
• Because the main queue is waiting for the lock to be released, it won't run.

The background thread is now blocked because it is waiting on the main queue to unblock, which un turn is waiting for the background thread to release the lock.

---

In order to fix this, I wanted to be the least disruptive possible since we are dealing with very old implementations.

Since submitting events is already an async operation, I decided to just introduce a new currentEndpointProfileWithCompletion: method that won't block the main queue when it needs to grab UIApplication.isRegisteredForRemoteNotifications, and call this method instead internally.

I also decoupled AWSPinpointEndpointProfile from AWSPinpointNotificationManager, so now the isRegisteredForRemoteNotifications flag needs to be provided externally when creating the profile.

Finally, I added Warning annotations to both isNotificationEnabled and currentEndpointProfile docs indicating that calling them from a non-main thread might cause deadlocks if the main queue is blocked.

Check points:

• Added new tests to cover change, if needed
• All unit tests pass
• All integration tests pass
• Updated CHANGELOG.md
• Documentation update for the change if required
• PR title conforms to conventional commit style

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[ruisebas]

⚠️: Since AWSPinpointContext is an internal type, this init is not externally accessible so this is not a breaking change

[royjit]

This is a public header right?

[ruisebas]

Yeah, but AWSPinpointContext is defined in the [Internal](https://github.com/aws-amplify/aws-sdk-ios/tree/main/AWSPinpoint/Internal) folder and those things can't be consumed, right?

At least on my sample app, this init never appeared as an option, and attempting to use AWSPinpointContext was greyed out because "it was not imported".

But maybe that's a Swift-only thing? If you prefer I can keep and deprecate the old method, and create a new one with the extra parameter that we use.

[royjit]

ah, I see what you are saying. Even here they are marked as private - https://github.com/aws-amplify/aws-sdk-ios/blob/main/AWSPinpoint.podspec#L17, I think we are good with this change.

[ruisebas]

⚠️ : I did consider making this return a AWSTask instead, to be consistent with all the other async operations that exists in this client (e.g. updateEndpointProfile).

However I decided against it because AWSTasks by definition can fail, while this method should always return a valid non-null AWSPinpointEndpointProfile instance.

So it would just add an unnecessary validation/unwrapping for any customer that needed to use this method.