- 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. ![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