Kitura / SwiftyRequest

SwiftyRequest is an HTTP networking library built for Swift.
Apache License 2.0
110 stars 19 forks source link

Fix memory leak with SwiftyRequest #60

Closed Andrew-Lees11 closed 5 years ago

Andrew-Lees11 commented 5 years ago

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:

djones6 commented 5 years ago

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 (https://github.com/apple/swift-corelibs-foundation/pull/640) and shared was more recent (https://github.com/apple/swift-corelibs-foundation/pull/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 commented 5 years ago

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

ianpartridge commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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.