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

Fix background download #2500

Merged
merged 2 commits into from Dec 27, 2018

Conversation

@zhongwuzw
Copy link
Member

zhongwuzw commented Oct 11, 2018

New Pull Request Checklist

  • I have read and understood the CONTRIBUTING guide

  • I have read the Documentation

  • I have searched for a similar pull request in the project and found none

  • I have updated this branch with the latest master to avoid conflicts (via merge from master or rebase)

  • I have added the required tests to prove the fix/feature I am adding

  • I have updated the documentation (if necessary)

  • I have run the tests and they pass

  • I have run the lint and it passes (pod lib lint)

This merge request fixes / reffers to the following issues: ...

Pull Request Description

Yeah, background download would never valid, because we mark task end immediately after create.

@zhongwuzw zhongwuzw requested review from bpoplauschi and dreampiggy Oct 11, 2018
@bpoplauschi

This comment has been minimized.

Copy link
Member

bpoplauschi commented Oct 28, 2018

@zhongwuzw I took a look at this PR and the code from 5.x branch (and master) is odd to me.
Not sure your fix is entirely correct.

Here's how I see it:

  • when you want to create a background task you need to
  • 1 - create the task
  • 2 - register as background task: call Application beginBackgroundTaskWithExpirationHandler where the expiration handler will cancel the task and call endBackgroundTask
  • 3 - now 2 cases
    • a - if the task is not finished and the app goes to background, the system will let it run. Only if reaching the maximum allowed time for a background task the system will call the expiration handler which basically ends the task
    • b - if the task is finished and the app is still in foreground, we just need to call endBackgroundTask so the background task is unregistered. Otherwise, the app might be killed if going to background.

From what I can see, you added this code in cancel method, which is not called when the task is done. I think we can call if from within done.
Also, I would refactor the background task code into a separate component.

@bpoplauschi

This comment has been minimized.

Copy link
Member

bpoplauschi commented Oct 28, 2018

@zhongwuzw

This comment has been minimized.

Copy link
Member Author

zhongwuzw commented Oct 29, 2018

@bpoplauschi Emm, seems you overlooked? 😂 I put it in reset method, not cancel.

@zhongwuzw

This comment has been minimized.

Copy link
Member Author

zhongwuzw commented Oct 29, 2018

Also, I would refactor the background task code into a separate component.

@bpoplauschi You mean separate to two methods, one for start background task, another for end? 🤔

@kinarobin

This comment has been minimized.

Copy link
Contributor

kinarobin commented Oct 29, 2018

@zhongwuzw Is there a situation like this: When the app enter background, the image download background, If so the app goes into the foreground, this moment need to end background task at start method ?

