search

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

Possible memory leak in StreamDispatcher #4287

Closed seuuuul closed 3 years ago

seuuuul commented 4 years ago

Please describe the problem you are having in as much detail as possible: The bot is continuously (every X seconds) streaming a different Youtube video with a random seek to a voice channel. The memory (heap size) keeps increasing as time goes on.

I don't understand much about memory/heap, but these are some heap snapshots I made with the node inspector, the last snapshot is 30 minutes in. The actual memory on my pc is larger when you count all the node processes.

What I think keeps allocating high 'retained size' are ArrayBuffer and Buffer, a lot of them with references to StreamDispatcher.js, VolumeTransformer.js, _stream_readable.js etc.

This is the 'size delta' on another run I did:

I think it has to do with me ending the dispatcher before the stream is finished and/or me seeking to a certain part of the video and then ending the dispatcher. I also tried this however without the seek option, and it shows the same result. Hopefully someone can point out something I'm missing.

Include a reproducible code sample here, if possible: I made a simple re-producible sample which I run in a single guild with 1 member. Say "!start" once you are in a voice channel. https://github.com/seuuuul/example

import { Client, Message, VoiceChannel } from "discord.js";
import ytdl from "ytdl-core";

const TOKEN = "token";
const YOUTUBE_LINKS = [
  "https://youtu.be/FzVR_fymZw4",
  "https://youtu.be/9pdj4iJD08s",
];

const client = new Client();

client.on("message", async (message) => {
  if (message.content === "!start") {
    const voiceChannel = message.member!.voice.channel!;
    while (true) {
      await procedure(voiceChannel);
    }
  }
});

const procedure = async (voiceChannel: VoiceChannel) => {
  console.log("New procedure.");

  // Set up stream/dispatcher and play at a random time.
  let voiceConnection = await voiceChannel.join();
  let stream = ytdl(YOUTUBE_LINKS[Math.round(Math.random())]);
  let dispatcher = voiceConnection.play(stream, {
    seek: Math.floor(Math.random() * 100),
  });

  // Let it play for 10 seconds.
  await new Promise((resolve) => {
    setTimeout(resolve, 10_000);
  });

  dispatcher.end();

  return;
};

(async () => {
  await client.login(TOKEN);
})();

Things I tried (all same issue):

let stream = ytdl(YOUTUBE_LINKS[Math.round(Math.random())], {
    highWaterMark: 1 << 25,
  });

Same as above.

let stream = ytdl(YOUTUBE_LINKS[Math.round(Math.random())], {
    filter: "audioonly"
  });

I don't use this option because streaming with "audioonly" gives a long delay when starting the audio, probably due to seeking to a timestamp. This does not happen when streaming both video and audio. However, they all have the same issue in the end.

Further details:

Some related issues are #2951. I asked this question on the discord and another user had the same problem.

GoldenAngel2 commented 4 years ago

Wouldn't this

while (true) {
      await procedure(voiceChannel);
    }

Call that function loads and loads of times, which could raise the memory.. (could be wrong)

Clemens-E commented 4 years ago

It would call the method endlessly, but it should only run after the procedure before it was ended, so this should not have a indefinitely increasing memory usage

seuuuul commented 4 years ago

It would call the method endlessly, but it should only run after the procedure before it was ended, so this should not have a indefinitely increasing memory usage

Yeah I am pretty confident this is not a problem because of this @ the message above you

ColinCee commented 4 years ago

I'm using typescript and having the same issue:

  async playSound(voiceConnection: VoiceConnection): Promise<void> {
    const file = getSoundPath(this.message.content);
    const event = voiceConnection.play(file);

    event.on("finish", () => {
      event.destroy();
    });
  }

After playing sound for 9 times this happens:

ts-node-dev detects memory leaks

Using ts-node version 8.10.1, typescript version 3.9.2
(node:58335) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 unpipe listeners added to [DailyRotateFile]. Use emitter.setMaxListeners() to increase limit
(node:58335) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 error listeners added to [DailyRotateFile]. Use emitter.setMaxListeners() to increase limit
(node:58335) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 drain listeners added to [DailyRotateFile]. Use emitter.setMaxListeners() to increase limit
(node:58335) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 close listeners added to [DailyRotateFile]. Use emitter.setMaxListeners() to increase limit
(node:58335) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 finish listeners added to [DailyRotateFile]. Use emitter.setMaxListeners() to increase limit
Clemens-E commented 4 years ago

That's not ts-node detecting anything, "detecting" possible memory leaks is normal nodejs behavior. I'm going to assume that you simply keep playing from the same stream and that's causing the issue. Your finish event doesn't make sense either, because why would you destroy an already finished stream

Clemens-E commented 4 years ago

After letting the example run for 16.7 minutes, I was not able to see a major increase of overall allocated objects, logging the used heap speaks for this too: I started logging the used heap, after it stabilized at the beginning, it was around 8MB, after the first procedure it was at around 9MB, after playing for 12 Minutes I was at 13MB, which doesn't sound too bad. Can anyone else reproduce this? Preferable using the "Allocation instrumentation on timeline" snapshots snapshot1

seuuuul commented 4 years ago

Hi @Clemens-E thanks for trying it out. I reproduced your results with the "Allocation instrumentation on timeline" option on and got the same results with very few blue lines which is good I think.

50m run Before (task manager): 167mb After (task manager): 515mb image

50m run (no node debug on) + the song plays every 5s instead of 10s After (task manager): 900mb image

15 min run with simple bot (no node debug on) I also did the same things on this very simple message on repeat bot:

Code ```typescript import {Client, TextChannel} from "discord.js"; const TOKEN = "token here"; const client = new Client(); client.on("message", async (message) => { if (message.content === "!start") { const textChannel = message.channel as TextChannel; while (true) { await procedure(textChannel); } } }); const procedure = async (textChannel: TextChannel) => { console.log("New procedure."); await textChannel.send("This is a message."); // Every 5 seconds. await new Promise((resolve) => { setTimeout(resolve, 5_000); }); return; }; (async () => { await client.login(TOKEN); })(); ```

Before: ~93mb After: ~93mb

So in the allocation timeline there doesn't seem to be a problem, but once I for example up the number of streams a bit the actual memory I see being used does keep increasing. I am not sure anymore what the cause can be, maybe it is something that is expected from node rather? Still then I would expect it to flatten out at some point or at least not increase up to 1gb.

Brainicism commented 4 years ago

I have also run across the same issue on my bot with a similar use case as well, but I'm streaming local files instead of using ytdl to create streams. Performing a heap snapshot at the beginning of execution and comparing it with the snapshot of when memory leakage has occurred shows very similar results. A massive +delta of JSArrayBufferData allocated, each of which have references to opus.js and StreamDispatcher.js.

Heap diff

I have also looked at the allocation timeline, but the problem doesn't seem to show itself. Diffing two snapshots appears to be the only way to see it.

I also managed to reproduce the issue with the code @seuuuul provided. The memory usage steadily increases as each song is streamed. The sample code started at 97MB usage, and steadily rose to 214MB after five minutes, with the same +delta of JSArrayBufferData being allocated.

seuuuul commented 4 years ago

@Brainicism Thanks for trying it out as well, I hope someone can take a deeper look into it, because there are some other people on the discord as well who are also dealing with it :/.

What things are you doing now to work with it?

Brainicism commented 4 years ago

I took a deeper look into it and plotted some charts printing out the data from process.memoryUsage().

On the latest version of djs (12.2.0), we can see that the external memory as well as the rss increase as time goes on. The node docs define external memory as "the memory usage of C++ objects bound to JavaScript objects managed by V8." This makes me believe that the issue involves the Opus bindings that StreamDispatcher is using. I have also tested and reproduced on 12.1.0 and 12.0.0, as well as on Windows and Ubuntu.

