From ebada93f0d2b2ec3a158dae2ba43e98d81f3182a Mon Sep 17 00:00:00 2001 From: Liam Date: Sun, 16 Jul 2017 09:16:31 -0400 Subject: Fix potential memory leak from DownloadController.once(canceled) assigned on every .download() --- src/http-music.js | 4 ++++ src/loop-play.js | 25 ++++++++++++++++--------- todo.txt | 6 +++--- 3 files changed, 23 insertions(+), 12 deletions(-) diff --git a/src/http-music.js b/src/http-music.js index 3f1c901..12c93e7 100755 --- a/src/http-music.js +++ b/src/http-music.js @@ -33,6 +33,10 @@ function downloadPlaylistFromOptionValue(arg) { } } +// Let this forever be of use to people who run into +// maxlistenersexceededwarning. +process.on('warning', e => console.warn(e.stack)) + Promise.resolve() .then(async () => { let sourcePlaylist = null diff --git a/src/loop-play.js b/src/loop-play.js index 701e590..a38e524 100644 --- a/src/loop-play.js +++ b/src/loop-play.js @@ -26,10 +26,16 @@ class DownloadController extends EventEmitter { // be canceled and replaced with a new download (see cancel) // which would void the result of the old download.) + this.cleanupListeners() + let canceled = false - this.once('canceled', () => { + + this._handleCanceled = () => { canceled = true - }) + this.cleanupListeners() + } + + this.once('canceled', this._handleCanceled) const file = await downloader(arg) @@ -38,12 +44,19 @@ class DownloadController extends EventEmitter { } } + cleanupListeners() { + if (this._handleCanceled) { + this.removeListener('canceled', this._handleCanceled) + } + } + cancel() { // Cancels the current download. This doesn't cancel any // waitForDownload promises, though -- you'll need to start // a new download to resolve those. this.emit('canceled') + this.cleanupListeners() } } @@ -128,7 +141,7 @@ class PlayController { file ]) - const handleData = data => { + this.process.stderr.on('data', data => { const match = data.toString().match( /(..):(..):(..) \/ (..):(..):(..) \(([0-9]+)%\)/ ) @@ -163,12 +176,6 @@ class PlayController { `\x1b[K~ (${percentStr}%) ${curStr} / ${lenStr}\r` ) } - } - - this.process.stderr.on('data', handleData) - - this.process.once('exit', () => { - this.process.stderr.removeListener('data', handleData) }) return new Promise(resolve => { diff --git a/todo.txt b/todo.txt index 6ad8e9f..6115430 100644 --- a/todo.txt +++ b/todo.txt @@ -173,10 +173,10 @@ TODO: Figure out the horrible, evil cause of the max listeners warning I'm getting lately.. current repro: play a bunch of files locally. Skipping tracks still leads to issue. Wait for them to start playing before skipping, though. + (Done!) -TODO: Something's requesting avprobe, and I'm not sure why. - (While playing flora.json/YouTube videos.) Perhaps - it's youtube-dl..? +TODO: Something's requesting avprobe, and I'm not sure why. (While playing + flora.json/YouTube videos.) Perhaps it's youtube-dl..? TODO: Re-implement skip. (Done!) -- cgit 1.3.0-6-gf8a5