Bug 37670 - NSUrlSessionDownloadTask seems to be garbage collected unexpectedly
Summary: NSUrlSessionDownloadTask seems to be garbage collected unexpectedly
Status: RESOLVED FIXED
Alias: None
Product: iOS
Classification: Xamarin
Component: Xamarin.iOS.dll ()
Version: master
Hardware: PC Mac OS
: --- normal
Target Milestone: Untriaged
Assignee: Rolf Bjarne Kvinge [MSFT]
URL:
Depends on:
Blocks:
 
Reported: 2016-01-13 23:30 UTC by Zhenya
Modified: 2016-01-21 11:24 UTC (History)
3 users (show)

Tags:
Is this bug a regression?: ---
Last known good build:

Notice (2018-05-24): bugzilla.xamarin.com is now in read-only mode.

Please join us on Visual Studio Developer Community and in the Xamarin and Mono organizations on GitHub to continue tracking issues. Bugzilla will remain available for reference in read-only mode. We will continue to work on open Bugzilla bugs, copy them to the new locations as needed for follow-up, and add the new items under Related Links.

Our sincere thanks to everyone who has contributed on this bug tracker over the years. Thanks also for your understanding as we make these adjustments and improvements for the future.


Please create a new report on Developer Community or GitHub with your current version information, steps to reproduce, and relevant error messages or log files if you are hitting an issue that looks similar to this resolved bug and you do not yet see a matching new report.

Related Links:
Status:
RESOLVED FIXED

Description Zhenya 2016-01-13 23:30:13 UTC
NSUrlSessionDownloadTask.RetainCount becomes 0 while NSUrlSessionDownloadDelegate.DidFinishDownloading is still executing. As a result, NSUrlSessionDownloadTask instances becomes invalid (all of its fields are reset).

Steps to reproduce:
1) Create an instance of NSUrlSession:
    var config = NSUrlSessionConfiguration.CreateBackgroundSessionConfiguration(BackgroundSessionID);
    config.RequestCachePolicy = NSUrlRequestCachePolicy.ReloadIgnoringLocalAndRemoteCacheData;
    session = NSUrlSession.FromConfiguration(config, new BackgroundSessionDelegate(), new NSOperationQueue());
2) Create a download task by calling session.CreateDownloadTask(NSUrlRequest request).
3) Download completes and BackgroundSessionDelegate.DidFinishDownloading(NSUrlSession session, NSUrlSessionDownloadTask downloadTask, NSUrl location) is called.
4) Very rarely, downloadTask.RetainCount becomes 0 in the middle of the execution of BackgroundSessionDelegate.DidFinishDownloading. The more time the execution takes, the higher the chances are. Also, calling GC.Collect() seems to be increasing the chances of RetainCount becoming 0.

Actual result:
downloadTask is unexpectedly released. A workaround may be hard to come up with. If downloadTask is released before we have a chance to read TaskIdentifier, then there is no way for us to tell, which task was just completed.

Expected result:
downloadTask remains valid until DidFinishDownloading completes execution.
Comment 1 Rolf Bjarne Kvinge [MSFT] 2016-01-14 14:19:12 UTC
@Zhenya,

This is very strange, the downloadTask instance's retainCount should not go to 0 unless the instance is Disposed.

Can you try this and see if it works around the issue?

    void DidFinishDownloading(NSUrlSession session, NSUrlSessionDownloadTask downloadTask, NSUrl location)
    {
        downloadTask.DangerousRetain ();
        try {
            // ...
        } finally {
            downloadTask.DangerousRelease ();
        }
    }

Also: do you have to background the app for this to occur, or does it still happen when the app is always executing in the foreground?
Comment 2 Nate Cook 2016-01-14 14:28:51 UTC
What additional info is needed?
Comment 3 Rolf Bjarne Kvinge [MSFT] 2016-01-14 14:32:51 UTC
@Nate, see comment #1.
Comment 4 Zhenya 2016-01-14 15:26:29 UTC
Yes, I tried calling DangerousRetain and DangerousRelease before submitting the defect. It did not resolve the issue.

