Possible Memory Leak in StreamDispatcher

[Deivu]

Please describe the problem you are having in as much detail as possible: I'm thinking that there maybe a leak on Discord's.js voice in Master. In 50 Hours of Uptime, My bot is now eating 12gb of ram, and it continues to rise as new dispatchers are being created.

I am fully aware that Music Bots tend to eat ram, but isn't this quite overboard?

Ram Usage Below over 50 hours of uptime. https://cdn.discordapp.com/attachments/450644221410410506/490741735501201418/Untitled.png

Include a reproducible code sample here, if possible:

class KashimaPlayer {
    constructor(client, guildID) {
        this.client = client;
        this.guildID = guildID;
        this.queue = this.client.queue.get(this.guildID);
        this.guildObj = this.client.guilds.get(this.guildID);
        this.voiceConnection = this.client.voiceConnections.get(this.guildID);
        this.textChannel = this.guildObj.channels.get(this.queue.textChannel);
        this.stream;
        this.dispatcher;
    };

    async start() {
        if (!this.queue) {
            this.voiceConnection.disconnect();
            return;
        } else if (!this.queue.songs.length) {
               try {
                await this.textChannel.send('Stream Ended');
            } catch (error) {
                this.client.cannons.fire(this.client, error);
            } finally {
                this.client.queue.delete(this.guildID);
                    this.voiceConnection.disconnect();
                return;
                };
        };
        let info;
        let error;
        let songlength;
        try {
            info = await this.client.ytdl.getBasicInfo(this.queue.songs[0].url);
            songlength = parseInt(info.length_seconds);
            if (songlength !== 0 && songlength <= 1800) {
                this.stream = this.client.ytdl(this.queue.songs[0].url, { quality: 'highestaudio' });
                this.dispatcher = this.voiceConnection.play(this.stream, { passes: 3 });
                } else {
                            this.queue.songs.shift();
                            await this.start();
                            await this.textChannel.send('\:negative_squared_cross_mark: Teitoku, This Video is a **livestream or longer than 30 minutes**. Hence I **cannot play it**.');
                return;
            };
        } catch (error) {
            this.queue.songs.shift();
               // this.client.cannons.fire(this.client, error); this line makes the logger go wild. Lmao.
                await this.start();
            await this.textChannel.send(`\:warning: Teitoku-san, this **song is unplayable**. Because\`\`\`${error}\`\`\``);
            return;
        };
        this.stream.on('error', (err) => {
                error = 1;
            this.dispatcher.end();
            this.textChannel.send(`\:negative_squared_cross_mark: Teitoku-san, **I encountered an error** while playing the song. Here is the Report.\`\`\`${err}\`\`\``)
            .catch(e => this.client.cannons.fire(this.client, e));
        });
        this.dispatcher.on('start', async () => {
            try {
                await this.textChannel.send({
                    embed: {
                        color: 0xc0c0c0,
                        thumbnail: {
                            url: this.queue.songs[0].thumbnail
                        },
                        fields: [{
                            name: "\:musical_note: Now Playing",
                            value: `**${await this.parse(`${songlength || 0}`)}** | [${this.queue.songs[0].title}](${this.queue.songs[0].url})`
                           }, {
                            name: "\:inbox_tray: Queued by",
                            value: `**@${(await this.client.users.fetch(this.queue.songs[0].memberId)).tag}**`
                           }
                        ],
                        timestamp: new Date(),
                        footer: {
                            icon_url: this.guildObj.iconURL({format: 'png', size: 512}) || 'https://i.imgur.com/CjcQBE6.jpg',
                            text: this.guildObj.name
                        }
                    }
                });
            } catch (error) {
                this.client.cannons.fire(this.client, error);
            };
        });
        this.dispatcher.on('finish', async () => {
            try {
                        this.queue.songs.shift();
                await this.destroy();
            } catch (err) {
                this.client.cannons.fire(this.client, err);
            } finally {
                try {
                    await this.start();
                } catch (err) {
                    this.client.cannons.fire(this.client, err);
                };
            };
        });
        this.dispatcher.on('error', (error) => {
            if (!error) {
                this.dispatcher.end();
                this.textChannel.send(`\:negative_squared_cross_mark: Teitoku-san, **I encountered an error** while playing the song. Here is the Report.\`\`\`${error}\`\`\``) 
                .catch(e => this.client.cannons.fire(this.client, e));
            };
        });
    };

    async destroy() {
        this.dispatcher.removeAllListeners();
        this.stream.removeAllListeners();
        this.dispatcher.destroy();
        this.stream.destroy();
    };

    async parse(seconds) {
        return ( seconds - (seconds %= 60) ) / 60 + (9 < seconds ? ':' : ':0') + seconds
    };
};

Further details:

• discord.js version: Discord.js Master

• Node.js version: 10.9.0

• Operating system: Ubuntu 16.04 LTS

• Priority this issue should have – please be realistic and elaborate if possible: Medium, Due to the fact that it will keep on increasing the memory usage over time.

• [ ] I found this issue while running code on a user account

• [x] I have also tested the issue on latest master, commit hash:c63f15c8bc98145c3e04955229648d69dbdf1678

[amishshah]

If you're removing all listeners you're removing the listeners that Discord.js uses internally to properly destroy dispatchers and connections, so I'd recommend destroying your own listeners specifically with removeListener. Try that and see if it works. I've not encountered any sort of memory leaks with voice myself so hopefully your issue is just because of the listener removal.

[Deivu]