if (self.ownedSession) {
[self.ownedSession invalidateAndCancel];
self.ownedSession = nil;
@synchronized (self) {

This comment has been minimized.

Copy link
@dreampiggy

dreampiggy Oct 29, 2018

Contributor

?What the reason to use @ synchronized. The self.dataTask and self.ownedSession is immutable after we initilize them in in the start method. No any write operation to that operation.

This comment has been minimized.

Copy link
@bpoplauschi

bpoplauschi Oct 29, 2018

Member

Agree - I don't see why we would need @synchronized here

This comment has been minimized.

Copy link
@dreampiggy

dreampiggy Oct 29, 2018

Contributor

Check you logic again. I remember there are previous a crash because of self.imageData = nil (trigger from user's [operation cancel] queue) can happend when URLSessionDelegate call didReceiveData: (triggered in URLSession delegate queue) at the same time. See #2011

However, thedataTask.cancel is different case from imageData. If you walkthrough this logic, there should not be chance to trigger self.dataTask = nil && self.dataTask at the same time in different queue. Because this is a NSOperation, the cancel state will cause the operation queue remove it. While the URLSession will also stop further delegate callback.

This comment has been minimized.

Copy link
@zhongwuzw

zhongwuzw Oct 29, 2018

Author Member

No, operation queue remove operation only if it's not started, that is executing is NO. session delegate queue, coder queue, both can call done method, and any other thread can be called when user call cancel. Think about thread context switch and parallel, concurrency, I don't know what you said no chance.

This comment has been minimized.

Copy link
@dreampiggy

dreampiggy Oct 29, 2018

Contributor

Could you give some code path ? I remember at that time (Half year ago) I check the total path for that thread-safe issue. Checked any write operation insdie reset method. Other property does not cause thread-safe issue.

This comment has been minimized.

Copy link
@zhongwuzw

zhongwuzw Oct 30, 2018

Author Member

User call cancel and url session queue call done can enter reset method concurrently, right? If it is, can concurrently execute code below, right ?

    self.dataTask = nil;
    
    if (self.ownedSession) {
        [self.ownedSession invalidateAndCancel];
        self.ownedSession = nil;
    }
@bpoplauschi

This comment has been minimized.

Copy link
Member

bpoplauschi commented Oct 29, 2018

@bpoplauschi Emm, seems you overlooked? 😂 I put it in reset method, not cancel.

@zhongwuzw sorry, the diff page mislead me. reset is probably a very good place for this code, as we actually reset the background task :)

screen shot 2018-10-29 at 9 57 41 am

@bpoplauschi

This comment has been minimized.

Copy link
Member

bpoplauschi commented Oct 29, 2018

@bpoplauschi You mean separate to two methods, one for start background task, another for end? 🤔

@zhongwuzw probably something like https://github.com/bradgmueller/CFABackgroundTask

@zhongwuzw

This comment has been minimized.

Copy link
Member Author

zhongwuzw commented Nov 15, 2018

@bpoplauschi @dreampiggy Guys, this bug should also be fixed in 4.x, background download has a long time not available, if you still stick to lock performance, maybe any of you can give a PR to review, because compared to the lock performance, bug needs fixed first. 🤔

@dreampiggy

This comment has been minimized.

Copy link
Contributor

dreampiggy commented Dec 23, 2018

This usage of background task contains issue. So maybe this is why current we disable this function.

We use weak-strong dance inside the expiration handler, but however, if the self become nil inside that handler, we will miss the endBackgroundTask call and UIKit will kill the app. The call inside that cancel or reset does not help, because there it's always possible that one operation can executed over the time limit of that background task.

So I now know it's a bug, but this PR's fix is not correct and will potential make thing worse. We'd better fix it from the scratch

@zhongwuzw

This comment has been minimized.

Copy link
Member Author

zhongwuzw commented Dec 24, 2018

?If operation is running, at least operation queue and downloader would retain operation, so operation is always valid, if operation is be canceled or done, reset method can be called, background task can balanced.
So If expiration hander be called, which means operation is still running, operation would not be nil.

@dreampiggy

This comment has been minimized.

Copy link
Contributor

dreampiggy commented Dec 24, 2018

This relay on the NSOperationQueue or NSOperation behavior. This assume that

if the NSOperation is dealloced, its cancel method must been called before it.

However, I don't know whether it's true or not and don't care about it.

The general correct writing of the background task identifier, it's to use a __block variable (for instance) or static variable to ensure it's always bind to the expiration handler. It's always safe.

__block UIBackgroundTaskIdentifier taskID = UIBackgroundTaskInvalid;
taskID = [UIApplication.sharedApplication beginBackgroundTaskWithExpirationHandler:^{
    [UIApplication.sharedApplication endBackgroundTask:taskID];
}];
@dreampiggy

This comment has been minimized.

Copy link
Contributor

dreampiggy commented Dec 24, 2018

Sorry for that I'm busy these weaks. I previously decide to fix it with another PR but I forgot that.

@zhongwuzw

This comment has been minimized.

Copy link
Member Author

zhongwuzw commented Dec 24, 2018

This relay on the NSOperationQueue or NSOperation behavior. This assume that

if the NSOperation is dealloced, its cancel method must been called before it.

However, I don't know whether it's true or not and don't care about it.

The general correct writing of the background task identifier, it's to use a __block variable (for instance) or static variable to ensure it's always bind to the expiration handler. It's always safe.

__block UIBackgroundTaskIdentifier taskID = UIBackgroundTaskInvalid;
taskID = [UIApplication.sharedApplication beginBackgroundTaskWithExpirationHandler:^{
    [UIApplication.sharedApplication endBackgroundTask:taskID];
}];

We can't use __block or even static variable in here, because we need to mark task end ASAP when operation have already finished, not just wait system tell us remaining time reaches 0.

And I think the nil thing would not happen in our framework, only when user don't use NSOperationQueue, don't use downloader, and call [operation start] directly, then dealloc it , if you guys think this exist, I can add a check in dealloc method.

@dreampiggy

This comment has been minimized.

Copy link
Contributor

dreampiggy commented Dec 25, 2018

I see.

Actually I means, from a general approach this style of code is not safe. We'd better provide two level protect for this. Because the background task is really easy to mass up on your code. And once the enter && end is not balanced, iOS system will kill your app in the background, it's a serious issue and I think we should take carefully to take the responsible for this.

My idea is that to add two-level protect, like this:

__block UIBackgroundTaskIdentifier taskID = UIBackgroundTaskInvalid;
__weak typeof(self) wself = self;
taskID = [UIApplication.sharedApplication beginBackgroundTaskWithExpirationHandler:^{
    __strong typeof(wself) sself = wself;
    UIBackgroundTaskIdentifier validTaskID = sself ? sself.taskID : taskID; // when self is dealloced, use the __block associated one to check
    if (validTaskID !=UIBackgroundTaskInvalid ) {
        [UIApplication.sharedApplication endBackgroundTask:validTaskID];   
    }
}];
self.taskID = taskID;
@dreampiggy

This comment has been minimized.

Copy link
Contributor

dreampiggy commented Dec 25, 2018

Any way, it's OK for this if you can ensure this does not cause extra issue. By running your local test or mock test from your Apps.

Because this issue exist in the 4.x branch as well. You can keep your current code, then change the base branch to master, after I merge it to the master branch, I can easily merge master to 5.x again.

It's better than you create two different PRs and let me merge it one by one. That will cause the merge conflict when I merge master to 5.x)

@dreampiggy dreampiggy added the fix label Dec 25, 2018
@dreampiggy dreampiggy added this to the 4.4.4 milestone Dec 25, 2018
@zhongwuzw zhongwuzw changed the title FIx background download Fix background download Dec 25, 2018
@zhongwuzw zhongwuzw changed the base branch from 5.x to master Dec 25, 2018
@zhongwuzw zhongwuzw changed the base branch from master to 5.x Dec 25, 2018
FIx background download

Add task check when operation will deallocated && tidy code

Tidy code further

Tidy further
@zhongwuzw zhongwuzw force-pushed the zhongwuzw:fix-background-download branch to 9617a34 Dec 26, 2018
@zhongwuzw

This comment has been minimized.

Copy link
Member Author

zhongwuzw commented Dec 26, 2018

@dreampiggy I tidy the code and take operation deallocated directly into account. We need to cherry pick this commit to master after this merged, change base branch may not work, two branches contain giant differences.

@dreampiggy

This comment has been minimized.

Copy link
Contributor

dreampiggy commented Dec 27, 2018

@zhongwuzw Seem CI failed. Our some test case relay on an AWS server to host the image. But recently the server is down. Maybe we'd better fix it or find another image source.

Such as this link: https://s3.amazonaws.com/fast-image-cache/demo-images/FICDDemoImage001.jpg

@dreampiggy

This comment has been minimized.

Copy link
Contributor

dreampiggy commented Dec 27, 2018

@bpoplauschi Is that AWS server owned by you or do you know anyone who can fix it ? Or maybe we'd better switch to another image resources.

@zhongwuzw

This comment has been minimized.

Copy link
Member Author

zhongwuzw commented Dec 27, 2018

Seems we use the images from external websites. So the images may missed. I suggest replace all our image urls, instead, create a static resources project in SD organization to host our own images. To prevent copyright issues, we can find photos from some free photos websites.

@dreampiggy dreampiggy merged commit 38a9222 into SDWebImage:5.x Dec 27, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@zhongwuzw

This comment has been minimized.

Copy link
Member Author

zhongwuzw commented Dec 28, 2018

@dreampiggy I' ll cherry-pick this PR to master.

@dreampiggy dreampiggy mentioned this pull request Dec 28, 2018
8 of 8 tasks complete
@dreampiggy

This comment has been minimized.

Copy link
Contributor

dreampiggy commented Dec 28, 2018

@zhongwuzw See #2570 and I'll merge that to master

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.