This happens when app is executing in foreground. I haven't tried backgrounding it when reproducing the defect.

Also, no Dispose is called on the task instance and it is not declared in 'using' statement.
Comment 5 Rolf Bjarne Kvinge [MSFT] 2016-01-14 18:46:03 UTC
This is very strange.

Can you print out downloadTask.Handle and see if it changes from the entry to DidFinishDownloading and when retainCount turns 0?
Comment 6 Zhenya 2016-01-14 18:55:38 UTC
Yes, it does change. Handle is a valid value at the start of the delegate call. And it becomes zero at the end.
Comment 7 Rolf Bjarne Kvinge [MSFT] 2016-01-14 19:00:04 UTC
OK, that means the object is disposed at some point.

Do you dispose the task returned from session.CreateDownloadTask(NSUrlRequest request)? If not, can you try disposing it (as soon as you're done with it) and see if something changes?
Comment 8 Zhenya 2016-01-14 20:23:31 UTC
No, I'm not disposing the task manually. I don't see how disposing task would help. Can you elaborate on that? Especially considering that it is already disposed by Xamarin too early (before the delegate call completes). Also, what would be the moment when I can decide I'm done with the task? A delegate method? But that could be a different task object, not even created inside my code (in case if download was completed in background after the application re-launch).
Comment 9 Rolf Bjarne Kvinge [MSFT] 2016-01-15 09:29:42 UTC
@Zhenya: so my current theory is this:

1. You call session.CreateDownloadTask(NSUrlRequest request), which creates a download task.
2. Some time later, the GC determines the download task can be garbage collected, and puts the instance in a queue of instances to be finalized.
3. DidFinishDownloading is called, we lookup the download task instance, and give it to you.
4. The finalizer for the download task is executed, effectively disposing it.

3. and 4. happen on different threads, so they can occur simultaneously.

Disposing the object manually before 2. would ensure that 3. does not find the same managed instance, and instead has to create a new one, thus working around the problem (disposing the object was not meant as a real workaround, just as a test to verify the theory).

Another way to test this theory would be to create a static list of download tasks (add every instance returned from session.CreateDownloadTask to the list), and then remove those instances from the list at the end of DidFinishDownloading. Could you try this and see if it works?
Comment 10 Zhenya 2016-01-15 21:29:22 UTC
Thanks the for the explanation, Rolf. That makes sense. I tried to modify the code so that task.Dispose() is called right after task.Resume(). However, I can still reproduce the problem. Also, I noticed that calling Dispose changes the RetainCount from 4 to 0. So the task object I created does seem to be disposed correctly.
Comment 11 Rolf Bjarne Kvinge [MSFT] 2016-01-18 09:56:10 UTC
@Zhenya, can you try putting the tasks in a static list instead of disposing them and see if that works? That will prevent the GC from collecting them (if it doesn't, my theory is wrong).
Comment 12 Zhenya 2016-01-19 16:39:53 UTC
Rolf, yes, I can't reproduce the bug once I introduced that static list of tasks. We'll use that as a workaround for now. Thanks! Not sure if it resolves the issue in case if task is completed when the app is re-started..
Comment 13 Rolf Bjarne Kvinge [MSFT] 2016-01-20 18:51:07 UTC
If the app is restarted everything will be recreated, so there is no race condition with the GC in the first place.
Comment 14 Zhenya 2016-01-20 23:19:43 UTC
Ah, ok. Good to know. Do you need any other information from me? I noticed that the status of the bug is still 'NEEDINFO'. Do you think it will get fixed in Xamarin iOS at some point?
Comment 15 Rolf Bjarne Kvinge [MSFT] 2016-01-21 09:41:31 UTC
Reopening, I'll try and fix this.
Comment 16 Rolf Bjarne Kvinge [MSFT] 2016-01-21 11:24:59 UTC
Fixed.

maccore/master: 91d77a052454322ed6a81012002e45cb198e1212