2

-Edit

I created a bug report to follow the issue

I am trying to upload a directory to my server. The folder contains big files including CT scan images. It's working fine but I have memory issues.

document.getElementById("folderInput").addEventListener('change', doThing);

function doThing(){
    var filesArray = Array.from(event.target.files);

  readmultifiles(filesArray).then(function(results){
    console.log("Result read :"+results.length);
  })
}

function readmultifiles(files) {
  const results = [];
  return files.reduce(function(p, file) {
    return p.then(function() {
      return readFile(file).then(function(data) {
        // put this result into the results array
        results.push(data);
      });
    });
  }, Promise.resolve()).then(function() {
    // make final resolved value be the results array
    console.log("Returning results");
    return results;
  });
}

function readFile(file) {

  const reader = new FileReader();

  return new Promise(function(resolve, reject) {
    reader.onload = function(e) {
      resolve(e.target.result);
    };
    reader.onerror = reader.onabort = reject;
    reader.readAsArrayBuffer(file);
  });
}

JSFiddle of the solution - Using response from this question

In this example, I do nothing with the data but you can see the memory usage growing.

Memory usage before uploading:

Memory usage after uploading:

The file folder uploaded is 342Mb so it makes sense but memory should be free, right?

If you have any idea to prevent this or maybe there is another API I could use instead of FileReader?

EDIT-----

I think this is definitely a bug linked to Chrome and V8. The memory is freed when I try on Firefox. It might be link to this bug

Bapt
  • 33
  • 6
  • You should use `Promise.all()`. – SLaks Feb 19 '18 at 15:54
  • 1
    @Bapt Try performing a memory profile and see what is holding up your object. Often you have to manually discard references in order for the interpreter to free up the memory. – Derek 朕會功夫 Feb 19 '18 at 15:59
  • @Derek朕會功夫 When i'm taking heap snapshot before and after uploading there is no difference in memory usage. – Bapt Feb 19 '18 at 16:05
  • @SLaks Using Promise.all() does not change anything. It could still be a better option but I want to read file one by one. – Bapt Feb 19 '18 at 16:08
  • This is an interesting problem. I'm wondering if eliminating scoped references might help avoid leakage. There are several scoped references that can be eliminated here, but I'm not certain if any of them are what's actually causing the leak. – Patrick Roberts Feb 19 '18 at 16:51
  • I currently wondering if it's not a bug linked to Chrome. I am trying to reproduce on Firefox but I can't. – Bapt Feb 19 '18 at 16:59

1 Answers1

1

All you need to do is remove your FileReader objects. You create them in a loop, create one, use it, create one use it, etc. The work flow should be create one, use one, delete one, create one, use one, delete one, etc.

So you've left them all in memory. Setting them to null will get them garbage collected (eventually).

Do

declare reader inside Promise handler. It will be garbage collected automatically then.
Randy Casburn
  • 13,840
  • 1
  • 16
  • 31
  • Why wouldn't they get garbage-collected with the current code? None of the closures over the `reader` variable is kept. Setting the variable to `null` should not be necessary. – Bergi Feb 19 '18 at 16:27
  • Yes, it changes nothing to set the variable to null – Bapt Feb 19 '18 at 16:30
  • The reader object does create a closure within the Promise object's .then() callback. – Randy Casburn Feb 19 '18 at 16:34
  • Ah...a closure is in two places. 1. Inside the .then() callback when reader is access with onerror, onabort and readAsArrayBuffer. 2. within the onload callback. – Randy Casburn Feb 19 '18 at 16:38
  • 1
    Yes but then the reader should be collected when the promise is resolved, no ? – Bapt Feb 19 '18 at 16:42
  • 1
    You did change the reader from const right? Since reader is only used within the Promise handler, I suggest putting the reader declaration inside the handler while still setting reader to null within the onload handler. – Randy Casburn Feb 19 '18 at 16:45
  • 1
    I just ran your fiddle with my changes and I see both before/after snapshots nearly identical. – Randy Casburn Feb 19 '18 at 16:53
  • @RandyCasburn looking into the issue more, it seems you are correct to suspect the scoped reference to `reader`. Instead of explicitly overwriting the instance with `null`, however, it makes more sense to move its instantiation inside the `Promise` callback with the rest of the code. Since there's a difference between Chrome and Firefox according to OP, it's possible that Chrome's garbage collector does not mark a `Promise` constructor's callback as inactive after it is executed if it contains a scoped reference, though more research would need to be done to confirm this possibility. – Patrick Roberts Feb 19 '18 at 17:16
  • 1
    If the `onload` handler does not directly reference `reader`, except through `e` or `this`, there should be no need to explicitly set it to `null` in there, since it does not contain any scoped references anyway and will allow `reader` to be marked for GC after the handler is invoked. – Patrick Roberts Feb 19 '18 at 17:19
  • Yeah, everything points to a closure issue. When I put the reader declaration inside the Promise handler and tested in Chrome everything was peachy. – Randy Casburn Feb 19 '18 at 17:20
  • "no need to explicitly set it to null" - agreed. – Randy Casburn Feb 19 '18 at 17:21