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

fixed potential memory leak in AssetManager.loadQueue that would prevent the calling object from garbage collection #776

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

soccerob
Copy link

I'm not sure why this is a problem, but I spent all morning tracking down a small memory leak and was able to trace it to this. The expected behavior of the variable "onProgress", is that it would lose scope and lose its reference from the calling object as soon as the loadQueue method completes, but for some reason when loading remote assets, it doesn't.
When loading remote assets (http://<some-asset.mp3>), a reference is kept to the calling class's onProgress method which prevents the calling class from being garbage collected.

example: SampleClass will not be garbage collected because a reference to "reportProgress" exists in AssetManager

class SampleClass {
    function beginLoading():void {
      function reportProgress(ratio:Number):void {
            if (ratio >= 1.0) {
                doSomething();
            }
        }
        assetManager.enqueue(<my-mp3-file.mp3>);
        assetManager.loadQueue(reportProgress);
    }
}

I'm not sure why this is a problem, but I spent all morning tracking down a small memory leak and was able to trace it to this. The expected behavior of the variable "onProgress", is that it would lose scope and lose its reference from the calling object as soon as the loadQueue method completes, but for some reason when loading remote assets, it doesn't. 
When loading remote assets (http://<some-asset.mp3>), a reference is kept to the calling class's onProgress method which prevents the calling class from being garbage collected. 

example: SampleClass will not be garbage collected because a reference to "reportProgress" exists in AssetManager
class SampleClass {
	function beginLoading():void {
	  function reportProgress(ratio:Number):void {
			if (ratio >= 1.0) {
				doSomething();
			}
		}
		assetManager.enqueue(<my-mp3-file.mp3>);
		assetManager.loadQueue(reportProgress);
	}
}
@soccerob
Copy link
Author

oops, messed something up. fixing...

removed an incorrect line setting onProgress = null inside cancel()
@soccerob soccerob closed this Oct 22, 2015
@soccerob soccerob reopened this Oct 22, 2015
@soccerob soccerob changed the title Update AssetManager.as fixed potential memory leak in AssetManager.loadQueue that would prevent the calling object from garbage collection Oct 22, 2015
@soccerob
Copy link
Author

Would it cause any problems to call cancel() after onProgress(1.0)
If not, then you could move onProgress = null into the cancel method, and just call cancel after the final onProgress has been reported.

example:

function cancel():void
{
    removeEventListener(Event.CANCEL, cancel);
    mNumLoadingQueues--;
    canceled = true;
    onProgress = null;
}

function finish():void
{
    setTimeout(function():void
    {
        if (!canceled)
        {
            onProgress(1.0);
            cancel();
        }
    }, 1);
}

@PrimaryFeather
Copy link
Contributor

Thanks a lot for the report, and for the attempt to fix it!

Hm ... that's really weird. I can reproduce the issue, but I simply cannot understand why the onProgress parameter prevents the instance from being garbage collected. I'd love to get to the root of this, to fix it properly ... but I haven't made any progress yet.

I first thought that it's maybe the "setTimeout" calls, because I'm not properly calling "clearTimeout". But that doesn't change anything. Event listeners seem to always be correctly removed. Very odd!

In any case, moving the "cancel" call above "onProgress" is not ideal, because people might start a new queue directly from the callback; and this wouldn't work any longer, then.

A working fix would be the following:

function cancel():void
{
    removeEventListener(Event.CANCEL, cancel);
    mNumLoadingQueues--;
    canceled = true;
    onProgress = null;
}

function finish():void
{
    setTimeout(function():void
    {
        if (!canceled)
        {
            var progressFunc:Function = onProgress;
            cancel();
            progressFunc(1.0);
        }
    }, 1);
}

I haven't committed the change yet, though — I want to sleep over it, perhaps I can find the original issue, after all.

... or does anyone else have a clue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants