discordjs / discord.js

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

Max CPU usage after disconnecting from voice #1000

Closed appellation closed 7 years ago

appellation commented 7 years ago

It doesn't happen all the time, but occasionally CPU usage will jump to max after calling <StreamDispatcher>.end() and <VoiceConnection>.disconnect(); it does not decline until the bot has been restarted. I experimented with having a delay between ending the dispatcher and disconnecting from the channel, but that didn't seem to have much impact; while sometimes it would last longer before the CPU spike, the CPU would still eventually spike.

This issue is only in indev (I've reverted to master without a problem), and I'm unsure about where it could be since there hasn't been a huge re-working of voice.

armando-g commented 7 years ago

I can also confirm this is a problem on at least indev. Once I migrate some things from indev to master code (such as embeds) I will test and see.

Here's some platform info:

OS: Ubuntu 16.10 ffmpeg: 3.0.5-0ubuntu0.16.10.1 node-opus: 0.2.4 discord.js: indev (latest commit as of comment)

It seems to be entirely related to connection termination though, as you mentioned. I can reproduce this often enough to see it be directly related.

EDIT: Reverting to master branch seems to have potentially eliminated the issue.

iCrawl commented 7 years ago

Can't reproduce.

OS: Ubuntu 16.04.1 LTS ffmpeg version 2.8.10-0ubuntu0.16.04.1 node-opus: 0.2.4 indev#e3921073696242ffbf44a50f7dafc12c2f362620

Might be an issue with the service's CPU power you get. Very questionable. But I also never had problems before when I was on the lowest OVH package for 3.50$

killedWithFire9001 commented 7 years ago

where i notice a spike in CPU activity on dispatcher end / leave its hardly anything i would worry about. its not sat there pinned or anything

not able to reproduce this.

OS: CentOS 7.3 ffmpeg version N-82786-gc188f35 node-opus: 0.2.4 indev

and yeh im on that OVH package @iCrawl is talking about

devsnek commented 7 years ago

unable to reproduce

OS: Debian Jessie (nightly 1:30 CST 2016-12-20) FFMPEG 3.1.1 node-opus: 0.2.4 opusscript: 0.0.1 indev#e3921073696242ffbf44a50f7dafc12c2f362620

appellation commented 7 years ago

I just upgraded my setup, so I'll do some more testing and get back.

appellation commented 7 years ago

Well, I just tested again on a better server (2 cores and 2GB of RAM) and that didn't change anything; the CPU still maxes on disconnect. It could be bad code taking advantage of a bug that shouldn't exist, in which case I guess I need to start debugging.

armando-g commented 7 years ago

@trinitynet it does sit there pinned, indefinitely until the bot has been restarted. Someone used my bot not long after I went to sleep, they soon went to sleep and the bot ended the dispatcher and stayed at 100% for the 12 hours I was not there.

Here's a video of me reproducting it:

https://gfycat.com/DownrightScratchyBanteng

This was reproduced on:

Lowest tier Vultr, single core 768MB. Lowest tier OVH SSD server, single core 2048MB. Lowest tier Digital Ocean server, single core, 512MB. Third-lowest tier Vultr server, dual core, 2048MB.

When doing the same test on the master branch, it works perfectly fine. I could let my bot go overnight and wake up in the morning with normal usage. With indev, I could walk away for an hour and come back to this, provided users are using it.

The code used for the testing is actually just Crawl's music commands ported and slightly changed for appearance (and also some other minor things, that do NOT touch dispatchers). Yet, he cannot reproduce it and I can. I have no idea what's causing this myself.

I decided to manually compile ffmpeg instead of going through the server's repo, so I can't really blame that now I don't think. The only thing I can see this being is this library or my server, which seems to be a stretch considering of how many servers I tested it on, some of which others have reported as being fine above. Though I'm not sure how long they tried to reproduce it, but it was easy enough doing it, and I can get it to show it's face within a minute.

devsnek commented 7 years ago

can you like, post the exact code you are using? kthx.

armando-g commented 7 years ago

https://github.com/WeebDev/Commando/tree/master/commands/music

Exactly as such, I was able to reproduce it with a clone of his repo.

appellation commented 7 years ago

Terminating the FFMPEG process in https://github.com/hydrabolt/discord.js/blob/indev/src/client/voice/pcm/FfmpegConverterEngine.js#L35 with SIGKILL seems to resolve this problem (as well as a secondary problem with extra FFMPEG processes cluttering memory).

This raises the question of whether this solution is indicative of a problem somewhere else. Clearly, in some cases, FFMPEG is not correctly responding to the SIGTERM signal. I have not investigated into why this might be the case, nor what the proper solution to this might be. Obviously terminating the process with SIGKILL solves the problem, but it does seem to be a bit harsh.

appellation commented 7 years ago

One thing that I have noticed with this approach is that it eventually works. The CPU will still occasionally spike to 100% but will decline to a normal level after maybe 5 or 10 seconds.

devsnek commented 7 years ago

we are rewriting a bunch of the voice stuff, specifically the part which calls ffmpeg. So expect a fix, but maybe not right away.

armando-g commented 7 years ago

That sounds fine to me, I can live with this partial patch as it's helluva lot nicer than staying at 100% until I intervene. I've recently just received an email about it too.

appellation commented 7 years ago

🎉

lmmfranco commented 7 years ago

~Still happens in version 11.0~ Edit: Sorry, mistaken this for another issue. Can't contribute to it.

appellation commented 7 years ago

I've also been seeing this on master again, but since I don't have any idea of what's causing it, I haven't brought it up again.