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

Add timeout to attributionTokenWithError to prevent hangs #6

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hsource
Copy link

@hsource hsource commented Apr 20, 2023

Motivation

In our production app, we were getting stacktraces like these:

CrashReporter Key:  b376ceb3ec365080441f66dfbab40a02757ad6bd
Hardware Model:     iPhone12,1
Process:            newiosapp
Identifier:         com.newiosapp.ios
Version:            2.90
Role:               Foreground
OS Version:         iOS 16.3.1


App Hang: The app was terminated while unresponsive

0  libsystem_kernel.dylib +0xda4   _mach_msg2_trap
1  libsystem_kernel.dylib +0x13a18 _mach_msg2_internal
2  libsystem_kernel.dylib +0x13c58 _mach_msg_overwrite
3  libsystem_kernel.dylib +0x12e8  _mach_msg
4  libdispatch.dylib +0x1f360      __dispatch_mach_send_and_wait_for_reply
5  libdispatch.dylib +0x1f6e8      _dispatch_mach_send_with_result_and_wait_for_reply
6  libxpc.dylib +0xfebc            _xpc_connection_send_message_with_reply_sync
7  Foundation +0xa30b0             ___NSXPCCONNECTION_IS_WAITING_FOR_A_SYNCHRONOUS_REPLY__
8  Foundation +0x36188             -[NSXPCConnection _sendInvocation:orArguments:count:methodSignature:selector:withProxy:]
9  Foundation +0x34958             -[NSXPCConnection _sendSelector:withProxy:arg1:]
10 Foundation +0x34890             __NSXPCDistantObjectSimpleMessageSend1
11 AdServices +0x1f60              0x20492cf60 (0x20492cd80 + 480)
12 newiosapp +0x6afd70             +[AppleAdsAttribution getAdServicesAttributionToken:] (AppleAdsAttribution.m:118:42)
13 newiosapp +0x6afe84             +[AppleAdsAttribution getAdServicesAttributionDataWithCompletionHandler:] (AppleAdsAttribution.m:144:38)
14 newiosapp +0x6b06fc             -[AppleAdsAttribution getAdServicesAttributionData:rejecter:] (AppleAdsAttribution.m:254:5)
15 CoreFoundation +0x746b0         ___invoking___
16 CoreFoundation +0x20b18         -[NSInvocation invoke]
17 CoreFoundation +0x20530         -[NSInvocation invokeWithTarget:]
18 newiosapp +0x3d48cc             -[RCTModuleMethod invokeWithBridge:module:arguments:] (RCTModuleMethod.mm:584:3)
19 newiosapp +0x3d6ac0             facebook::react::invokeInner(RCTBridge*, RCTModuleData*, unsigned int, folly::dynamic const&, int, (anonymous namespace)::SchedulingContext) (RCTNativeModule.mm:196:17)
20 newiosapp +0x3d6748             facebook

Based on reading the branch.io fix, it looks like it's possible that this function is just hanging.

Fix

Adapted fix from BranchMetrics/ios-branch-deep-linking-attribution#1114 with some minor changes to pass through errors.

Testing

Built and ran app with the changes and verified that we still successfully retrieved the token on a real iOS device.

Did not verify that the timeout works, but I'll report back in a few weeks after I release the next version to see if this fixes it.

Copy link

@pxpeterxu pxpeterxu left a comment

Choose a reason for hiding this comment

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

Looks good to me, and seems like a straightforward copy! Just one question about 100ms vs. 500ms

});

// Apple said this API should respond within 50ms, lets give up after 100 ms
dispatch_semaphore_wait(semaphore, dispatch_time(DISPATCH_TIME_NOW, (int64_t)(100 * NSEC_PER_MSEC)));

Choose a reason for hiding this comment

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

I notice that Branch.io uses 500ms as the timeout -- curious if there was a particular reason you switched to 100ms instead of 500ms

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.

2 participants