jishi / node-sonos-http-api

An HTTP API bridge for Sonos easing automation. Hostable on any node.js capable device, like a raspberry pi or similar.
http://jishi.github.io/node-sonos-http-api/
MIT License
1.85k stars 460 forks source link

echo-sonos Lambda timeout issue #231

Closed jplourde5 closed 8 years ago

jplourde5 commented 8 years ago

The promises version does not throw any king of response back until everything is finished. The problem is that some of the appleMusic functions take some time to finish which is causing Lambda to timeout and Echo/Alexa to say there was a problem, when in reality, everything worked fine.

Is there a way to throw a response back and then continue to work on additional functions?

GregRocket commented 8 years ago

I noticed that the beta version of node-sonos-http-api has the same timeout problem when issuing a playlist command with echo-sonos. The playlist will play and work correctly, but sometimes (not always) the response from echo/alexa is "there was a problem with the requested skills response" instead of the expected "OK".

This behavior started happening at some point when I started using/testing the beta version of node-sonos-http-api. I never experience the error response while using the master version.

I increased the Lambda Function timeout from 3 to 10 seconds in the Lambada Management Console which seems to have stopped the error from happening for me. Still testing this but so far it seems to have helped.

jplourde5 commented 8 years ago

I increased the Lambda timeout to 30 seconds and that resolves the issue for everything except when I ask for an artist to play and there are upwards of 50 tracks to get queued, which can take longer than 30 seconds. I tried setting the timeout to 5 minutes and then something strange happened. After about maybe a minute I got a 500 error and then go stuck in some kind of loop. Could have just been a problem with the Alexa test function.

What I'd really like to do is return status:success quickly and then continue to queue up the tracks for as long as it takes. I've tried a few things but nothing has worked so far.

jplourde5 commented 8 years ago

Does the Promises used here support progressBack?

jishi commented 8 years ago

Hi guys. This was a deliberate change, to simplify chaining of calls which was impossible before. I don't want to change this globally though, but might make sense for certain calls, or maybe have it controllable via a special header or similar. I'll have to think about it.

Maybe we should investigate if there is a way to speed up multiple queuing of tracks, if someone could make test and see if adding multiple tracks via official means are equally slow or not.

jishi commented 8 years ago

In addition, my last suggestion on this issue: https://github.com/rgraciano/echo-sonos/issues/15

Would mitigate this completely, if it worked.

jplourde5 commented 8 years ago

SQS etc. might be interesting. For most use cases what we have now works with a slightly longer timeout, with the exception of adding many new tracks to the queue. The trick is that we still want to send a quick response after the search checks out with positive results so that the Echo can say all is good, or let you know there was a problem, but waiting for 1 to a few minutes is a long time for the outcome to be announced. I'm not sure that a queue approach would give any valid feedback unless some sort of bi-directional approach was employed, which seems rather complicated. Seems simpler just to be able to throw back an early success once things are known to be good, without having to wait for everything else to finish.

It is an interesting way to avoid poking holes in your firewall though.

jishi commented 8 years ago

Well, pub/sub is a not acknowledging technique so it would consider a successful publish as success. If you don't have any retry logic or similar, this might be fine.

jplourde5 commented 8 years ago

Exactly. That is the problem with that approach. Everything would be a "success", even if there was a problem on the other end. Maybe the task needs to be broken up into two calls, one for clearing the queue and starting the first song, and a second call to load the rest of the queue and ignore the timeout. A little bit of a kludge, but it would work.

jishi commented 8 years ago

But what kind of feedback do you get from Alexa with a failed request from the lambda?

jplourde5 commented 8 years ago

It can say through Echo/Alexa what ever you want it to say based on the error condition or message that is returned by your api.

jishi commented 8 years ago

On a sidenote, I just found out that there is a AddMultipleURIsToQueue call one could make. The official controller calls that one in chunks of 16 URIs (not sure if that is a hard limit).

Maybe that would speed it up some, even though it seems kind of slow anyway, adding 68 tracks takes about 30 seconds.

jishi commented 8 years ago

In addition, it is possible to return response at any time and continue to work if that is a preferred behavior, but it will make it a lot harder for people that uses other means to call these actions (not all users use echo).

jplourde5 commented 8 years ago

Good point. Maybe I can have a nowait action that does the same as the quename action, except it throws back an early return.

jplourde5 commented 8 years ago

... How would I return early?

jishi commented 8 years ago

The Promise chain that you return, will be waited for, but you can still continue with another promise chain that would happen asynchronously.

Example:

function returnsPromise() {
  return Promise.resolve()
  .then(() => {
    // This will be waited for
    return Promise.resolve();
  })
  .then(() => {
     // This will be waited for
    return Promise.resolve();
  })
  .then(() => {
    // This will NOT be waited for
    // Since we didn't return this promise. The outer chain is "ended"
    // because we are implicitly returning undefined
    Promise.resolve()
    .then(() => {
      // These will still wait for previous promise to finish, but not keep the whole chain waiting
      return Promise.resolve();
    })
    .then(() => {
      // This too.
      return Promise.resolve();
    });    
  });
} 

Maybe not super-clear, but maybe you get the concept.