svrooij / node-sonos-ts

:speaker: Sonos control library, use this library in your own appliction.
https://sonos-ts.svrooij.io/
MIT License
84 stars 18 forks source link

Let me handle subscription error #127

Closed hklages closed 3 years ago

hklages commented 3 years ago

Is your feature request related to a problem? Please describe

If user accidentally enters an invalid ip (in this case 192.168.178.1) for a SONOS player - or the player is not available - he receives a "succces" message in the program although the subscription failed - see this log file extract:

sonos:service:renderingcontrol:192.168.178.1 Subscriping for events failed FetchError: request to http://192.168.178.1:1400/MediaRenderer/RenderingControl/Event failed, reason: connect ECONNREFUSED 192.168.178.1:1400
    at ClientRequest.<anonymous> (C:\Users\hekla\Development\node-red-contrib-sonos-events\node_modules\node-fetch\lib\index.js:1461:11)
    at ClientRequest.emit (events.js:315:20)
    at ClientRequest.EventEmitter.emit (domain.js:486:12)
    at Socket.socketErrorListener (_http_client.js:469:9)
    at Socket.emit (events.js:315:20)
    at Socket.EventEmitter.emit (domain.js:486:12)
    at emitErrorNT (internal/streams/destroy.js:106:8)
    at emitErrorCloseNT (internal/streams/destroy.js:74:3)
    at processTicksAndRejections (internal/process/task_queues.js:80:21) {
  type: 'system',
  errno: 'ECONNREFUSED',
  code: 'ECONNREFUSED'

Describe the solution you'd like

You catch the error and just send a debug message.

await this.subscribeForEvents()
            .catch((err) => {
              this.debug('Subscriping for events failed', err);
            });

Wouldn't it make sense to also throw the error to enable me to handle it? Hope that routes thru and and I only have to add a .catch to my call.

await player[serviceName].Events.on('serviceEvent', ...)

with player=new SonosDevice( ....)

Describe alternatives you've considered

I can validate the ip address before subscribing - for instance with http request to "http://192.168.178.1:1400/info" or do a discovery.

Additional context

Apart from beta "Invisible" and beta "SonosEventListener" this is the last thing I would like to solve. I prefer to proactive check things or give feedback if something goes wrong.

Everything else in sonos-ts works much better then expected for "my" event management and it now works fine.

Well done!

Next step is to make a plan how to migrate my "bigger" package node-red-contrib-sonos-plus from node-sonos to sonos-ts.

svrooij commented 3 years ago

I have no clue on how that would work. Because the subscribe command is triggered from an event in the first place. This is related to checking the event name #78

If you know a solution for that one, it might also be a solution for this issue.

help or ideas are greatly appreciated.

hklages commented 3 years ago

Can not believe it - haha. You always have an idea :-) Ok we can close this issue.

I use the work around and just send an http request with time out. Now my node is complete. User can select the listener ip, the player, events - all from drop down list. He gets feedback if something went wrong. The port is also shown in case a subscription was successful. With RBE node only changed data are forwarded.

player validation

configuration

svrooij commented 3 years ago

I can see this would be a useful addition, I just have no clue on how to fix it?

Do you think we should keep this issue open and hope that someone else got a great idea on how to fix this?

The screenshots above look awesome, by the way!!! Very user friendly!

hklages commented 3 years ago

could make sense - close it whenever you want. When a good idea comes up, I will test it.

svrooij commented 3 years ago

I had a beer, and then figured that if you can subscribe for events, why not subscribe for errors. Would that help? Something like:

device.Events.on(SonosEvents.Error, (err) => {
  console.error('Subscribe error', err);
});
hklages commented 3 years ago

Hi - no. It does not provide any information. Do you catch that case (invalid ip) and use this.emit( 'error', err )? - I could not find anything in your code.

What can happen: invalid ip, sonos player but switched off or not reachable, not a sonos player. These cases I now filter before I do the subscription (using axios and a helper isTruthyProperty to validate a property). In addition I get the capabilities like VOICE (necessary for MicEnabled), .... and only subscribe to those being available.

let response
  try {
    response = await request.get(`http://${player.host}:${player.port}/info`, { timeout: 4000 })  
  } catch (error) {
    debug('invalid player - http request >>%s', JSON.stringify(error.message))
    // TODO check ECONNREFUSED
    throw new Error('invalid player - error or timed out')
  }
  if (isTruthyProperty(response, ['data', 'device', 'capabilities'])) {
    capabilities = response.data.device.capabilities
  } else {
    throw new Error('invalid player - missing capabilities)')
  }
github-actions[bot] commented 3 years ago

:tada: This issue has been resolved in version 2.4.0-beta.4 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

github-actions[bot] commented 3 years ago

:tada: This issue has been resolved in version 2.4.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket: