search

hobbyquaker / lgtv2

Control LG WebOS TV using node.js :tv:
MIT License
332 stars 45 forks source link

In the event of a successful request, don't fire a timeout later. #3

Closed forty2 closed 8 years ago

forty2 commented 8 years ago

Likewise, in the event of a timeout, don't fire a 'success' if it happens to come in while processing the timeout (much less likely).

I think this is a bug -- in my experience, the old version will always fire a timeout after 15s, even when the request completes successfully. What do you think? Feel free to accept/decline/discuss as you deem appropriate.

hobbyquaker commented 8 years ago

Hi Zach, I think we could solve this with less code, could you pls take a look at https://github.com/hobbyquaker/lgtv2/commit/7acc6830eab7517c7b6d9b98455ac2352cf959fe? In case of a successful request callback the reference in callbacks[cid] will be deleted, so I just added a check inside the setTimeout if the reference still exists. Do you think this would solve the issue also? Regards, Sebastian

forty2 commented 8 years ago

The only potential problem I see with not clearing the timeout is that if calling cb() takes longer than the timeout duration, both could still get called. Not sure if that's worth worrying about.

Having said that, I'm 99% sure https://github.com/hobbyquaker/lgtv2/commit/7acc6830eab7517c7b6d9b98455ac2352cf959fe will fix the problem I was seeing (I won't be able to test until Tuesday) and I'm happy to go with it.

hobbyquaker commented 8 years ago

replaced by https://github.com/hobbyquaker/lgtv2/commit/bed88d5047d8d33fd6afba82aceb5edfbabcd555