Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dispatcher ends without a reason #1387

Closed
1 task done
appellation opened this issue Apr 16, 2017 · 29 comments
Closed
1 task done

Dispatcher ends without a reason #1387

appellation opened this issue Apr 16, 2017 · 29 comments

Comments

@appellation
Copy link
Member

appellation commented Apr 16, 2017

Please describe the problem you are having in as much detail as possible:
When ending a dispatcher and immediately attempting to start another one, the first dispatcher ends properly, but the new dispatcher ends immediately with no reason (ie. the reason is undefined). This behaviour may be the result of my code, but regardless the dispatcher should not end with an undefined reason.

Include a reproducible code sample here, if possible:

function endListener(reason) {
  dispatcher = null;
}

// for sake of brevity, dispatcher is currently playing at this point
dispatcher.end('temp');
dispatcher = voiceConnection.playStream(stream, { volume: 0.2 });
dispatcher.once('end', endListener);

Further details:

  • discord.js version: master
  • node.js version: 7.9
  • Operating system: Ubuntu 16.10, 17.04; Windows 10 (tested but could not reproduce on Windows)
  • Priority this issue should have – please be realistic and elaborate if possible:
    Likely an edge-case.
  • I have also tested the issue on latest master, commit hash: 6bae7a1
@iCrawl iCrawl added this to the 11.1.0 milestone Apr 16, 2017
@amishshah
Copy link
Member

Probably my crappy code, I made a quick commit a few days ago to clean up internal dispatchers and streams, will take a look later today!

@iCrawl iCrawl removed this from the 11.1.0 milestone May 1, 2017
devsnek pushed a commit to devsnek/discord.js that referenced this issue May 14, 2017
devsnek pushed a commit to devsnek/discord.js that referenced this issue May 14, 2017
@appellation
Copy link
Member Author

appellation commented May 15, 2017

I'm closing this issue because it only occurs when streaming audio from ytdl-core and therefore is likely not an issue with this library.

This problem is apparently related to the stream that is being terminated: it only appears when the dispatcher that is being terminated is streaming through ytdl. I'm unsure how the previous stream could cause the new stream to immediately terminate without a reason, but that appears to be the case.

As a side note, I have seen others with this problem as well: my initial assumption that it was related to a local issue seems to be incorrect.

@iamtraction
Copy link

iamtraction commented May 17, 2017

@appellation This happens even when using youtube-dl lib.

@appellation
Copy link
Member Author

I did some testing and found that the AudioPlayer emits two error events after terminating the dispatcher, both of which are ECONNRESET errors.

@iamtraction
Copy link

iamtraction commented May 19, 2017

Did you find out what could be causing the other side to abruptly close its end of the connection?
And does this commit d85695e fix that?

@appellation
Copy link
Member Author

No I haven't made any progress on this issue.

@appellation
Copy link
Member Author

appellation commented May 25, 2017

However, I am hearing from several people that this issue is appearing with other stream types and I can confirm that that is the case. As such, this appears to be a D.js issue.

From what I can tell, this appears to be a race condition as the issue does not appear on slower systems.

@appellation appellation reopened this May 25, 2017
@kyranet
Copy link
Member

kyranet commented May 25, 2017

Can confirm. Discord.js is raceconditing. When I create a new stream instantly after closing one with Dispatcher#end, it ends the new one, causing a 'double skip'. Doesn't happen when the next stream starts a bit later (timeout of ~100ms).

@appellation
Copy link
Member Author

appellation commented May 26, 2017

Alright, time for a bit more investigation into this issue. First of all, here's a more detailed reproducible code sample.

const conn = message.guild.voiceConnection || await message.member.voiceChannel.join();
if (conn.dispatcher) conn.dispatcher.end();
const dispatcher = conn.playStream(ytdl('some yt link', { filter: 'audioonly', quality: 'lowest' }));
dispatcher.once('end', console.trace);
dispatcher.player.once('error', console.trace);

When running this code the second time, the following is output:

output

I can say that this is not the result of a ytdl error, as the exact same output is produced when replacing line 3 of the above code with the following: const dispatcher = conn.playStream(request('some audio stream'));

Note: does not appear to happen with playFile.

This, as of 4292134 running in a Ubuntu 16.04 Docker container.

@ibexes
Copy link

ibexes commented Jun 14, 2017

I thought I was going crazy until I saw this thread. This happens to me every other 'skip', aka, when I call the dispatcher end method, the end event fires twice - two completely different events. E.g.

been fired: user skipped
been fired: undefined <-- skipped
been fired: user skipped <--fine here
been fired: user skipped
been fired: undefined <--another skip, etc
been fired: user skipped
been fired: user skipped
been fired: undefined

Can provide a condensed snippet of code upon request, but enough has been previously given already. Not sure if it is race conditioning, since it occurs every other skip (edit: I'd like to add that this has consistently occurred for more than 100 attempts - yes, I should've given up sooner). Could it be some missed case during error handling/not clearing properly?

Not sure how relevant it is, but when I force skip, the next song that plays seems to take far longer than usual, I'm afraid I don't know D.js well enough to speculate on that.

Further details:
-discord.js version: master
-node.js version: 7.7.4
-Operating system: Ubuntu 16.10, 17.04; OSx Sierra
-Using it with ytdl (I'm 99% sure this is not the issue)

@telunc
Copy link

telunc commented Jun 15, 2017

The issue is with destroy function in src/client/voice/dispatcher/StreamDispatcher. The structure of AudioPlayer.js is passing itself with the same properties. Here is the line const dispatcher = new StreamDispatcher(this, stream, options);. When a new dispatcher was immediately added after the previous dispatcher, the emitter in the destroy function hasn't completed. Once the emitter finished emitting, it caused the re-initialized dispatcher to end instead. Hence, a race condition occurred.

Stream Dispatcher

destroy(type, reason) {
    if (this.destroyed) return;
    this.destroyed = true;
    this.setSpeaking(false);
    this.emit(type, reason);
    /**
     * Emitted once the dispatcher ends.
     * @param {string} [reason] the reason the dispatcher ended
     * @event StreamDispatcher#end
     */
    if (type !== 'end') this.emit('end', `destroyed due to ${type} - ${reason}`);
}

Ratismal pushed a commit to Ratismal/discord.js that referenced this issue Jul 5, 2017
@alexisvisco
Copy link

alexisvisco commented Aug 18, 2017

Why when adding a delay to play another stream it doesn't work :

Skip:

function playerSkip(message: Message) {
    const dontPlayMusic = ":x: **Le bot ne joue actuellement pas de musique.**";

    const player = Player.getInstance();
    if (!player.isConnected)
        message.channel.send(dontPlayMusic);
    else {
        player.dispatcher.end("skip")
    }

}

PlayCore:

async function playCore(message: Message, playlist: Playlist) {
    const musicPlaying = (m) => ":musical_note: Now playing :  `" + m.name + " - [" + m.length + "s]`  :musical_note:";

    const player: Player = Player.getInstance();

    player.playlist = playlist;
    player.voiceChan = message.member.voiceChannel;
    player.connection = message.guild.voiceConnection || await message.member.voiceChannel.join();

    const music: Music = player.nextMusic();
    message.channel.sendMessage(musicPlaying(music));
    player.dispatcher = player.connection.playStream(YTDL("http://www.youtube.com/v/" + music.id, {filter: "audioonly"}));

    player.dispatcher.once("end", reason => {
        console.log("end ?? " + music.name + " reason " + reason);
        if (player.isConnected)
            setTimeout(() => playCore(message, player.playlist), 500);
        else
            player.disconnect();
    });
    player.dispatcher.on('error', e => {
        console.log(e)
    });
    player.dispatcher.player.on('warn', console.warn);
    player.dispatcher.on('warn', console.warn)
}

Once I could skip and hear each time the music came in. But now it does not work, the music does not play and there is always this error: prism transcoder error - Error: read ECONNRESET

After some moment, the dispatcher end with this reason : reason Stream is not generating quickly enough.

Anyone has a solution ?

@appellation
Copy link
Member Author

The core of this issue is over 2 months old and seems to have vanished.

@DeltaEvo
Copy link

DeltaEvo commented Sep 4, 2017

This issue have not vanished,

The bug is currently upstream in https://github.com/hydrabolt/prism-media, because after kill it send error for stdin read fail and this cause the dispatcher's end https://github.com/hydrabolt/discord.js/blob/master/src/client/voice/player/AudioPlayer.js#L110

@MarleyPlant
Copy link

I am also still having this issue at 11.2.1 via npm

Running dispatcher.end() will end the current dispatcher and then the next one created after that.

@DeltaEvo
Copy link

@MarleyPlant prism-media has not been yet upgraded on npm, can you try to use the version on github ?

   npm install --save hydrabolt/prism-media

@MarleyPlant
Copy link

I'm using discord.js and yt-dl.

is prism-media a requirement for discord.js or am I missing something?

@DeltaEvo
Copy link

yes, prism-media is a dependency of discord.js

@Mauyeung
Copy link

Might be a bit late, but trying the upgraded version of prism-media fixed the issue for me.

@vmarchesin
Copy link

vmarchesin commented Oct 24, 2017

As of this date the bug is still happening. This is indeed a race condition, and can be circumvented by using a delay before your next call.

  dispatcher.on('end', () => {
    setTimeout(() => yourcode, 200)
  })
  

@DeltaEvo
Copy link

DeltaEvo commented Oct 24, 2017

@vmarchesin can you try to do a npm update on your project and check if the bug is still happening ?

@iamtraction
Copy link

It's been months and I don't see this bug being fixed. I'm still getting this issue.

@amishshah
Copy link
Member

There's a brand new voice rewrite, if you want to test it out early you can test the voice-rewrite branch

@appellation
Copy link
Member Author

I have not encountered this bug since like June. Please ensure that you've updated to the latest release of Discord.js. Also, as hydra mentioned, voice is getting rewritten for v12.

@vmarchesin
Copy link

I tried with the latest version of all dependencies, and I still get the issue. I'm running off an AWS EC2 machine, which is quite fast, so the race condition is likely to appear. As I said adding a delay solves the issue. For now it's easier to have this workaround than to wait for a fix. I did not try the voice-rewrite branch but since everything is working for me now I won't use it until the next stable release.

@iamtraction
Copy link

Adding a delay is just a workaround, not a fix.
And voice-rewrite branch fixes the issue. Thanks hydrabolt. But since I can't use that on production, will have to wait until v12 is released.

@appellation appellation reopened this Nov 4, 2017
@Sergey-bhw
Copy link

I also noticed that this bug happened to me when I restart the bot without it fully go offline. Even with this delay walkaround. Maybe that will help someone.

@amishshah
Copy link
Member

voice-rewrite has landed on master for those looking for a fix

@amishshah
Copy link
Member

This is actually removed behaviour in master now, if no error event is emitted you can assume the stream has ended gracefully.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests