CODEWITHSUNDEEP
swiftmemory-leakscombine
Peter Warbo
Peter Warbo

Reputation: 11700

swift combine sink receiveValue memory leak

I'm having a hard time with dealing with Combine. After the publisher is complete I want to update a value but whenever I update that value the memory is allocated and never goes away.

Whenever I try to assign image there is a leak. If I don't assign no leak.

EDIT: Reproducible example here: https://github.com/peterwarbo/MemoryAllocation

This is what my code looks like:

final class CameraController: ObservableObject {

    private var storage = Set<AnyCancellable>()    
    var image: UIImage?

    func capture(_ image: UIImage) {

        PhotoLibrary.saveImageToTemporaryDirectory(image) // AnyPublisher<URL, Error>
            .zip(PhotoLibrary.saveImage(image, location: self.locationObserver.location) // AnyPublisher<UIImage, Error>)
            .sink(receiveCompletion: { [weak self] (completion) in
                switch completion {
                case let .failure(error):
                    Log.error(error)
                    self?.handleCaptureError(error)
                case .finished: break
                }
            }) { [weak self] (value) in
                print(value.1) // no leak
                self.image = value.1 // leak

            }
            .store(in: &self.storage)
     }
}

I've also tried instead of using sink:

.receive(
    subscriber: Subscribers.Sink(
        receiveCompletion: { [weak self] completion in
            switch completion {
            case let .failure(error):
                Log.error(error)
                self?.handleCaptureError(error)
            case .finished: break
            }
        },
        receiveValue: { value in
            print(value.1) // no leak
            self.image = value.1 // leak            
        }
    )
)

Upvotes: 5

Views: 5641

Answers (3)

zdravko zdravkin
zdravko zdravkin

Reputation: 2378

You are using .store(in: &self.storage)

need to cancel this private var storage = Set()

storage.cancel() storage.removeAll()

and also self need to be weak

Upvotes: -1

matt
matt

Reputation: 534893

An obvious problem with your code is that you create and store a new pipeline every time capture is called. That is the opposite of how to use Combine; you might as well not be using Combine at all. The way to use Combine is to create a pipeline once and let information come down the pipeline asynchronously from then on.

You posted an example project in which you use a Future to introduce a delay in the passing of an image down a pipeline. In your project, the user chooses an image from the photo library, repeatedly. Once again, in your project you create and store a new pipeline every time an image is chosen. I rewrote the example as follows:

import UIKit
import Combine

class ViewController: UIViewController, UINavigationControllerDelegate {
    let queue = DispatchQueue(label: "Queue", qos: .userInitiated, attributes: [], autoreleaseFrequency: .workItem)
    var image: UIImage?
    var storage: Set<AnyCancellable> = []
    let publisher = PassthroughSubject<UIImage, Never>()
    override func viewDidLoad() {
        super.viewDidLoad()
        self.publisher
            .flatMap {image in
                self.futureMaker(image: image)
            }
            .receive(on: DispatchQueue.main)
            .sink(receiveCompletion: { (completion) in
            }) { (value) in
                print("finished processing image")
                self.image = value
            }
            .store(in: &self.storage)
    }
    @IBAction func didTapPickImage(_ sender: UIButton) {
        let picker = UIImagePickerController()
        picker.delegate = self
        present(picker, animated: true)
    }
    func futureMaker(image: UIImage) -> AnyPublisher<UIImage, Never> {
        Future<UIImage, Never> { promise in
            self.queue.asyncAfter(deadline: .now() + 0.5) {
                promise(.success(image))
            }
        }.eraseToAnyPublisher()
    }
}
extension ViewController: UIImagePickerControllerDelegate {
    func imagePickerController(_ picker: UIImagePickerController, didFinishPickingMediaWithInfo info: [UIImagePickerController.InfoKey : Any]) {
        dismiss(animated: true)
        guard let image = info[UIImagePickerController.InfoKey.originalImage] as? UIImage else { return }
        print("got image")
        self.publisher.send(image)
    }
}

Note the architecture: I create the pipeline once, in viewDidLoad, and whenever an image arrives I pass it down the same pipeline. Some memory is used, to be sure, because we are storing a UIImage; but it does not grow in any uncontrolled way, but levels off in an optimal manner.

enter image description here

We're using 8.4 MB after picking all the images in the library repeatedly. No problem there!

enter image description here

Also, no surplus large images are persisting. Looking at memory that comes from choosing in the image picker, one image persists; that is 2.7 MB of our 8.4 MB:

enter image description here

That is exactly what we expect.

Upvotes: 11

Asperi
Asperi

Reputation: 257533

Just by code reading... use weak self, not direct self,

}) { [weak self] (value) in
    print(value.1) // no leak
    self?.image = value.1     // << here !!
    self?.storage.removeAll() // just in case
}

also I would add delivery on main queue, as

PhotoLibrary.saveImageToTemporaryDirectory(image)
    .zip(PhotoLibrary.saveImage(image, location: self.locationObserver.location)
    .receive(on: DispatchQueue.main)          // << here !!
    .sink(receiveCompletion: { [weak self] (completion) in
    // ... other code here

Upvotes: 1

Related Questions