Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Connection loss causes infinite loop #172

Closed
DavidVujic opened this issue Jun 3, 2019 · 4 comments
Closed

Connection loss causes infinite loop #172

DavidVujic opened this issue Jun 3, 2019 · 4 comments
Assignees
Labels

Comments

@DavidVujic
Copy link
Collaborator

DavidVujic commented Jun 3, 2019

Connection loss causes infinite loop
When a connection loss occur, and the library reconnects, there is a scenario that causes the library to get stuck in an infinite loop between the zk_io_cb and yield functions in
the C++ wrapper src/node-zk.cpp.

To Reproduce
Steps to reproduce the behavior:

  1. run the ZooKeeper server in a docker container
  2. run a Node.js ZooKeeper client in a different docker container.
  3. The Node.js client should have watchers on one or more nodes
  4. run docker stop on the ZooKeeper server, and wait a couple of seconds.
  5. Start the server again with docker start

This has only been reproduced in a docker container with a Linux OS.

Error Log example
A large number of log rows like this:
ZOO_ERROR@zk_io_cb@353: yield:zookeeper_process returned error: -117 - (not error) no server responses to process

Expected behavior
The library should emit a close event.

Desktop:

  • OS: CentOS
  • Node.js version: 10
  • C/C++ compiler and version: gcc 7
@DavidVujic
Copy link
Collaborator Author

As I understand it, the C client (zookeeper.c) returns the status ZNOTHING when a socket read returns 0 (zero). The C++ wrapper does not handle that scenario, and will re-run the yield function.

A possible solution:

if (rc != ZOK) {
     LOG_ERROR(("yield:zookeeper_process returned error: %d - %s\n", rc, zerror(rc)));
            
     // Explicitly check for the return code, and close the session.
     // That will in turn emit a close event to the Node.js code
     if (rc == ZNOTHING) {
         zk->realClose(ZOO_EXPIRED_SESSION_STATE);
         return;
      }
}

@DavidVujic DavidVujic added the bug label Jun 3, 2019
@DavidVujic
Copy link
Collaborator Author

@mdlavin @kuebk Do you have any input on this? My C/C++ skills are very limited 😄

@kuebk
Copy link
Collaborator

kuebk commented Jun 9, 2019

Code seems to be ok, but I'm not sure should we close using ZOO_EXPIRED_SESSION_STATE, what about ZOO_CLOSED_STATE?

@DavidVujic
Copy link
Collaborator Author

I didn't find a ZOO_CLOSED_STATE in the wrapper code. Maybe it should be added in a separate PR @kuebk?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants