Add a 'error' event to listen to?

[AxelTerizaki]

Bug Description

When node-mpv errors out by itself, an unhandled rejection in the app calling it occurs.

Here's more info on where it error'ed out from Sentry where we caught the error on one of our users : https://sentry.io/share/issue/4b6547d78cf84b34874a802ae4681207/

This error happened several times already ever since we started deploying sentry on our test versions (so some users could try the newest version of the app)

Here node-mpv errors out on isPaused() for some reason, visibly mpv is not running anymore, but we woul dlike to catch that properly and eventually restart mpv if needed.

How To Reproduce

Not sure about that

Expected behavior

Here's a proposition : emit to an "error" event so we can listen to it and catch any possible errors within our apps. Something like

const player = new mpv(...options);
player.start();
player.on('error', err => {
  // Returns an Error object or something like it.
  // Handle node-mpv errors in a better way.
});

Software Versions

• Node-Mpv: 2.0.0-beta1
• MPV: 0.29
• OS: Windows 10

Additional context

Thanks for all the good work :)

[j-holub]

Actually, there is an error event, for some reason I forgot to mention it in the documentation. However it just propagates the errors happening on stdout, I'm not sure if that's doing enough.

Currently (as in commit 55e8cd3e782c7cbff4e38d71de84834d30c4c207), the relevant code for this is in event.js line 90 and startStop.js line 189.

[AxelTerizaki]

Weird, I thought I had seen it before but couldn't find it ever again, hence why I posed the issue.

I think all logs/errors/debug should go to an event instead of console.log, so people could log it in more efficient ways.

[j-holub]

The errors for sure, the logs, I don't know. What is the advantage of putting console outputs through an error event (or any event) as opposed to console.log?

[AxelTerizaki]

Well, the app that calls node-mpv can catch them and treat them. Like, if I know it's a 8 errorcode, I can guess mpv has crashed or has been killed, and I can restart it safely.

Right now I'm able to reproduce this by launching my app, quitting mpv with the Q key, and trying to stop the player (aka stop(), not quit()) and my console gets filled with messages 👍

This error originated either by throwing inside of an async function without a c
atch block, or by rejecting a promise which was not handled with .catch(). The p
romise rejected with the reason:
{
  errcode: 8,
  verbose: 'MPV is not running',
  method: 'isPaused()',
  stackTrace: 'Error\n' +
    '    at ErrorHandler.errorMessage (C:\\dev\\karaokemugen-app\\node_modules\\
node-mpv\\lib\\error.js:79:18)\n' +
    '    at C:\\dev\\karaokemugen-app\\node_modules\\node-mpv\\lib\\ipcInterface
\\ipcInterface.js:194:37\n' +
    '    at new Promise (<anonymous>)\n' +
    '    at ipcInterface.send (C:\\dev\\karaokemugen-app\\node_modules\\node-mpv
\\lib\\ipcInterface\\ipcInterface.js:190:10)\n' +
    '    at ipcInterface.getProperty (C:\\dev\\karaokemugen-app\\node_modules\\n
ode-mpv\\lib\\ipcInterface\\ipcInterface.js:117:15)\n' +
    '    at mpv.getProperty (C:\\dev\\karaokemugen-app\\node_modules\\node-mpv\\
lib\\mpv\\_commands.js:10:22)\n' +
    '    at mpv.isPaused (C:\\dev\\karaokemugen-app\\node_modules\\node-mpv\\lib
\\mpv\\_information.js:14:15)\n' +
    '    at Timeout._onTimeout (C:\\dev\\karaokemugen-app\\node_modules\\node-mp
v\\lib\\mpv\\_startStop.js:163:30)\n' +
    '    at listOnTimeout (internal/timers.js:531:17)\n' +
    '    at processTimers (internal/timers.js:475:7)'
}
Unhandled Rejection at: {
  errcode: 8,
  verbose: 'MPV is not running',
  method: 'isPaused()',
  stackTrace: 'Error\n' +
    '    at ErrorHandler.errorMessage (C:\\dev\\karaokemugen-app\\node_modules\\
node-mpv\\lib\\error.js:79:18)\n' +
    '    at C:\\dev\\karaokemugen-app\\node_modules\\node-mpv\\lib\\ipcInterface
\\ipcInterface.js:194:37\n' +
    '    at new Promise (<anonymous>)\n' +
    '    at ipcInterface.send (C:\\dev\\karaokemugen-app\\node_modules\\node-mpv
\\lib\\ipcInterface\\ipcInterface.js:190:10)\n' +
    '    at ipcInterface.getProperty (C:\\dev\\karaokemugen-app\\node_modules\\n
ode-mpv\\lib\\ipcInterface\\ipcInterface.js:117:15)\n' +
    '    at mpv.getProperty (C:\\dev\\karaokemugen-app\\node_modules\\node-mpv\\
lib\\mpv\\_commands.js:10:22)\n' +
    '    at mpv.isPaused (C:\\dev\\karaokemugen-app\\node_modules\\node-mpv\\lib
\\mpv\\_information.js:14:15)\n' +
    '    at Timeout._onTimeout (C:\\dev\\karaokemugen-app\\node_modules\\node-mp
v\\lib\\mpv\\_startStop.js:163:30)\n' +
    '    at listOnTimeout (internal/timers.js:531:17)\n' +
    '    at processTimers (internal/timers.js:475:7)'

