DataPublisher created from async function should start lazily

[alexvasilkov]

I need to show a photo grid where each photo has a tiny preview stored in local DB, and I want to display these previews shortly until a bigger thumbnail is loaded from remote URL.

To load local previews I'm creating a custom ImageRequest like this:

    let request = ImageRequest(
      id: "preview_\(photoId)",
      data: { await loadPreviewFromDb(photoId) },
      options: [.disableDiskCache]
    )

Now when the grid is shown only the first 6 previews are loaded and the rest are just ignored, even though I can confirm that the actual data is loaded from the DB correctly. In the logs I see Data loader returned empty data. errors.

Adding .skipDataLoadingQueue option kinda helps but I still see random blank previews from time to time.

It turns out the async data function is called eagerly when it's converted into Publisher (see DataPublisher.swift/publisher(...) code), and PassthroughSubject will ignore data that was sent while it had no subscribers (queued?).

I managed to fix it by using lazy Publisher created like this:

    let request = ImageRequest(
      id: "preview_\(photoId)",
      dataPublisher: Future { await loadPreviewFromDb(photoId) },
      options: [.disableDiskCache]
    )

private extension Future where Failure == Error {
  convenience init(action: @escaping () async throws -> Output) {
    self.init { promise in
      Task {
        do {
          let result = try await action()
          promise(.success(result))
        } catch {
          promise(.failure(error))
        }
      }
    }
  }
}

This seems to work just fine, but I think it worth fixing it in the library as well.

[kean]

Nice catch; thanks for the report. It was never intended to work this way. The async work should be performed lazily. The implementation-wise, it should probably not perform the conversion to a publisher, but it's also OK if it does. There are ways to make it lazy. I'll address it when I get some time, but in the meantime, PRs are always welcome.

[alexvasilkov]

Sorry for delayed reply, I wanted to test it in production to make sure it works, and it doesn't work that good after all.

About 0.5% of users who received this update got crashes like this:

0  libswiftCore.dylib             0x39c40c _swift_release_dealloc + 32
1  Combine                        0x1ded4 Future.__deallocating_deinit + 204
2  libswiftCore.dylib             0x39c424 _swift_release_dealloc + 56
3  PhotoCircle                    0xfe1730 type metadata accessor for PublisherCompletion + 2554352 (<compiler-generated>:2554352)
4  libswiftCore.dylib             0x39c424 _swift_release_dealloc + 56
5  PhotoCircle                    0xfe14f8 DataPublisher.__deallocating_deinit + 2553784 (<compiler-generated>:2553784)
6  libswiftCore.dylib             0x39c424 _swift_release_dealloc + 56
7  PhotoCircle                    0xfdf120 outlined destroy of ImageRequest.Resource + 2544608 (<compiler-generated>:2544608)
8  PhotoCircle                    0xfddc28 ImageRequest.Container.__deallocating_deinit + 2539240 (<compiler-generated>:2539240)

My implementation of loadPreviewFromDb function is pretty simple, it just loads Data from Realm DB, so I can't see what can be wrong with my code. But I found this thread where similar problem is discussed, so I tend to blame Future for the crashes.

I agree that it should probably not be converted to a publisher internally and better just call the async method when needed.

[b3ll]

I'm also having a similar situation, where I'm loading an image from data stored on a CoreData object

I'd like to load / decompress / cache (in memory) the image via the same Nuke pipeline I use for loading remote images, however none of the local images load :( Data loader returned empty data

extension MyObject {

  var image: UIImage? {
    get async {
      let request = ImageRequest(id: albumIdentifier, data: {
        return imageData
      })

      do {
        return try await ImagePipeline.shared.image(for: request)
      } catch {
        debugPrint("Failed to load image: \(error)")
        return nil
      }
    }
  }

}

Is this sort of use-case unsupported? Or do you have a suggested workaround (other than the publisher one above?)?

(Thanks for making a pretty slick library btw!)

[kean]

Fixed in version 12.1.1

[kean]

Hey, @b3ll.

Is this sort of use-case unsupported? Or do you have a suggested workaround (other than the publisher one above?)?

It was a defect. The goal has always been to execute the work lazily. The fix seems solid.

[alexvasilkov]

Ah, indeed, Future must be wrapped in Deferred, otherwise it will start execution upon creation. I wish Apple documentation was a little bit clearer about that.

I'm not sure using Deferred will solve the deallocation crashes I mentioned above, though it is quite likely.

Let me share my custom lazy publisher that solved the crashes for me, just in case.

private final class LazyPublisher<Output, Failure>: Publisher where Failure: Error {
  typealias Promise = (Swift.Result<Output, Failure>) -> Void

  private let handler: (@escaping Promise) -> Void

  init(_ handler: @escaping (@escaping Promise) -> Void) {
    self.handler = handler
  }

  func receive<S>(subscriber: S) where S: Subscriber, Failure == S.Failure, Output == S.Input {
    let subject = PassthroughSubject<Output, Failure>()
    subject.receive(subscriber: subscriber)

    handler { result in
      switch result {
      case .success(let value):
        subject.send(value)
        subject.send(completion: .finished)
      case .failure(let error):
        subject.send(completion: .failure(error))
      }
    }
  }
}

The usage looks similar to regular Future then:

private func loadImageData(action: @escaping () async -> Data?) -> LazyPublisher<Data, Error> {
  LazyPublisher { promise in
    Task {
      do {
        let result = try await action()
        promise(.success(result))
      } catch {
        promise(.failure(error))
      }
    }
  }
}

    let request = ImageRequest(
      id: "preview_\(photoId)",
      dataPublisher: loadImageData { await loadPreviewFromDb(photoId) },
      options: [.disableDiskCache]
    )

[kean]

Thanks for the details. I would be surprised that there is something inherently wrong with Future, but let me try to run some concurrency tests to make sure it works.

My long-term goal is to do it the other way around and update the framework to use async/await internally.

[alexvasilkov]

I'm a pretty inexperienced Swift / iOS developer, but in my understanding the Future is actually kinda broken, see this discussion of similar Future issue I shared before.

I was not able to reproduce the crashes myself but 0.5% crash rate was high enough for us to stop the rollout and release the fix asap. For us it was not a problem that Future starts eagerly because local preview loading is very fast and relatively cheap, so Future usage without Deferred would be an ok approach if not the crashes.

Deferred may help to solve the crashes or at least reduce the crash rate, but I can't be sure the crashes won't happen at all.

[kean]

I went through it, and I'm skeptical about the described scenario with blocking the threads using semaphores.

But yeah, I think the best solution would be to entirely remove Combine from the equation and add first-class support for async/await on the ImageRequest level.

[b3ll]

Thanks for the fix @kean! This makes my code so much easier