parnic / node-screenlogic

Pentair ScreenLogic Javascript library using Node.JS
https://www.npmjs.com/package/node-screenlogic
MIT License
53 stars 15 forks source link

add .on(‘error’) handlers that emit the error #21

Closed schemers closed 4 years ago

schemers commented 4 years ago

First, thanks for writing this module! I was able to quickly write a Homebridge module using it.

Recently, I added some Telsa Powerwalls to back up my house when the power goes out, and noticed that Homebridge was crashing when the power walls kicked in after the power from the grid went out.

My pool equipment isn't backed up during a power outage, so when my Homebridge module that is using node-screenlogic polled for status, it caused an error to be emitted inside of node-screenlogic, which causes Homebridge to crash due to an unhandled error event.

This behavior is documented:

For all EventEmitter objects, if an 'error' event handler is not provided, the error will be thrown, causing the Node.js process to report an uncaught exception and crash unless either: The domain module is used appropriately or a handler has been registered for the 'uncaughtException' event.

the fix was to add an .on('error') handler anywhere that errors could be emitted internally, and then propagate those errors out via FindUnits/RemoteLogin/Connection, which thankfully are already EventEmitters.

Then, in my code, I can add .on('error') and properly deal with the error.

Here is the before test when I configure an unreachable IP address. Homebridge ends up crashing:

[5/25/2020, 10:27:11 AM] Loading 1 platforms...
[5/25/2020, 10:27:11 AM] [ScreenLogic] Initializing ScreenLogic platform...
[5/25/2020, 10:27:11 AM] [ScreenLogic] Fetching ScreenLogic Info...
[5/25/2020, 10:27:11 AM] Loading 0 accessories...
[5/25/2020, 10:28:27 AM] Error: connect ETIMEDOUT 192.168.86.254:80
    at TCPConnectWrap.afterConnect [as oncomplete] (net.js:1129:14)
rmbp16:.homebridge $ 

After the fix, I can properly catch and log, and Homebridge keeps running:

[5/25/2020, 10:24:32 AM] Loading 1 platforms...
[5/25/2020, 10:24:32 AM] [ScreenLogic] Initializing ScreenLogic platform...
[5/25/2020, 10:24:32 AM] [ScreenLogic] Fetching ScreenLogic Info...
[5/25/2020, 10:24:32 AM] Loading 0 accessories...
[5/25/2020, 10:25:47 AM] [ScreenLogic] unable to get pool config: Error: connect ETIMEDOUT 192.168.86.254:80
    at TCPConnectWrap.afterConnect [as oncomplete] (net.js:1129:14) {
  errno: 'ETIMEDOUT',
  code: 'ETIMEDOUT',
  syscall: 'connect',
  address: '192.168.86.254',
  port: 80
}
Setup Payload:
X-HM://0023ISYWYEC2A
Scan this code with your HomeKit app on your iOS device to pair with Homebridge:

I can't think of any negative side effects this change will have to existing callers, since either way if they do nothing they end up with an unhandled error event. With the fix however, they at least have the opportunity to handle the error.

parnic commented 4 years ago

I love it. Thanks for the contribution!

schemers commented 4 years ago

thanks! Are you in a place where you can push a new release? No worries if not, I can point to my branch until you are.

parnic commented 4 years ago

Sure.