Skip to content

Fix memory leak with SwiftyRequest#60

Merged
djones6 merged 6 commits intomasterfrom
URLSessionLeak
May 13, 2019
Merged

Fix memory leak with SwiftyRequest#60
djones6 merged 6 commits intomasterfrom
URLSessionLeak

Conversation

@Andrew-Lees11
Copy link
Contributor

This pull request fixes the memory leak caused by creating multiple URLSessions when making requests. There are three cases where URLSession holds on to memory:

  • When creating a URLSession, Every session will create an SSL cache associated to your app, which takes 10 minutes to clear, regardless of it you clear the session. For this reason you are not intended to create a new session for each request. For requests without a delegate, I have fixed this by using URLSession.shared. This uses a shared singleton session for all the requests.

  • When creating a URLSession with a delegate:
    "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."
    To solve this I have added a call to the session finishTasksAndInvalidate() to the RestRequest deinitialiser. (For URLSession.shared, this call doesn't do anything)

  • When using Circuit Breaker, The rest request is never deallocated .
    As raises in issue RestRequest not deallocating #59, When you use a CircuitBreaker in the RestRequest references circuit breaker that references handleInvocation that references the request meaning it isn't deallocated. This Leak will be address in a future PR.

@djones6
Copy link
Contributor

djones6 commented May 2, 2019

Looks like URLSession.shared.finishTasksAndInvalidate() is having an effect on Linux (contrary to the documentation).

This is probably because in Foundation, finishTasksAndInvalidate was implemented first (swiftlang/swift-corelibs-foundation#640) and shared was more recent (swiftlang/swift-corelibs-foundation#1365), and I guess this subtlety may have been missed.

In the meantime we should be able to keep track of whether we're using the shared session or not, and just avoid calling the function if so.

@ianpartridge
Copy link
Contributor

Can you raise an SR for finishTasksAndInvalidate() on .shared? We should fix that - @pushkarnk?

@ianpartridge
Copy link
Contributor

Can we use invalidateAndCancel() instead? Looks like that might work properly on Linux:
https://github.com/apple/swift-corelibs-foundation/blob/dffdfe6b7ab472648669742166fa9ee3e24c1bb6/Foundation/URLSession/URLSession.swift#L324

@Andrew-Lees11
Copy link
Contributor Author

I have added a check to see if we are using a shared session before calling finishTasksAndInvalidate().
I have also added to check for Swift 4.0 before using .shared since it was not implemented on linux then.
Finally I have made the reference from CircuitBreaker to RestRequest (Via the handleInvocation closure) Weak which breaks the retain cycle and allows RestRequest to be deallocated.
This fixes issue #59 and has stopped the memory leaks in testing.

@Andrew-Lees11
Copy link
Contributor Author

The travis tests for this PR are currently failing due to the API key for the external endpoints no longer being considered valid. This has been raised previously in issue #55.

Copy link
Contributor

@djones6 djones6 left a comment

Choose a reason for hiding this comment

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

Thanks!

@djones6 djones6 merged commit d9915f2 into master May 13, 2019
@djones6 djones6 deleted the URLSessionLeak branch May 13, 2019 12:42
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.

3 participants