Crash on AuthorizationMoyaPlugin.prepare

angelolloqui commented 2 years ago

What did you do?

We have an app with several thousand sessions per day and using the GetStream sdk for an activity feed. We have seen that most of our crash reports (around 90% of them, a total of 0.1% of our total sessions are affected, with our current numbers is about 200 crashes/week) are coming from GetStream Moya plugin. Here is the crash report:

Crashed: org.alamofire.session.rootQueue.requestQueue
EXC_BAD_ACCESS KERN_INVALID_ADDRESS 0x000000098c54bc20
Crashed: org.alamofire.session.rootQueue.requestQueue
0  libobjc.A.dylib                0x3970 objc_retain + 16
1  libswiftCore.dylib             0x3db6fc swift_bridgeObjectRetain + 48
2  libswiftFoundation.dylib       0x2f4cc8 URLRequest.addValue(_:forHTTPHeaderField:) + 48
3  GetStream                      0x13ad0 AuthorizationMoyaPlugin.prepare(_:target:) + 180 (<compiler-generated>:180)
4  GetStream                      0x13b50 protocol witness for PluginType.prepare(_:target:) in conformance AuthorizationMoyaPlugin + 20 (<compiler-generated>:20)
5  Moya                           0x11554 closure #1 in MoyaProvider.interceptor(target:) + 169 (MoyaProvider+Internal.swift:169)
6  Moya                           0xbcec specialized MoyaRequestInterceptor.adapt(_:for:completion:) + 133 (Moya+Alamofire.swift:133)
7  Moya                           0xb894 protocol witness for RequestAdapter.adapt(_:for:completion:) in conformance MoyaRequestInterceptor + 28 (<compiler-generated>:28)
8  Alamofire                      0xb7388 specialized Session.performSetupOperations(for:convertible:shouldCreateTask:) + 1078 (Session.swift:1078)
9  Alamofire                      0xb61f4 closure #1 in closure #1 in Session.perform(_:) + 1015 (Session.swift:1015)
10 Alamofire                      0x3ea20 thunk for @escaping @callee_guaranteed () -> () + 28 (<compiler-generated>:28)
11 libdispatch.dylib              0x1e68 _dispatch_call_block_and_release + 32
12 libdispatch.dylib              0x3a2c _dispatch_client_callout + 20
13 libdispatch.dylib              0xb124 _dispatch_lane_serial_drain + 668
14 libdispatch.dylib              0xbcb4 _dispatch_lane_invoke + 444
15 libdispatch.dylib              0xb000 _dispatch_lane_serial_drain + 376
16 libdispatch.dylib              0xbcb4 _dispatch_lane_invoke + 444
17 libdispatch.dylib              0xb000 _dispatch_lane_serial_drain + 376
18 libdispatch.dylib              0xbc80 _dispatch_lane_invoke + 392
19 libdispatch.dylib              0x16500 _dispatch_workloop_worker_thread + 648
20 libsystem_pthread.dylib        0x10bc _pthread_wqthread + 288
21 libsystem_pthread.dylib        0xe5c start_wqthread + 8

I have tried using the tentative fix contained in cd5dffc725f426701e7800e741937bb123dbee78 but did not help either.

I am not sure what to do next, because it seems like there is some kind of race condition, maybe due to the memory management in the background thread.

Apart from that, in general third party SDKs should minimize dependencies to external libraries to slim them as much as possible, so I wonder why do you need Moya at all instead of implementing the calls yourselves.

What did you expect to happen?

No crash an no big dependencies like Moya or Alamofire included in the SDK

What happened instead?

Crash 0.1% of the sessions (90% of our total crashes)

GetStream Environment

GetStream version: 2.2.2 (and cd5dffc725f426701e7800e741937bb123dbee78 commit too) Xcode version: 13.2.1 (others too) Swift version: 5 Platform(s) running GetStream: iOS and Android macOS version running Xcode: 12.3 (others too)

Additional context

Affecting all iOS versions in expected proportion compared to our userbase:
Affecting to all iPhone devices in expected proportion compared to our userbase:
I do not see any pattern on user behavior. Many happening after a fresh app start, but some exceptions exist too.

angelolloqui commented 2 years ago

BTW, I think you are running on a thread race condition due to the mutating nature of the token in your plugin. See this for a similar example.

Instead, the token plugin should be constant (let) and then maybe reset the networkAuthorization variable (and the network provider configuration) when token needs to be refreshed

b-onc commented 2 years ago

Hello @angelolloqui

Thank you for the report & suggestions!

I'm looking into this and will keep you updated.

angelolloqui commented 2 years ago

Any update on this @b-onc ? it is the most frequent crash in our app, about 10x more than the next top crash

nuno-vieira commented 2 years ago

Hi @angelolloqui,

Version 2.2.5 has been released with this fix. Let us know if everything is okay.

Best, Nuno

angelolloqui commented 2 years ago

Awesome! I am updating right now! I will try to report results in a few days. Thanks again for the support!

angelolloqui commented 2 years ago

I can confirm that after almost a month with this on production all crashes have dissapear! thanks!

GetStream / stream-swift