- From: Mattias Buelens <notifications@github.com>
- Date: Sat, 24 Nov 2018 05:42:29 -0800
- To: whatwg/streams <streams@noreply.github.com>
- Cc: Subscribed <subscribed@noreply.github.com>
- Message-ID: <whatwg/streams/pull/968@github.com>
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.

## 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:

## 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