fix(Analytics): Making PinpointEndpointProfile thread safe.

[ruisebas]

Issue #

• Datarace using pinpoint identifyUser #3439

Description

This PR makes the PinpointEndpointProfile a struct to prevent race conditions crashes when two different identifyUser invocations attempt to modify it.

General Checklist

• Added new tests to cover change, if needed
• Build succeeds with all target using Swift Package Manager
• All unit tests pass
• All integration tests pass
  • PushNotifications
  • Analytics
• Security oriented best practices and standards are followed (e.g. using input sanitization, principle of least privilege, etc)
• Documentation update for the change if required
• PR title conforms to conventional commit style
• New or updated tests include Given When Then inline code documentation and are named accordingly testThing_condition_expectation()
• If breaking change, documentation/changelog update with migration instructions

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

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (a0ee257) 68.13% compared to head (45e5867) 68.06%.
Report is 4 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3457      +/-   ##
==========================================
- Coverage   68.13%   68.06%   -0.08%     
==========================================
  Files        1085     1085              
  Lines       36248    36248              
==========================================
- Hits        24699    24671      -28     
- Misses      11549    11577      +28     

Flag	Coverage Δ
API_plugin_unit_test	67.07% <ø> (ø)
AWSPluginsCore	64.38% <ø> (ø)
Amplify	48.21% <ø> (ø)
Analytics_plugin_unit_test	81.16% <100.00%> (ø)
Auth_plugin_unit_test	79.08% <ø> (+0.05%)	⬆️
CoreMLPredictions_plugin_unit_test	59.44% <ø> (ø)
DataStore_plugin_unit_test	80.76% <ø> (-0.52%)	⬇️
Geo_plugin_unit_test	70.75% <ø> (ø)
Logging_plugin_unit_test	63.22% <ø> (ø)
Predictions_plugin_unit_test	37.29% <ø> (ø)
PushNotifications_plugin_unit_test	87.03% <100.00%> (ø)
Storage_plugin_unit_test	78.10% <ø> (ø)
unit_tests	68.06% <100.00%> (-0.08%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[atierian]

It looks like we're calling addUserId(_:), addUserProfile(_:), and setAPNsToken(_:) ourselves (hidden behind SPI) from async contexts and Tasks.

It's my understanding that the use of the DispatchQueue here, in particular with sync and .barrier, can violate the forward progress expectations of structured concurrency.

Can we turn the PinpointEndpointProfile class into an actor, update some callsites to await these methods, and call it a day?

[atierian]

Looks good. I think you're missing one change for the unit tests to pass.

diff --git a/AmplifyPlugins/Analytics/Tests/AWSPinpointAnalyticsPluginUnitTests/AWSPinpointAnalyticsPluginClientBehaviorTests.swift b/AmplifyPlugins/Analytics/Tests/AWSPinpointAnalyticsPluginUnitTests/AWSPinpointAnalyticsPluginClientBehaviorTests.swift
index 9a0529e26..0c7bad83f 100644
--- a/AmplifyPlugins/Analytics/Tests/AWSPinpointAnalyticsPluginUnitTests/AWSPinpointAnalyticsPluginClientBehaviorTests.swift
+++ b/AmplifyPlugins/Analytics/Tests/AWSPinpointAnalyticsPluginUnitTests/AWSPinpointAnalyticsPluginClientBehaviorTests.swift
@@ -57,7 +57,7 @@ class AWSPinpointAnalyticsPluginClientBehaviorTests: AWSPinpointAnalyticsPluginT
                                                plan: testPlan,
                                                location: testLocation,
                                                properties: testProperties)
-        let expectedEndpointProfile = PinpointEndpointProfile(applicationId: "appId",
+        var expectedEndpointProfile = PinpointEndpointProfile(applicationId: "appId",
                                                               endpointId: "endpointId")
         expectedEndpointProfile.addUserId(testIdentityId)
         expectedEndpointProfile.addUserProfile(userProfile)
@@ -108,7 +108,7 @@ class AWSPinpointAnalyticsPluginClientBehaviorTests: AWSPinpointAnalyticsPluginT
                                                plan: testPlan,
                                                location: testLocation,
                                                properties: testProperties)
-        let expectedEndpointProfile = PinpointEndpointProfile(applicationId: "appId",
+        var expectedEndpointProfile = PinpointEndpointProfile(applicationId: "appId",
                                                               endpointId: "endpointId")
         expectedEndpointProfile.addUserId(testIdentityId)
         expectedEndpointProfile.addUserProfile(userProfile)

[atierian]

🚀