Change setTimeout to setInterval

[randomnerd]

Change setTimeout to setInterval for consistent reconnection attempts.
Otherwise it just tries to reconnect once and if server was not online it just hangs forever doing nothing...

[vsivsi]

This seems reasonable to me, I've run into this issue in the past and worked around it in other ways, but this PR does seem like the correct behavior.

What do you think @emgee3? I can merge this, but I don't have the power to push an update to npm.

[emgee3]

I don't really do a whole lot of Meteor work these days @vsivsi , so while I'll help out when I can, I trust your decision on any such matters. I have added you to the people who can publish on npm.

[vsivsi]

Great, I'll take a closer look at this in the morning and push an update to npm sometime tomorrow.

[vsivsi]

@randomnerd Hi, I've looked into this a bit and while I understand what you are trying to achieve, it doesn't appear to be as simple as your proposed change assumes it to be.

One basic issue is that this line now needs to be a clearInterval(). Right?

The bigger issue seems to be that the existing autoReconnect functionality doesn't seem to have really been designed to handle indeterminately long server / connectivity outages. It's more for the simple TCP socket disconnections. This is why I said in my OP above that I've handled this in "other ways" at the application level, because the correct thing to do when a server or connectivity drops may not be to just keep retrying endlessly on some interval.

Anyway, the change you submitted causes at least one unit test to fail here:
https://github.com/oortcloud/node-ddp-client/blob/master/test/ddp-client.js#L153

I think for some version of this PR to be acceptable, you should to construct a mocked failing unit test for the case you are trying to address, and then use that to show your changes address that case without breaking anything else.

Let me know what you think.

[randomnerd]

Hi there.
Just wondering if you can share how you handled this in “other ways” :)
Not sure if I have enough skill/time to do this the right way with unit tests for this package...

Thanks!

On 12 Aug 2015, at 02:06, Vaughn Iverson [email protected] wrote:

@randomnerd https://github.com/randomnerd Hi, I've looked into this a bit and while I understand what you are trying to achieve, it doesn't appear to be as simple as your proposed change assumes it to be.

One basic issue is that this line randomnerd@f4bb574#diff-c83a3d1a3a7606c1385905057a6af8c9L104 now needs to be a clearInterval(). Right?

The bigger issue seems to be that the existing autoReconnect functionality doesn't seem to have really been designed to handle indeterminately long server / connectivity outages. It's more for the simple TCP socket disconnections. This is why I said in my OP above that I've handled this in "other ways" at the application level, because the correct thing to do when a server or connectivity drops may not be to just keep retrying endlessly on some interval.

Anyway, the change you submitted causes at least one unit test to fail here:
https://github.com/oortcloud/node-ddp-client/blob/master/test/ddp-client.js#L153 https://github.com/oortcloud/node-ddp-client/blob/master/test/ddp-client.js#L153
I think for some version of this PR to be acceptable, you should to construct a mocked failing unit test for the case you are trying to address, and then use that to show your changes address that case without breaking anything else.

Let me know what you think.

—
Reply to this email directly or view it on GitHub #63 (comment).

[vsivsi]

The DDP connection object is an event emitter. If you add a handlers for 'socket-error' and/or 'socket-close' then you can implement whatever re-connection attempt logic you want. Here's some sample code that hooks those events to implement an orderly shutdown. You could just as easily use them to wait until a connection/server returns, say with an exponential backoff...

https://github.com/vsivsi/meteor-job-collection-playground-worker/blob/master/work.coffee#L89-L102

[rantecki]

I just ran into the same underlying issue where if the server goes away, the client attempts to reconnect just once. I'm not sure I understand the point of that?

The way I solved this was to remove the check for ! self._connectionFailed in _recoverNetworkError(). It now attempts to reconnect every autoReconnectTimer ms (similar to this PR I imagine). I'm not sure of the reason that logic is there, but perhaps someone can enlighten me?

[vsivsi]

@rantecki I agree, something is fishy with self._connectionFailed being in that test. I'll look at it more closely, and if it holds up, make that change in the next release. Thanks