Ok, I updated my code with

    async destroy() {
        this.dispatcher.destroy();
        this.stream.destroy();
        this.dispatcher.removeListener('start');
        this.dispatcher.removeListener('finish');
        this.dispatcher.removeListener('error');
        this.stream.removeListener('error');
    };

in my destroy method. I will be back with feedback soon after testing.

Edit 1 I choose to do this over the removeListener because this does not require a callback, which what I want. Since if there is a supplied event name on the removeAllListeners([eventname]) it will just remove that listener.

    async destroy() {
        this.dispatcher.destroy();
        this.stream.destroy();
        this.dispatcher.removeAllListeners('start');
        this.dispatcher.removeAllListeners('finish');
        this.dispatcher.removeAllListeners('error');
        this.stream.removeAllListeners('error');
    };

[appellation]

this does not actually prevent the removal of internal listeners; you need to specify a specific listener to remove by providing the callback to the remove listener method.

[Deivu]

Ok, will do. maybe I'll just do a log or blank function as a callback.

[1Computer1]

What he meant was that the function provided to .on must be provided to .removeListener (not just the same code but the actual same function) e.g.

const cb = () => console.log('event happened');
ee.on('event', cb);
ee.removeListener('event', cb);

[Deivu]

I see, thanks for the clarification. Will put this into test immediately.

[Deivu]

with 60 hours of uptime its pretty much more or less the same https://cdn.discordapp.com/attachments/450644221410410506/492168131419045899/Untitled.png 11gb Usage

This is the code structure we did we used bind because this will be different once it is called on error because it isn't an arrow function.

this.errorbind = this.error.bind(this);
this.dispatcher.on('error', this.errorbind);
this.dispatcher.removeListener('error', this.error);
async error(error) {
    this.dispatcher.end();
    await this.textChannel.send(`\:negative_squared_cross_mark: Teitoku-san, **I encountered an error** while playing the song. Here is the Report.\`\`\`${error}\`\`\``);
};

Or should we really use arrow function here

[paulstelian97]

It may be the bound functions surviving and each using up RAM. That isn't exactly NodeJS specific. Every bound function is separate even if it binds the same this to the same function.

[Deivu]

        this.error = async (error) => {
            this.dispatcher.end();
            await this.textChannel.send(`\:negative_squared_cross_mark: Teitoku-san, **I encountered an error** while playing the song. Here is the Report.\`\`\`${error}\`\`\``);
        };
                this.dispatcher.once('error', this.error);
                this.dispatcher.removeListener('error', this.error);

I updated it with this, and hope this would fix it.

[paulstelian97]

Uhm, once removes the listener immediately after triggering it once. Only listeners added with on need to be removed manually.

[Deivu]

what if it never fires the error event, then redo the whole function, it will not remove the once error do it?

[paulstelian97]

Well, you overwrote the this.error instance which was registered anyway so... Simply, that extra code is worth nothing.

The remove will only work for listeners added with on. once is really on with two handlers: the one passed to once and the one removing that and itself.

[Deivu]

so what you mean is pretty much .once solves my problem?

[paulstelian97]

Maybe. But definitely the remove doesn't have its place there. A memory leak may still be there, removing that remove call makes it easier to debug as it's a fake fix.

I'm not sure what the real one is, or even whether it's from there.

[paulstelian97]

Using promises may be a better idea. Promises automatically handle running a code only once. You would do then on error, run the actual reject() of the promise. First time, promise rejected. Second time if needed, ignored.

Using promises and async functions I think generally is a good idea, for any event that you only want to handle once from a source.

[Deivu]

Promises with event listeners?

[Deivu]

const promise = new Promise(function(resolve, reject) {
  dispatcher.once('finish', function() {
     return resolve('done');
  });
  dispatcher.once('error', function(err) {
    return reject(err);
  });
});

Perhaps something like this?

[paulstelian97]

You can definitely use the resolve or reject method of a promise to work with events which needed to be handled once.

On Thu, Sep 20, 2018, 18:23 Deivu (Saya) notifications@github.com wrote:

Promises with event listeners?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/discordjs/discord.js/issues/2838#issuecomment-423224197, or mute the thread https://github.com/notifications/unsubscribe-auth/AJvL9UMhWSznAYpftcuDoIvVB4WzsUlOks5uc7LygaJpZM4WqrJa .

[Deivu]

So the example I posted above could potentially fix this leak due to this listeners?

[paulstelian97]

Pretty much. Just instead of 'done' either don't provide anything (leave it as undefined) or provide an actual result.

On Thu, Sep 20, 2018, 18:27 Deivu (Saya) notifications@github.com wrote:

const promise = new Promise(function(resolve, reject) { dispatcher.once('finish', function() { return resolve('done'); }); dispatcher.once('error', function(err) { return reject(err); }); });

Perhaps something like this?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/discordjs/discord.js/issues/2838#issuecomment-423225484, or mute the thread https://github.com/notifications/unsubscribe-auth/AJvL9T62ybDnmj2zF7hGLjoIyyvm9Dxtks5uc7PpgaJpZM4WqrJa .

[bdistin]

Hi, sorry to interject, but it appears this issue has devolved into support chat. There exists a lovely platform for support chat here: https://discord.gg/bRCvFy9 It would mean a lot to everyone who subscribes to this repo to not get every message you are exchanging here as an email. This space is reserved for issues and feature requests, not questions or support chat.

[Deivu]

Well this could be an issue or not, but either ways, I'll leave it open for 3 days again once I finished applying the fix and compare the memory usage before and after before commenting again. Thanks for tip.

[Deivu]

I just decided to go with Lavalink in my rewrite. It's still leaking but not as much as it was before. Thanks for those who participated and gave me tips in fixing this issue.