allow edge to gc chunks#3508
allow edge to gc chunks#3508butonic wants to merge 1 commit intoblueimp:masterfrom butonic:allow-edge-to-gc-chunks
Conversation
|
Thanks for your contribution @butonic var clone = Object.assign({}, o);
delete clone.data;You're assigning all option properties to a new object and then remove the Could you confirm that this change fixes the memory leak on edge? |
|
updated pr incoming ... I forgot to actually use clone as a parameter to That method seems to indirectly reference the object by itself, or rather by an anonymous handler listening on progress events. This seems to prevent the gc to collect the chunk data. |
|
AFAICT jQuery-File-Upload/js/jquery.fileupload.js Lines 417 to 435 in a451d60 In 419 Im not too much into garbage collection, but I have a feeling that the edge implementation does not yet handle that good enough. |
|
hm, indeed, making the --- apps/files/js/jquery.fileupload.js (date 1529351608000)
+++ apps/files/js/jquery.fileupload.js (date 1529351608000)
@@ -428,9 +428,11 @@
e.total = oe.total;
that._onProgress(e, options);
});
- options.xhr = function () {
- return xhr;
- };
+ if (options.xhr && options.xhr() !== xhr) {
+ options.xhr = function () {
+ return xhr;
+ };
+ }
}
}, |
|
hm the condition is wrong, let me see if I can polish this. |
|
debugging shows:
@blueimp Why is the xhr added to the options when initializing the progress listener? jQuery-File-Upload/jquery.fileupload.js Lines 211 to 234 in 8ae0b06 Also if settings.onProgess wes a function it would be called with the xhr in jQuery-File-Upload/jquery.fileupload.js Line 246 in 8ae0b06 and jQuery-File-Upload/jquery.fileupload.js Line 284 in 8ae0b06 Access to a Today, the only place that might rely on the objects having the xhr property is After fiddling around a bit, it seems that just cloning the options already allows edge to gc the chunks. Currently, this PR should allow code relying on |
|
It seems to be a variant of https://blog.meteor.com/an-interesting-kind-of-javascript-memory-leak-8b47d2e7f156 |
|
the options are also referenced by the jquery cache ... so maybe https://makandracards.com/makandra/31325-how-to-create-memory-leaks-in-jquery is related .... too late to dig further. will have another look tomorrow. |
|
it seems I can clean this up on our side: fileupload.on('fileuploadchunkdone', function(e, data) {
$(data.xhr().upload).unbind('progress').remove();
});will test tomorrow ... really getting too late |
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
|
Ok, so actually just unbinding the progress listener is enough to let gc happen: fileupload.on('fileuploadchunkdone', function(e, data) {
$(data.xhr().upload).unbind('progress');
});The cleanest way I could come up with was using always() to remove the progress listener. Updated this PR accordingly, Maybe this saves others some time, trying to figure out why uploads fail randomly. |
|
Thanks a lot for your investigation and fix, I've included a slightly modified version of it in the latest release. |
Try uploading > 5GB with edge. It does not matter if chunks are used or not. Edge will kill the tab process once it hits 5GB. This can be monitored quite nicely when keeping the EDGE dev tools memory tab open. When the process is killed the tools get closed. Sadly, memory is not even freed after an upload completes.
This PR allows edge to cleanup chunks. Currently, by only omitting the actual upload data from the options used to initialize the progress. Maybe it makes more sense to always omit it ... my js is not good enough to wrap my head around that.
Comments welcome.