discordjs / discord.js

A powerful JavaScript library for interacting with the Discord API
https://discord.js.org
Apache License 2.0
25.36k stars 3.97k forks source link

Voice Leaks #2951

Closed Deivu closed 5 years ago

Deivu commented 5 years ago

Please describe the problem you are having in as much detail as possible: I Opened this same issue in past, but that time I don't have too much evidence about this leak. From that moment, I tried to debug this via dumping heapdumps and tracking it inside d.js voice code & my code and it seems like it was really leaking. Previous Heapdumps shows that "FFmpegTransform", "Encoder" & "VolumeTransformer16LE" arent getting gced and just piles up overtime, IF the dispatcher ended prematurely (when the writer stream ended without even draining the data from the readable stream). So what I did is I tried various libs from npm, and it seemed to work fine, so I tested it again with prism-media, but this time, what I did is to make sure to that all the streams are getting closed and It seems this fixes this issue.

Old Memory Usage on 20k servers 58H of uptime. https://cdn.discordapp.com/attachments/450644221410410506/490741735501201418/Untitled.png

New Memory Usage on 28k servers 43H of uptime. https://cdn.discordapp.com/attachments/450644221410410506/512826205079339010/unknown.png

Further details:

amishshah commented 5 years ago

Hi sorry for such a late reply, can you confirm whether or not you can still reproduce this issue?

amishshah commented 5 years ago

Can you also clarify as to how you're making sure all the streams are being closed?

Deivu commented 5 years ago

Yes you could reproduce this issue if you use dispatcher.end() or dispatcher.destroy() to end the stream that is not yet done.

For closing basically, the streams that prism media creates, I try to close them manually instead of leaving them as is. Although this did not solve this 100%, it migrated it to a great degree.

amishshah commented 5 years ago

Recently encountered this and just made some commits that I think fix the issue, can you try them out and tell me if it fixes the issue?

amishshah commented 5 years ago

Note you'll also need to update your prism install

Deivu commented 5 years ago

Thanks for the response, I'll try this as soon as I was able to do so

Deivu commented 5 years ago

Ok Update about this, the commit did make the music more stable, but the issue when you are ending the streams early via dispatcher.end() makes the memory creep up a bit over time is still noticable

Code I used in testing this

    const stream = await ytdl(serverQueue.songs[0].url)
    const dispatcher = connection.play(stream, { type: 'opus', passes: 2, bitrate: 128 })
        .on('error', onError)
        .once('finish', async () => {
            dispatcher.removeListener('error', onError);
            serverQueue.songs.shift();
            await play(client, guild, serverQueue.songs[0]);
        });
    dispatcher.setVolumeLogarithmic(serverQueue.volume / 7.5);

usage

amishshah commented 5 years ago

Could you try dispatcher.destroy() instead of end ()?

Deivu commented 5 years ago

destroy doesn't emit finish when I used it causing the music to be stucked, That's why I use dispatcher.end()

amishshah commented 5 years ago

That's really strange I'll have a look in the next hour or two

amishshah commented 5 years ago

@Deivu I think I've managed to fix it in https://github.com/discordjs/discord.js/commit/6aa792f9ab8008c22397d1a46a862fa88ba5aef5 could you give it a try? This means that the dispatcher will be cleaned up properly when using end()

Deivu commented 5 years ago

@amishshah Thanks for that commit, I do think right now the memory is much more stable than before. I will observe this for a few days and close it finally when I do see if it works as intended

As for test session, I limited my max old space size to 128mb to force the it to gc and see if there's still unusual behaviors in regard to this.

amishshah commented 5 years ago

That's great news so far! Good luck with your testing, let's hope this has finally fixed it.

Deivu commented 5 years ago

Closing this issue, I feel like its now the cache what makes the memory rise up, and for normal personal usage its fine.

amishshah commented 5 years ago

That's great news! Thanks for being so helpful and patient with the testing 🎉