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

should not add url to failedURLs when timeout, cancel and so on. #707

Closed
Whirlwind opened this issue May 8, 2014 · 19 comments
Closed

should not add url to failedURLs when timeout, cancel and so on. #707

Whirlwind opened this issue May 8, 2014 · 19 comments
Labels
Milestone

Comments

@Whirlwind
Copy link
Contributor

I found I often fail to show image because of some season, but it will not display forever.

I debug it, and I found I got a NSURLErrorTimedOut, and there is a code like this:

                    if (error.code != NSURLErrorNotConnectedToInternet)
                    {
                        @synchronized(self.failedURLs)
                        {
                            [self.failedURLs addObject:url];
                        }
                    }

I think I should not add the url to failedURLs if I got a timeout. I think I can retry download to be OK, because the url is ok, and the network is bad.

I think that NSURLErrorCancelled, NSURLErrorTimedOut, NSURLErrorNotConnectedToInternet should be retry.

@benjaminjackson
Copy link

+1 on this, I think I'm being bitten by the same bug.

@PaulWoodIII
Copy link

+1 as well, or if the author thinks he really needs this feature, maybe make it an optional feature to add those failed URLs with a boolean we could set after initializing the SDWebImageManager

@PaulWoodIII
Copy link

Backed up my request with some code, as for now on my own projects, after I download via cocoapods I just comment out [self.failedURLs addObject:url];

After working with that code, I realized that the property should be placed in the header file, so hopefully I'll make another pull request shortly

@rs
Copy link
Member

rs commented May 21, 2014

Ok to not add URL as failed in case of timeout. I will accept a pull request for this.

@PaulWoodIII
Copy link

I could do that but should I remove the feature or modify the feature to make it optional?

@Whirlwind
Copy link
Contributor Author

I think the best way is to set the default option for adding fail url.

@rs
Copy link
Member

rs commented May 21, 2014

Only permanent errors should be added to failed URLs by default. You may add an option to treat temporary errors like timeout as failures if you want though.

@PaulWoodIII
Copy link

after doing some research I have a different proposition, how about these different Error codes don't cause the URL to be added to failedURLs as well,

NSURLErrorTimedOut
NSURLErrorNetworkConnectionLost

And we just add these to that if statement @Whirlwind has shared above. Then we don't have this weird option and things work as proposed by @Whirlwind

@Whirlwind
Copy link
Contributor Author

Why not NSURLErrorCancelled?

@PaulWoodIII
Copy link

Apple docs say "Returned when an asynchronous load is canceled." so this would occur if the user canceled the SDWebImageOperation right? I agree this shouldn't cause the URL to be added to the failedURLs as well

@Whirlwind
Copy link
Contributor Author

Is it like this:

if (error.code != NSURLErrorNotConnectedToInternet && error.code != NSURLErrorCancelled && error.code != NSURLErrorTimedOut) {
@synchronized(self.failedURLs)
                        {
                            [self.failedURLs addObject:url];
                        }
}

right?

@staufman
Copy link

+1 for including NSURLErrorTimedOut as an error code that doesn't add to failedURLs. I have made this change to my local codebase but would love to see it make its way into the main repo.

@bpoplauschi
Copy link
Member

Can anyone create a pull request with this?

@Whirlwind
Copy link
Contributor Author

@bpoplauschi the pull request like #766 ?

@bpoplauschi bpoplauschi modified the milestones: 3.7.0, Future Jun 17, 2014
bpoplauschi added a commit that referenced this issue Jun 17, 2014
should not add url to failedURLs when timeout, cancel and so on. #707
@bpoplauschi
Copy link
Member

Fixed by #766

@hasanalbukhari
Copy link

if (error.code != NSURLErrorNotConnectedToInternet && error.code != NSURLErrorCancelled && error.code != NSURLErrorTimedOut)

This failed to detect not connected to internet. I just removed [self.failedURLs addObject:url]; even if it was a broken link.

I am getting kCFURLErrorCannotConnectToHost which is -1004. when I am not connected to internet. that should be added (Maybe).

I prefer to remove failedURLs for good.
Otherwise, it was very helpful. Thank you very much.

@erango
Copy link

erango commented Aug 27, 2015

On iOS > 6, the error codes that don't make it to shouldBeFailedURLAlliOSVersion can still make it to shouldBeFailedURLiOS7 and ruin things still, since the code checks either of them to be YES.

@bpoplauschi
Copy link
Member

@erango check #1297, it should fix the issue you are describing. Please note this is only merged into master and not yet included in any release.

@erango
Copy link

erango commented Oct 13, 2015

@bpoplauschi thanks!

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

8 participants