node-zkplus icon indicating copy to clipboard operation
node-zkplus copied to clipboard

Session expired bad behavior

Open vvo opened this issue 12 years ago • 2 comments

Hello.

Since you switched to your mcavage/node-zookeeper fork, there's no more a 'close' event sent when a zookeeper session expires.

We have been able to reproduce this, very easy:

  • Start a zookeeper server in a VirtualBox
  • Connect to your zookeeper server from your host machine, with a very low timeout (seems 4000ms is the lowest), using latest node-zkplus
  • Stop communication between zookeeper server and your client: sudo ifconfig vboxnet0 down
  • Wait 5-6s
  • Restart communication: sudo ifconfig vboxnet0 up

What happens:

  • all watches are getting -1 etc.. as seen in https://github.com/yfinkelstein/node-zookeeper#known-bugs--issues

Expected behavior:

  • a close event should be sent by the native zookeeper lib

Your fork does not send a close event, thus our code around session disconnection and reconnect does not work anymore: 6a97b307fcaefee0e5d9bde0767f4b4d2ed5f75c

Putting back "official" node-zookeeper works well, because we do receive a close event when session expires.

I guess we could hack https://github.com/mcavage/node-zkplus/blob/733906cd7f7faea642cfb81c1db8c5c7f7f1f056/lib/client.js#L799 to watch for session event and call onClose manually.

What do you think ?

PS1: I hope you do monitor your zk connection status from a client side. Because when session expires (eg: network interruption, garbage collector working for more than zookeeper timeout), you will never know it ONLY if you do watch for -1 state in your watches.

PS2: We could not understand how is it possible that our nodejs client and zookeeper server were not able to reach each other for 30 seconds (our timeout), did you already have experienced similar issues with node-zookeeper? (your fork, the official)

vvo avatar Sep 25 '12 15:09 vvo

Hi Vincent,

Thanks for the report. Well, I really want to have node-zookeeper merge in my changes, but until they do zkplus can't use it in 0.8, so I guess the best thing would be either to hack around it in zkplus, or actually just fix this in the fork, since it's already diverged. I've talked with Yuri about merging mine back in, and I know he wants to, I'm guessing this just isn't high on his priority list - long winded way of saying "if we fix it in the fork it will probably make its way back to mainline eventually".

I probably don't see this because I do that poll on getState bit (i.e., "yes, I am aggressively monitoring the client state").

Thoughts?

m

On Tue, Sep 25, 2012 at 8:39 AM, Vincent Voyer [email protected]:

Hello.

Since you switched to your mcavage/node-zookeeper fork, there's no more an 'close' event sent when a zookeeper session expires.

We have been able to reproduce this, very easy:

  • Start a zookeeper server in a VirtualBox
  • Connect to your zookeeper server from your host machine, with a very low timeout (seems 4000ms is the lowest), using latest node-zkplus
  • Stop communication between zookeeper server and your client: sudo ifconfig vboxnet0 down
  • Wait 5-6s
  • Restart communication: sudo ifconfig vboxnet0 up

What happens:

  • all watches are getting -1 etc.. as seen in https://github.com/yfinkelstein/node-zookeeper#known-bugs--issues

Expected behavior:

  • a close event been sent by the native zookeeper lib

Your fork does not send a close event, thus our code around session disconnection and reconnect does not work anymore: 6a97b30https://github.com/mcavage/node-zkplus/commit/6a97b307fcaefee0e5d9bde0767f4b4d2ed5f75c

Putting back "official" node-zookeeper works well, because we do receive a close event when session expires.

I guess we could hack https://github.com/mcavage/node-zkplus/blob/733906cd7f7faea642cfb81c1db8c5c7f7f1f056/lib/client.js#L799to watch for session event and call onClose manually.

What do you think ?

Also, I hope you do monitor your zk connection status from a client side.

Because when session expires (eg: network interruption, garbage collector working for more than zookeeper timeout), you will never know it ONLY if you do watch for -1 state in your watches.

— Reply to this email directly or view it on GitHubhttps://github.com/mcavage/node-zkplus/issues/15.

mcavage avatar Sep 25 '12 15:09 mcavage

I switched back to node-zookeeper in our zkplus fork and it seems to work well. I know you already told me not to do so but for now it works.

About polling the state. I feel that if I need to do a setInterval() to check zookeeper state and deal with it, then it means I do not need zookeeper at all and better do my own simple datastore and poll it with setInterval()!

This compare to websockets() and long polling in browser. If I have websockets and I still need to setInterval an ajax request then I failed.

Perhaps I am wrong on this as I do not have a long experience with zookeeper and do not understand the whole idea behind it.

vvo avatar Sep 25 '12 16:09 vvo