Cancel download task when response status code invalid

[zhongwuzw]

I think we don't need to go on downloading data if the status code is invalid.

[onevcat]

Thanks for this PR. However, it is not doing the same thing.

By calling the completion handler with .cancel, you will get a "cancelled" error. In some cases, it is important for users to know why the download does not succeed, it requires we to get the whole response body (maybe it could be some permission errors or not found). By allowing the request continuing, more information is provided to know what's going on.

What's your opinion on it?

[zhongwuzw]

@onevcat Emm, IMO, status code should do this thing, to tell user what's wrong with it, e.x. not found 404, forbidden 403 (status code seems more clear than response body and also is its responsibility), if status code belongs to error category, which means this request is invalid, for us the image library, means no valid image.

But if user always wants to get the response data, no matter what error code is, I think he can implement the delegate of isValidStatusCode(_ code: Int, for downloader: ImageDownloader) and always return true, so he can get all what he wants. @onevcat What do you think 🤔 ?

[onevcat]

Hi, @zhongwuzw

Yes, a status code could indicate what happens. However, by calling completionHandler with .cancel would result in an error with code -999 "user cancelled", instead of an error containing the status code. So it will erase the error information and users will have no idea on what happened.

The error handing of Kingfisher is quite old, from the age of Swift 1.x, before Error comes out. I have an idea to make it more compatible with latest tech. Maybe we could improve it in a major update later for Swift 5. But not in this way, I am afraid...

[zhongwuzw]

@onevcat Emm, am I missing something? we call completion handler in place, and in callCompletionHandlerFailure, we would clean fetchload, so no erasing issue would happen.

[onevcat]

@zhongwuzw Maybe you misunderstood me. I mean, if the request fails due to an invalid status code number after we .allow it in this delegate method, the user could get a detail response error with its actual code to know what happened. However, if we .cancel it here, only an NSURLError with code -999 (user canceled) will be thrown. It hides the real reason of the error.

[zhongwuzw]

@onevcat 🤔 Emm, could you please read the code again to ensure what you want to express? 😂 We call callCompletionHandlerFailure In urlSession(_:dataTask:didReceive:completionHandler:), it removes downloadHolder fetchLoad or any callbacks.

TBO. After this PR, no things breaking.

[onevcat]

No, the error which is passed to users is different.

[zhongwuzw]

Emm, can you show me the code link that I can figure out? Where it would passed to users? Thanks. - URLSession:task:didCompleteWithError: would be called, but nothing would do because every callback has been cleaned.

Sorry I truly can't find any issue like you said. 😢

[onevcat]

@zhongwuzw Oh, sorry. I think I was wrong with it. You are right, the URL session handler would do nothing since the fetch load happens to be cleared already.

It is a bit hidden and more like a side effect. I am not quite like it and it prints out some confusing logs like URL request errored with -999 user-canceled. But you are right, the behavior of Kingfisher does not change for it and it improves since no additional loading required.

Thanks for the p-r!