juliuscc / heos-api

🎵 A zero-dependency low level api-wrapper for communicating with HEOS devices for Node.js 🎵
MIT License
79 stars 12 forks source link

Writing to an interrupted connection does not fail #24

Closed maszaa closed 5 years ago

maszaa commented 5 years ago

When HEOS device is disconnected from the network and I try to write to the connection (e.g. .write('system', 'heart_beat')) the heos-api does not fail. It would be good to know if the connection to the device has an interruption.

Possible solution: HeosConnection could set timeout to its socket via socket.setTimeout(timeout) and have socket.on('timeout', listener) event handler which would close the connection.

juliuscc commented 5 years ago

I think it makes sense that the user could listen to the timeout event exactly like they can listen to an error or close event. And also that there would be an internal event listener that closes the connection.

Are you sure that that is the problem though? As I understand it, the default behaviour of a socket in node is that there is no timeout. If the other connected device would timeout then they would close the connection, which would trigger a close event.

I could submit a pull request and you can try to see if it solves your problem. I might be wrong. Otherwise I do not think that timeout should ever be listened to as it should not happen.

maszaa commented 5 years ago

In this case, I think once timeout is set atimeout event is emitted by net.Socket if HEOS device

juliuscc commented 5 years ago

I created a PR #25. Please try it. I hope it works. It does two things.

  1. When the socket times out the connection is closed. This is contrary to Node.js' Net module where

The user must manually close the connection

  1. The timeout event is exposed so that users can listen to it.
maszaa commented 5 years ago

Had to change the other onError to onTimeout.

Did some testing and this doesn't solve my problem. I even tried setting timeout for the socket but it timeouts if nothing is written to the socket before the set timeout. And still I wasn't able to get any kind of notification if writing to the socket didn't succeed.

But could we detect if HEOS device becomes unreachable?

juliuscc commented 5 years ago

Firstly now when I tested I had forgotten to rename the function so it did not even compile. I updated the PR. Maybe you did not test the code in the PR?

The easiest way to test code from a PR is to:

  1. Clone the repo.
  2. Run npm run build.
  3. Create a file in the project called something like temp.js.
  4. Write your test-code in temp.js where you in the top write: const heos = require('.').
  5. Run your temporary file by running node temp.js in a terminal.
juliuscc commented 5 years ago

I can help you do some simple debugging. Step 1 should be to make sure that you can even discover your HEOS device. If you run this code in your project it should tell you if you can discover a device:

const heos = require('.')
heos.discoverOneDevice().then(console.log)

It should return an IP address. In my terminal it looks like this:

~/Coding/node-misc/heos-api feature/timout_listener
❯ node temp.js 
192.168.1.154
juliuscc commented 5 years ago

Step 2 should be to make sure that you can connect to your HEOS device with a normal TCP socket. If step 1 worked try the following code:

const heos = require('.')
const { createConnection } = require('net')

heos.discoverOneDevice().then(host => {
    const socket = createConnection({ port: 1255, host, localPort: 0 }, () => {
        console.log('Socket connected')
        socket.write('heos://system/heart_beat\r\n')
    })

    socket.on('close', () => {
        console.log('Closing socket')
    })

    socket.on('end', () => {
        console.log('Ending socket')
    })

    socket.on('error', err => {
        console.log('Socket had an error')
        console.error(err)
    })

    socket.on('timeout', () => {
        console.log('Socket had a timeout')
    })

    socket.on('data', data => {
        console.log('data recieved')
        console.log(data.toString())
        socket.end()
    })
})

My output looks like this:

~/Coding/node-misc/heos-api feature/timout_listener
❯ node temp.js
Socket connected
data recieved
{
    "heos": {
        "command": "system/heart_beat",
        "result": "success",
        "message": ""
    }
}

Ending socket
Closing socket
maszaa commented 5 years ago

Here is the snippet which I'm using for testing

const heos = require('.')

async function test() {
  const connection = await heos.connect('192.168.0.105');
  connection.onAll(console.log)
  connection.onTimeout(() => console.log('timeout'))
  setInterval(() => {
    connection.write('system', 'heart_beat');
  }, 5000)
}

test()

For testing I have set my HEOS device's network control off (Setup -> Network -> Network Control -> Off) which means that HEOS device is unreachable after powering it off.

When I power off my device I don't get any error though I'm writing heartbeat every 5 seconds.

What I would like to have is that heos-api could tell me if the socket is "alive" (i.e. the remote end is still connected/receiving). But is that even possible?

After I power on the unit I get

{ Error: read ECONNRESET
    at TCP.onStreamRead (internal/stream_base_commons.js:111:27) errno: 'ECONNRESET', code: 'ECONNRESET', syscall: 'read' }
Error [ERR_STREAM_DESTROYED]: Cannot call write after a stream was destroyed

Maybe I have to deal with just that.

juliuscc commented 5 years ago

That is very very strange. I really do not know why that happens. What output do you get when running "step 2". Modify it if you want to send the heartbeat periodically, but make sure that you only use a pure nodejs socket to write, and subscribe to all the relevant events. The socket might get closed for some reason.

maszaa commented 5 years ago

Tried with this

const heos = require('.')
const { createConnection } = require('net')

function test() {
  const socket = createConnection({ port: 1255, host: '192.168.0.105', localPort: 0 }, () => {
    console.log('Socket connected')
  })

  socket.on('close', () => {
    console.log('Closing socket')
  })

  socket.on('end', () => {
    console.log('Ending socket')
  })

  socket.on('error', err => {
    console.log('Socket had an error')
    console.error(err)
  })

  socket.on('timeout', () => {
    console.log('Socket had a timeout')
  })

  socket.on('data', data => {
    console.log('data recieved')
    console.log(data.toString())
  })

  setInterval(() => {
    console.log()
    socket.write('heos://system/heart_beat\r\n')
  }, 2000)
}

test()

and got this

Socket had an error
{ Error: read ECONNRESET
    at TCP.onStreamRead (internal/stream_base_commons.js:111:27) errno: 'ECONNRESET', code: 'ECONNRESET', syscall: 'read' }
Closing socket
Socket had an error
Error [ERR_STREAM_DESTROYED]: Cannot call write after a stream was destroyed

after a while after powering the device off and on.

Before powering the device off it works if that's been left unclear.

Seems that at least my device closes connection after it's powered on again... maybe there is some network initializing phase at the power on when the network control is off.

Usually I keep the network control on but I would like react to e.g. power outages and such.

juliuscc commented 5 years ago

Yes this is a dilemma. I am not sure what to do. At least you can answer to it receiving an error and close the connection yourself. I am considering removing all of my own connection logic, and just exposing the socket to the user. At least then the user knows how the socket behaves, and can rely on Node.js documentation and forums.

maszaa commented 5 years ago

I am able to solve this issue by sending heartbeat every X seconds and keeping track whatever the device has responded with 'success' within the X seconds.

As this seems to be a possible issue on how HEOS devices handle connections side I'll close this issue.

juliuscc commented 5 years ago

There is a boolean that indicates if a connection is closed or not. Link. Would it help to use that one? I could try a PR where I expose the whole socket, and the user can access all of these low level constructs.