I use process.on('unhandledRejection') so I can catch it, but that's really a last resort unhandled rejection mechanism and I don't really wish to add node-mpv interpretations in there (it's in my index.ts file, not in mpv.ts, to give you a more precise example)

Also writing to an event would allow devs to log mpv errors to the transport of their choice, like a file for example.

EDIT: Just to be clear, the first unhandled rejection message seems to come from node-mpv, while the second one is from my app's unhandled Rejection's catcher

[AxelTerizaki]

Tried to add this to my code :

player.on('error', (err: any) => {
			logger.error(`[mpv] Error: ${err}`);
			if (err.errcode === 8) startmpv();
			sentry.error(new Error(err));
		});

But it didn't seem to catch anything for some reason.

Also edited my previous message.

[j-holub]

Yeah the first JSON object with errcode=8 is the error object I created. Feel free to share your thoughts on that one as I'm not sure if it was good idea or not.

As for your second comment, yes this error object is rejected by the methods so It should appear in the catch(err: any) block but not in the event. I thought that would be a nice way to handle errors, you know

try{
    player.doSomething();
}
catch(error) {
   // react to the error
}

Maybe it would be nice to also emit these JSON error objects over an error event, as both seems to be useful.

[AxelTerizaki]

It is usually a good idea.

The thing is I'm not entirely sure if it's my code, but isn't node-mpv sending commands by itself, like from a setInterval call or something? Because that's the kind of idea I had when I first encountered the error. My code isn't catching it, and I don't believe I forgot any try/catch anywhere.

The error object is fine with me, it's precise and helpful.

From what I understand of the stack trace I posted, there's a timeout, then isPaused is called, and then it crashes, but I don't believe I have any way to catch that in my code.

A sure way to test it is by starting mpv through node-mpv, then quitting mpv by hand (pressing Q key for example or killing its process) and waiting a few seconds.

If you can't reproduce it like that, I'll have to look closer at my code.

EDIT: Emiting both an event AND throwing could trigger errors twice, but if we have the error object in the emitted message we can probably determine if it's caused by us or not. Or the emit could be used for errors not directly resulting from node-mpv API calls.

[j-holub]

Hey again,

currently, I'm motivated to work on this module again, I'm sorry that I'm so inconsistent. I'm not using this module myself anymore, so all the time I spend on this, is my personal free time, that I'm spending for others and sometimes, between work and life I just don't feel like it. But this module is still close to my heart so I really want to make it as good as I possibly can.

I started the player and loaded a YouTube stream using Node-MPV and I just pressed q to quit the player. However, using this I cannot reproduce your issue. Here's my relevant JS code and the console output (including some event output, better see what's happening, I hope you don't mind).

const player = new mpvLib({
    "debug": true,
});

const run = async () => {
    try {
        await player.load('https://www.youtube.com/watch?v=jakpo7tj7Qw');
    }
    catch (err)  {
        console.log(err);
    }
}

player.start()
.then(() => {
	run();
})
.catch((error) => {
	console.log(error);
});

[Node-MPV]: Connected to socket '/tmp/node-mpv.sock'
[Node-MPV] sending stimulus
[Node-MPV] stimulus received true
Duration: undefined
Title: undefined
Title: watch?v=jakpo7tj7Qw
Duration: 232.321
Title: Falling In Reverse - "Popular Monster"
PlaylistSize: 4
Timepos: 0
Timepos: 0.625
Timepos: 1.666667
Timepos: 2.666667
Timepos: 3.666667
Timepos: 4.666667
Timepos: 5.666667
Timepos: 6.666667
Timepos: 7.666667
[Node-MPV]: Socket closed on the other side. This usually occurs when MPV has crashed
quitted
[Node-MPV]: MPV was quit by the user.

I'm further looking into how to create a good error event, because I don't think I really have all the relevant information at hand in the part of the code, where an error is thrown. Maybe I can somehow integrate it into the error handler itself, which would make sense. But that might require some serious refactoring to make this not ugly.

Anyway, I hope you're having a great 2021 so far, despite everything that's going on in the world right now.

[AxelTerizaki]

Hello.

I understand completely :)

For Karaoke Mugen we stopped using node-mpv completely in favor of our own implementation of the module, from scratch, to better react to any issues we might encounter while communicating with mpv, so I don't think I can help in reproducing errors or helping :/

If you want to take a look, the core of the code is there : https://lab.shelter.moe/karaokemugen/karaokemugen-app/-/blob/master/src/utils/MpvIPC.ts (it doesn't include any direct functions which sends commands, it's just the communication management between node and mpv)

[j-holub]

Hey

Sorry I let you down with all my late replies and slow working haha :D but I'm very happy that you have used my module for so long and for all the help that you guys provided. If that single file provides everything you need for your project, that's actually pretty nice, I can definitely see the node-mpv inspiration in the code :D

But out of curiosity, what features did you miss? I might like to add them just to improve the module : ) Or was it just my slow issue fixing? :D

[AxelTerizaki]

Hey :p

Don't worry, it's okay. Without node-mpv we wouldn't have started Karaoke Mugen anyway, and it was very helpful for rewriting a shorter version of it for our needs.

You're doing open source, voluntary work so we can't expect you to fix everything at once. But we had time and really wanted to work on something tighter-knitted into how KM's work, especially in the error management area. Since we just pass the commands to mpv as is it makes the code simpler, but you need to put more work than that in it to make it a module like node-mpv is.