[iOS] ExpoRequestInterceptorProtocol implementation leads to memory leaks and excessive usage of URLSession instances

[hakonk]

Summary

When observing the memory graph after in the Bare Expo app, I found something problematic. It seems several instances of ExpoRequestInterceptorProtocol were leaked. After looking at the implementation, it is clear why:

@objc(EXRequestInterceptorProtocol)
public final class ExpoRequestInterceptorProtocol: URLProtocol, URLSessionDataDelegate {
  private static let REQUEST_ID = "ExpoRequestInterceptorProtocol.requestId"
  private static var requestIdProvider = RequestIdProvider()
  private lazy var urlSession = URLSession(
    configuration: URLSessionConfiguration.default,
    delegate: self, // <- this is the culprit
    delegateQueue: nil
  )
...
}

The reason is pointed out in the Apple docs:
"The session object keeps a strong reference to the delegate until your app exits or explicitly invalidates the session. If you do not invalidate the session by calling the invalidateAndCancel() or finishTasksAndInvalidate() method, your app leaks memory until it exits."

There is nothing calling the above mentioned methods that would in turn invalidate the URLSession instance. Thus, there will be retain cycle.

Another issue with the implementation is that each instance of ExpoRequestInterceptorProtocol will create its own instance of URLSession each time it is initialised. This is contrary to Apple's best practices for how to employ URLSession, i.e., one should not create a new URLSession per request. Additionally, wouldn't this also mean that each request is sent twice?

Perhaps it is better to method swizzle the URLSessionDelegate implementation of RCTHTTPRequestHandler instead?

What platform(s) does this occur on?

iOS

SDK Version

No response

Environment

expo-env-info 1.2.0 environment info:
System:
OS: macOS 14.5
Shell: 5.9 - /bin/zsh
Binaries:
Node: 22.0.0 - ~/.nvm/versions/node/v22.0.0/bin/node
Yarn: 1.22.21 - ~/.nvm/versions/node/v22.0.0/bin/yarn
npm: 10.5.1 - ~/.nvm/versions/node/v22.0.0/bin/npm
Watchman: 2024.05.06.00 - /opt/homebrew/bin/watchman
Managers:
CocoaPods: 1.14.2 - /Users/user/.rvm/gems/ruby-2.7.2/bin/pod
SDKs:
iOS SDK:
Platforms: DriverKit 23.5, iOS 17.5, macOS 14.5, tvOS 17.5, visionOS 1.2, watchOS 10.5
Android SDK:
API Levels: 26, 33, 34
Build Tools: 26.0.3, 30.0.3, 33.0.1, 34.0.0, 35.0.0
System Images: android-26 | Google APIs Intel x86_64 Atom, android-34 | Google APIs ARM 64 v8a
IDEs:
Android Studio: 2023.3 AI-233.14808.21.2331.11709847
Xcode: 15.4/15F31d - /usr/bin/xcodebuild
Expo Workflow: bare

Minimal reproducible example

• Clone https://github.com/hakonk/expo/
• Install dependencies and open the Bare Expo project.
• Enable malloc stack logging in the scheme for the app.
• Run the Bare Expo app, open the different tabs, and select a few elements from the lists.
• Click "Debug memory graph" in Xcode.
• Filter memory graph for leaks and observe there are several leaked ExpoRequestInterceptorProtocol instances.

[hakonk]

@Kudo @brentvatne I have a work in progress branch with a proposed solution that I believe can work quite well. tl;dr: Intercept the calls made by the React's RCTHTTPRequestHandler by making use of the objc runtime. With this approach, all requests made by the app can be intercepted without the need for creating a new url session. This also solves the problem of the retain cycle. The old solution would also work for other URLSession instances. However, as these instances are created in Expo Core, one can easily intercept the calls with by passing a delegate upon initialising the URLSession instance. Please let me know what you think ☺️

[Kudo]

hi @hakonk, thanks for bringing this great issue. i did not notice the invalidateAndCancel / finishTasksAndInvalidate note until you mentioned to me. i had a fix in #29798 to fix the leakage by reusing the shared URLSession. as your branch that's not ideal for our case because we want to monitor all native requests than just react's XHR. hopes that makes sense to you.

[hakonk]

Hi, @Kudo! I'm glad you are addressing the issue with the leaks.

There are however some other problematic things about the implementation that I would like to point out:

From a test run of Bare Expo with malloc stack logging enabled, I can see in the memory graph that there are 45 instances of URLSession. This can be seen in the screenshot below:

While you have found a solution to the problem with the leaked instances of URLSession, it is still problematic that such a high number of URLSession instances have at all been initialised. According to the Apple documentation, one should treat an instance of URLSession much in the same way as a tab in a browser. That is, one URLSession instance should dispatch many requests. One should however not create an instance per request. If I understand this correctly, not only is the ExpoRequestInterceptorProtocol implementation against best practices, one is actually doubling the number of network requests, because a ExpoRequestInterceptorProtocol is added as a URLProtocol type for the URLSessionConfiguration, which in turn means that each task created on the URLSession creates another URLSession with a duplicate request . The growing number of URLSession instances for each request is in my opinion a symptom for this. Moreover, in that case one is not actually intercepting the original request.

Another problematic thing is that when swizzling the default URLSessionConfiguration, one may either swizzle too late, which means some requests are never intercepted. But more importantly, via RCTSetCustomNSURLSessionConfigurationProvider, one can provide a custom URLSessionConfiguration into React. If this configuration is not based on the default one, all requests made by RCTHTTPRequestHandler will never be intercepted.

If you want to intercept all network requests made in the app, I believe it is better to swizzle URLSession.delegate and return a delegate proxy through which all the requests can truly be intercepted.

Edit: I can see that URLSession is now static in the PR. That at least reduces the number of instances, but does not fix the issue of request duplication.

[Kudo]

Edit: I can see that URLSession is now static in the PR. That at least reduces the number of instances, but does not fix the issue of request duplication.

can you double check if the duplicate requests are true? i tested with mitmproxy and it looks only having single request to me.

i may not familiar with enough for URLSession but my current understanding is that when canInit() returns true. it means we are taking over the request handling and have to send the requests by our own.

please feel free to correct me or if i missed anything from you. thanks again for looking into the problem.

[hakonk]

Edit: I can see that URLSession is now static in the PR. That at least reduces the number of instances, but does not fix the issue of request duplication.

can you double check if the duplicate requests are true? i tested with mitmproxy and it looks only having single request to me.

i may not familiar with enough for URLSession but my current understanding is that when canInit() returns true. it means we are taking over the request handling and have to send the requests by our own.

please feel free to correct me or if i missed anything from you. thanks again for looking into the problem.

I will look into it when I have the time, but it sounds like you are right. My apologies, I merely tested with breakpoints. I believe the issue of missing interception for other configuration than default still persists though. Curious to hear your thoughts on that.

[Kudo]

I believe the issue of missing interception for other configuration than default still persists though. Curious to hear your thoughts on that.

that's valid. having non-default URLSessionConfiguration is kind of out of my original scope. let's see if people have this requirement first. i guess i'll try to either expose ask people to manually add the protocolClasses or swizzle RCTSetCustomNSURLSessionConfigurationProvider then, neither to be a perfect solution though.

[Kudo]

i'm landing #29798 and close this issue. if there'are any further issues, please create new issue or we can still follow up here. i'm super glad to have you discussing issues with in-depth insight.

[hakonk]

i'm landing #29798 and close this issue. if there'are any further issues, please create new issue or we can still follow up here. i'm super glad to have you discussing issues with in-depth insight.

Likewise! Thanks for the fix and the discussion.