Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SWIFT TASK CONTINUATION MISUSE: tried to resume its continuation more than once, returning nil! #3650

Open
3 tasks done
hp1909 opened this issue Dec 18, 2023 · 6 comments
Open
3 tasks done
Labels

Comments

@hp1909
Copy link

hp1909 commented Dec 18, 2023

New Issue Checklist

Issue Info

Info Value
Platform Name ios
Platform Version 15.0 and above
SDWebImage Version e.g. 5.14.0
Integration Method manually
Xcode Version Xcode 15
Repro rate Usually the first time install the application
Repro with our demo prj e.g. does it happen with our demo project?
Demo project link e.g. link to a demo project that highlights the issue

Issue Description and Steps

When I wrap the sd_setImage in Swift Concurrency context, the crash happens, usually the first time I load and scroll the pretty large image list

public func loadImage(
    imageView: UIImageView,
    url: URL?,
    additionalView: UIImageView?,
    placeholderImage: UIImage?
) async -> UIImage? {
    await withCheckedContinuation { continuation in
        Task {
            await MainActor.run {
                guard let url else {
                    imageView.image = nil
                    continuation.resume(returning: nil)
                    return
                }

                imageView.sd_setImage(
                    with: url,
                    placeholderImage: placeholderImage,
                    options: [.retryFailed, .continueInBackground]
                ) { image, _, _, _ in
                    additionalView?.image = image
                    continuation.resume(returning: image)
                }
            }
        }
    }
}

Crash log:

image

image

image

The crash happens after I update SDWebImage from 5.9.1 to 5.14.0 (to use lazyDecoding config). I tried to use the latest version (5.18.7) but it still happens.

@dreampiggy
Copy link
Contributor

Didn't you use that callbackQueue context to avoid queue switch ?

I think this is indeed for that advanced control

@hp1909
Copy link
Author

hp1909 commented Dec 18, 2023

@dreampiggy Ohh I didn't aware of that context, how can I use it correctly? Thank you in advance.

@hp1909
Copy link
Author

hp1909 commented Dec 18, 2023

@dreampiggy I set callbackQueue but it still crashes. I think it's about calling completion block twice, not related to the callbackQueue.

image

@dreampiggy
Copy link
Contributor

dreampiggy commented Dec 29, 2023

Using un-attached Task with main actor and wrap that sd_setImage API is not a good idea, actually, bad idea. Becaues that API does not ensure the Main actor isolation.

Swift Actor and async is actually, different concept. They should not just hack to match the exists Objc API like your wrapper.

A proper solution is to wait my #2980

Or you can do a wrapper by direclly use API in SDWebImageManager level, and provide the extension on UIImageView as well.

@iDevPro
Copy link

iDevPro commented Apr 9, 2024

It is not only with sd_setImage that this problem occurs.

@discardableResult
func loadImage(url: URL,
               options: SDWebImageOptions,
               context: [SDWebImageContextOption: Any]?) async throws -> UIImage {

    try Task.checkCancellation()

    return try await withCheckedThrowingContinuation { [url, options, context] continuation in
        guard Task.isCancelled == false else {
            continuation.resume(throwing: CancellationError())
            return
        }

        var context: [SDWebImageContextOption: Any] = context ?? [:]

        // Select queue for work
        let queue = SDCallbackQueue.main // .current also has trouble
        queue.policy = .safeExecute // also tried without this
        context[SDWebImageContextOption.callbackQueue] = queue

        manager.loadImage(with: url,
                          options: options,
                          context: context,
                          progress: nil) { [continuation] image, _, error, _, _, _ in

            // Cancelled
            guard Task.isCancelled == false else {
                continuation.resume(throwing: CancellationError())
                return
            }

            // Error thrown
            guard let image = image else {
                // MISSUSE continuation here :)
                continuation.resume(throwing: ImageLoaderError.imageLoadFailed.error(underlyingError: error, info: ["url": url]))
                return
            }

            // Result exist
            continuation.resume(returning: image)
        }
    }
}

@dreampiggy
Copy link
Contributor

dreampiggy commented Apr 9, 2024

The bug exists because this API (loadImageWithURL) has many possible interruption usage, like.cancel, nil url, and no actual queue guarantee (sometime it will callback on caller queue, sometimes on main queue)

So, actually , pay attention:

There are no guarantee in any cases, that the call to SDWebImageManager.loadImageWithURL always callback the completionBlock on specify queue

This is why this can not be simple to wrap into a async function

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants