Make disconnect detection more robust

[gizmotronic]

@DubbyTT's addition of error handling addressed a major reason why it was hard to handle disconnections. These changes address some of the situations that I discovered come up when working on my own similar changes:

• It's easy to get duplicate intervals firing, depending on what triggered the error. Make sure we clear the interval timer before setting up another.
• A spurious error is thrown in roomRegister when we close an existing WebSocket. This should be ignored since we're about to reconnect.
• Sometimes it takes an extraordinarily long time for the WebSocket to notice there's a problem. Provide a failsafe 2 minute timeout.

What follows is a very simple automatic reconnect example that doesn't require a watchdog.

var Bot    = require('ttapi');
var AUTH   = 'xxxxxxxxxxxxxxxxxxxxxxxx';
var USERID = 'xxxxxxxxxxxxxxxxxxxxxxxx';
var ROOMID = 'xxxxxxxxxxxxxxxxxxxxxxxx';
var bot    = new Bot(AUTH, USERID);
var error  = false;

var connect = function (roomid) {
  error = false;
  console.log('connect');
  bot.roomRegister(roomid, function (data) {
    console.log('registered = ' + (data.success ? 'true' : 'false'));
  });
};

var onError = function () {
  if (!error) {
    error = true;
    console.log('onError');
    setTimeout(function () {
      connect(ROOMID);
    }, 5000);
  }
};

bot
  .on('ready', function (data) {
    console.log('ready');
    connect(ROOMID);
  })
  .on('roomChanged', function (data) {
    console.log('roomChanged', data.room ? data.room.name : 'unknown');
  })
  .on('error', function (data) {
    console.log('error', data);
    onError();
  })
  .on('speak', function (data) {
    if (data.text.match(/^\/hello$/)) {
      bot.speak('Hey! How are you @' + data.name + '?');
    }
  });

[ghost]

DubbyTT's code looked fine the way it was, it didn't have to be changed.

[technobly]

@gizmotronic I think you have presented your changes well. I like some of what you've done, but I don't think this change is necessarily better than what we already have. In fact I prefer what we have for several reasons.

• The current implementation only needed 6 line additions to the TTAPI, and deleted nothing.
• The current implementation and example shows a clear way to set a timer on not just any error, but a websocket error. If multiple websocket errors are thrown (which there usually are), the example shows how to avoid setting up multiple timers by testing for "null". Only upon receiving verification from the TTAPI that the bot has in fact logged in and is "alive" will the timer be cleared.
• The current implementation fixed the same error handler.
• The current implementation doesn't care how many or how long the websocket errors are... once it gets them it is on a mission to log back in as soon as possible. Waiting two minutes does not make it more "robust", but it will create more confusion in your room for a longer period of time. With the current implementation I've seen down times of only 15 seconds (using the example's /uptime command).
• The current implementation is more lock-stepped about the error handling and reconnecting.
• In your commits you change your mind about the "disconnect" message and other error handling changes, which makes me feel like you haven't tested this very well.
• Your code compiled in a strange way.. go see if you can "catch" it ;-)

[gizmotronic]

Greetings,

• I don't even know why the size of the change is being brought up. New functionality often requires more code.
• The current implementation example isn't adversely affected, though you'll probably want to catch the error event, even if you just decide to ignore it.
• Yes, I agree that we both changed the whichServer http error handler. Thanks for pointing that out?
• My new code doesn't care how many websocket errors there or, or there timing, or whatever. I think you've completely misunderstood the change. The maintainPresence() function is there for the case where a websocket error is not emitted, but there's also been no response from the server. It's strictly a fail-safe and eliminates the requirement for an external watchdog timer. You can use a watchdog, of course, if you want to catch that condition sooner than later.
• I don't even know how to address the "lock-step" comment. What do you mean by that? I'd argue that throwing an event right away is more lock-step that waiting for a watchdog to fire, but I can't tell if we're talking about the same thing.
• I realized while writing up the documentation that there was no effective difference between the error event and the disconnect event. So sue me. I'm not too big to admit that an earlier design choice could have been improved.
• I assume you're speaking of the two onError() implementations. Automatic merge FTL. But while ugly and in need of fixing, it doesn't meaningfully affect operation. In any event thanks for the heads-up.

Regardless of all of this, there are several things addressed here that I feel are quite important.

1. The interval timer must be cleared in roomRegister. If you don't, you will create a new interval timer each time you register in another room.
2. The error handler should be cleared before closing an existing websocket in roomRegister. If you don't, you'll emit a spurious error event in a variety of common circumstances.
3. An error should be emitted when the server responds with something that JSON.parse() can't interpret. This frequently happens during both planned and unplanned Turntable outages.
4. The whichServer http error handling as it currently stands just logs a message instead of relaying the Error object up the chain. It's useful to be able to get a backtrace when debugging issues. This also means that you don't need to wait for a watchdog timer to fire if you want to handle it right away.
5. For advanced users, it's handy to be able to configure the presence ping time and, provided this change is accepted, to configure how long before a "no response" error event is emitted.

Finally, you should understand that I've had a watchdog-based implementation to do this for a very long time, and didn't require a single change to TTAPI to accomplish that feat. All you need to do in your watchdog routine is to check bot.lastActivity and make sure it hasn't been excessively far in the past. Since the data is readily available - just not documented - there's no need for an event to be emitted or responded to. (But, note that I didn't touch the 'alive' event that you added.)

[technobly]

@gizmotronic I'm pretty sure nobody besides you and I know much of what the hell we are talking about here... and I will add that I'm pretty sure you don't understand fully how my code works... AND I have some more comments about your example code... BUUUUT since I don't feel like typing up something to address your lengthy response... you win! :tongue:

[gizmotronic]

Actually, I was gonna say, I see that you're handling the wserror event, which I re-emitted as an error event. If I just changed on('wserror') to on('error') in your existing example, we'd both be pretty happy. Wouldn't we?

[samuri51]

i think bot.('wserror') is more descriptive of what's happening than bot.on('error'), that could literally mean anything given the context. in the websocket api you have 'error', but there you know your dealing specifically with websocket's not turntable bot's... didn't see anything wrong with current way its being handled. i think the option for it to be handled programatically rather than directly by the bot is fine. plus it allows the programmer to handle it the way they want to. rather be restricted by a single implementation

[gizmotronic]

The errors are not all websocket errors. You can inspect the Error object to see the specific source of the error, if that's important to you. Also, I deliberately chose the 'error' event because it has special handling.

[technobly]

After looking at this more, the benefit of these tweaks to the setInterval don't seem needed. The only way you are going to lag out after 120 seconds is if you are not connected to turntable.fm, and if that happened it threw some websocket errors a long time ago. You might as well start trying to reconnect, and stop trying when you are connected again. The current implementation does this. What you have submitted has compiler issues, possibly due to the old coffee-script version you are using... and with the constant changes and lack of testing I'm going to close this for now unless anyone besides @gizmotronic has tested this and feels it's working and provides value.

[gizmotronic]

You're wrong; a transparent proxy issue can easily create a situation where the websocket doesn't throw an error, and yet you also don't get responses. I wrote the fail-safe code because I don't live in an ideal world, apparently unlike you. Also, the version of CoffeeScript I'm using happens to be exactly the same one that Alain has been using; I would prefer to use a newer version, but I don't feel it's my call to make.

[technobly]

Orly, http://i.imgur.com/mueoLce.png

[gizmotronic]

rly

If he's updated, I'll be more than happy to. I downgraded in order to submit pull requests #164 and #173. But, I'd love for you to point out the compilation problem since I'm clearly too thick to see any functional difference.

[gizmotronic]

You know what - part of this is my fault for assuming that everyone sees the wisdom in splitting functionally distinct changes into separate commits. Another place I failed was putting needed bug fixes into the same pull request as an enhancement. I'm splitting out the bug fixes - one of which was pre-existing but exacerbated by the reconnect change (the interval timer issue), and the other of which was introduced by the same - and committing them directly. I'll refactor and re-submit the enhancement as a new pull request.

By the way, this does not nullify my previous request to be shown the compilation problem. I'd really like to know what you see (you've helped me before, so I don't doubt your skill).

[technobly]

It was just compiling differently... just look at the diffs. Stuff you didn't change was being changed. There's no reason not to compile with v1.6.2 since that's what was used on the last two commits of bot.js.

I would agree, start with your bug fixes.

Then move on to your feature proposals.