UnhandledPromiseRejectWarning

[sseiber]

@jishi I found this unhandled exception today (I'm running on node 7.x)

(node:53725) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 7): Error: Got status 500 when invoking /MediaRenderer/AVTransport/Control
(node:53725) DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

File: request.js
Line: 43

        res.on('end', () => {
          error.body = buffer.join('');
          reject(error);
        });

To repro use the node-sonos-http-api linein command on a player with no linein configured.

The exception is on line 43 when the reject is called. I would expect the exception to be caught, but it must be getting swallowed by another Promise somewhere.

[jishi]

Thanks for noticing. Wasn't aware of that pending change, and would indeed cause a crash in later versions.

I think I log all unhandledRejections, but since they doesn't cause any real side effects most of the time, it was safe to let them go un-noticed.

I will go through the code base and make sure that they are handled correctly.

[sseiber]

@jishi Actually looking more into this I think I needed to catch the exceptions on my side. Promises were a bit new to me (my code uses the async lib throughout) so I had to make sure any promises coming from your lib were resolved. I had code like this:

return player.system.applyPreset(preset)
    .then(() => myCallback());

Which ignored any thrown exceptions (e.g. a soap call in your lib that returned 500). So I just need to make sure I follow the Promise rules when interfacing with your lib.

return player.system.applyPreset(preset)
    .then(() => myCallback())
    .catch(e => myCallback(e));

[jishi]

Ah, yes. If you are using the library directly, you need to handle any potential rejects.

However, remember that it is easy to chain promises if you ever have that need, so if you want to call multiple promises and then invoke your callback, it is probably to do that in one call, and not rely on async for the Sonos calls.

Another thing to remember is that when you invoke a callback in a promise, any exceptions that are thrown from that callback, bubbles up into the promise and causes a rejection rather than a global uncaught exception. This is problematic in some cases where you have a framework relying on uncaught exceptions (for example, express, restify, hapi or similar). In those cases, I advise you to invoke the callback on the next tick (using setImmediate), like this:

return player.system.applyPreset(preset)
    .then(() => setImmediate(myCallback))
    .catch(e => setImmediate(myCallback, e));

This "breaks out" of the promise chain and re-allow an exception to be thrown globally.

[sseiber]

OK good to know. I'm using the hapi framework. I'm assuming that since the interface layer to node-sonos-discovery is the last line of any exceptions that might be generated from you then any "catch" I implement would be resolved there. From that point on I'm back in my code.

From what you suggest, my callback is technically still within the context of the .then and .catch clauses and my own exceptions could be swallowed up. From the description of setImmediate Node would queue the processing of my callback after the resolution of your promise.

Does that sound right?

[jishi]

Yes, if an exception is thrown in a catch handler of a promise, it will boil over to the next catch chain, which in your case would be lacking, thus causing an unhandledRejection rather than an uncaughtException. The setImmediate remedies that.