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

Only send error event if not from cancelling the task #21

Conversation

eliotfowler
Copy link

@eliotfowler eliotfowler commented May 17, 2020

Closes #20.

Upon cancelling a Kingfisher task, the Kingfisher library calls the completion block twice:

  • once for the request timing out
  • once for the task being cancelled

This causes issues in Rx because it detects a re-entrancy anomaly. I'm not actually sure if it causes a real issue given that the task is being cancelled from the stream being disposed of anyway, but it prints a scary message to the console and it's best to clean things up.

Important note: I don't have enough familiarity with the Kingfisher library to be confident that there's no time they would only send the task cancelled error and not from us explicitly calling task.cancel(). If that were ever to happen, we would want to re-think this solution.

@cddjr
Copy link

cddjr commented Aug 24, 2020

My solution

public func retrieveImage(with source: Source,
                              options: KingfisherOptionsInfo? = nil) -> Single<Image> {
        return Single.create { [base] single in
            var task: DownloadTask?
            let cancelTask = Disposables.create { task?.cancel() }
            
            task = base.retrieveImage(with: source,
                                          options: options) { result in
                guard !cancelTask.isDisposed else {
                    return
                }
                switch result {
                case .success(let value):
                    single(.success(value.image))
                case .failure(let error):
                    single(.error(error))
                }
            }

            return cancelTask
        }
    }

@freak4pc freak4pc closed this Jan 2, 2021
@freak4pc freak4pc deleted the branch RxSwiftCommunity:master January 2, 2021 10:21
Copy link
Member

@freak4pc freak4pc left a comment

Choose a reason for hiding this comment

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

Apparently changing the primary branch name closes all PRs. Do you mind opening a new PR with this fix? Thanks!

Comment on lines 21 to +24
case .failure(let error):
single(.error(error))
if !error.isTaskCancelledError {
single(.error(error))
}
Copy link
Member

Choose a reason for hiding this comment

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

I think you can just do case .failure(let error) where !error.isTaskCancelledError

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.

Reentrancy anomaly detected when using KingfisherManager
3 participants