[whatwg/streams] Fix memory leak in pipe loop reference implementation (#968)

The reference implementation of `ReadableStream.pipeTo` leaks memory when piping a lot of chunks.

## Steps to reproduce

1. Go to the project root
1. Run `npm i heapdump`
1. Create a file called `leak.js` containing the code snippet below
1. Run `node --expose_gc leak.js`
1. Open the two created snapshots (`before.heapsnapshot` and `after.heapsnapshot`) in Chrome DevTools
    1. Click the Memory tab
    1. Right-click in the Profiles sidebar
    1. Select Load
    1. Open the heap snapshot file
1. Select the `after` profile and switch to the Comparison view

<details>
<summary>leak.js</summary>

```js
'use strict';

const { ReadableStream } = require('./reference-implementation/lib/readable-stream.js');
const { WritableStream } = require('./reference-implementation/lib/writable-stream.js');
const heapdump = require('heapdump');

const delay = ms => new Promise(resolve => setTimeout(resolve, ms));

(async () => {
  let controller;
  const readable = new ReadableStream({
    start(c) {
      controller = c;
    }
  });
  const writable = new WritableStream(); // no-op sink
  const promise = readable.pipeTo(writable);

  gc();
  heapdump.writeSnapshot('./before.heapsnapshot');

  for (let i = 0; i < 1e4; i++) {
    controller.enqueue(i);
  }
  await delay(0);

  gc();
  heapdump.writeSnapshot('./after.heapsnapshot');

  controller.close();
  await promise;
})();

```
</details>

## Expected results
* The difference in total memory usage is small.
* The difference in amount of `Promise` instances (`# Delta`) is zero.

## Actual results
* The difference in total memory usage is ~1 MB.
* The difference in amount of `Promise` instances is 10,000.

![diff-leak](https://user-images.githubusercontent.com/649348/48968855-b6574a00-eff5-11e8-862e-9c4937b569f4.png)

## Root cause analysis

The cause of this memory leak is due to [`pipeLoop` creating a long promise chain using recursion](https://github.com/whatwg/streams/blob/46c3b89dd3aff28b2fc381dd1d397c12b4fb8a16/reference-implementation/lib/readable-stream.js#L171):
```js
function pipeLoop() {
  if (shuttingDown === true) {
    return Promise.resolve();
  }

  return writer._readyPromise.then(() => {
      // snipped
  })
  .then(pipeLoop); // <<< recursion!
}
```
Every time a chunk is piped through, the promise chain is *extended*. This chain keeps a ton of promises alive, and cannot be garbage collected until the pipe ends (i.e. when either end closes or errors).

## Proposed solution
This PR fixes the memory leak by creating a single `Promise` for the return value of `pipeLoop`, and manually resolving/rejecting it.

With this fix, the leak is gone:
![diff-fixed](https://user-images.githubusercontent.com/649348/48968886-09c99800-eff6-11e8-969c-3154a970b8d7.png)

## Environment
* Windows 10 1803
* Node 10.13.0

You can view, comment on, or merge this pull request online at:

  https://github.com/whatwg/streams/pull/968

-- Commit Summary --

  * Avoid long promise chains from pipe loop in reference implementation

-- File Changes --

    M reference-implementation/lib/readable-stream.js (22)

-- Patch Links --

https://github.com/whatwg/streams/pull/968.patch
https://github.com/whatwg/streams/pull/968.diff

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/whatwg/streams/pull/968

Received on Saturday, 24 November 2018 13:42:50 UTC