On an older version of djs (11.6.4), the external memory allocated is negligible image

I've just been restarting the process every time I ran into ENOMEM exceptions as my "workaround," which has been pretty bad considering it begins to eat up all my memory within a few hours. I'm thinking of reverting to 11.6.4 for the time being.

I modified your sample code a bit to get rid of the dependence on youtube streams, as well as writing out memory values to a file here if anyone else wants to take a look: https://gist.github.com/Brainicism/b7e06bf92d51293941a4cb29ec31e8f6

EDIT: I have reverted my bot to 11.6.4, and the issue has been resolved.

seuuuul commented 4 years ago

@Brainicism Appreciate the tests again, actually it is good news it does work on 11.6.4. I might try and revert back as well if it is not too much work. I also might take a quick look at some diffs between the two versions in those particular files, depends on what will be less work. Will post here if I find anything.

Roki100 commented 4 years ago

I'm having the same issue for pretty long time now, and what i also noticed is memleak in tcp/socket memory

Brainicism commented 4 years ago

Has anyone found a workaround for this other than downgrading at v11? There's a ton of other voice related bugs I've been running into in v11 that have been fixed in v12.

Roki100 commented 4 years ago

40874-putty_2020-09-28_21-12-35 i think this should be fixed asap because this memleak is so huge

karelkryda commented 4 years ago

Hi, I have a simple code that connects my bot to a voice channel when I write a message.

  console.log("Ready!");

  client.user.setPresence({
    status: "online",
    activity: {
      name: "/help",
      type: "LISTENING"
    }
  });
});

client.on('message', async message => {
    // Join the same voice channel of the author of the message
    if (message.member.voice.channel) {
        const connection = await message.member.voice.channel.join();
    }
});

// Start bot
client.login(bootconfig.token).catch(error => console.log("Error: " + error));

If I move the bot four times manually between voice channels, I get this error. unknown

Is this the same issue you are discussing here? Thank you

emelin2004 commented 3 years ago

Will this be fixed soon? I'm experiencing very big memory usage which increases by time when using streamDispatcher.

wolf-yuan-6115 commented 3 years ago

I hope so too When I was using my own bot to listening music, I heard some weird sounds and my bot goes offline after that sound ended.

InfiniteMarcus commented 3 years ago

Hi, I have a simple code that connects my bot to a voice channel when I write a message.

  console.log("Ready!");

  client.user.setPresence({
    status: "online",
    activity: {
      name: "/help",
      type: "LISTENING"
    }
  });
});

client.on('message', async message => {
  // Join the same voice channel of the author of the message
  if (message.member.voice.channel) {
      const connection = await message.member.voice.channel.join();
  }
});

// Start bot
client.login(bootconfig.token).catch(error => console.log("Error: " + error));

If I move the bot four times manually between voice channels, I get this error. unknown

Is this the same issue you are discussing here? Thank you

@karelkryda did you managed to solve your memory leak? I also have a MaxListenersExceeded when I manually move my bot or use a join command like channel.join() four times between voice channels.

karelkryda commented 3 years ago

@Infinitemarcus no, I start using Lavalink. Btw yes. I had also MaxListeners errors when moving bot manually between channels.

amishshah commented 3 years ago

Hi there,

We're working on a new implementation of Discord's Voice API that has better playback quality and is more reliable than what we currently support in Discord.js v12 - check it out at https://github.com/discordjs/voice!

The new library solves many of the issues that users are facing, and as part of this, we're dropping built-in support for voice in our next major release. We have a PR (https://github.com/discordjs/discord.js/pull/5402) that adds native support for our new voice library - once this PR is merged, this issue will be closed.

You can still use our new voice library before that PR lands - just take a look at our music bot example to see how to get started upgrading your voice code. By using the boilerplate music player in the example, you can make it even easier to upgrade your code.

Note that the PR above only reduces some of the boilerplate code you'd otherwise have to write - you do not have to wait for the PR to be merged to start using the new voice library.

If you have any questions about this, feel free to: