Unable to capture connection failure events and timeout is ignored

[joelnet]

Describe the bug Unable to capture connection failure events. Documentation does not have an .on("error", ...) documented so I need to ask for help.

To Reproduce Use the following code with connect set to either invalidHost or noPortHost. Both behave differently, yet do not emit any errors.

• In the case of invalidHost, the app will hang indefinitely. This can happen with an invalid host, or if the zookeeper server is temporarily unreachable. timeout is ignored.
• In the case of noPortHost, the app quits without any errors.

import ZooKeeper from "zookeeper";

const invalidHost = "localhost:2181";
const noPortHost = "localhost";

const client = new ZooKeeper({
  connect: noPortHost,
  timeout: 1000,
});

client.on("error", (err) => {
  console.log(`ZooKeeper error: ${err}`);
});

client.on("connect", () => {
  console.log('EVENT: "connect"');
});

client.on("close", () => {
  console.log('EVENT: "close"');
});

client.connect((err) => {
  if (err) {
    console.error(`ERR: ${err}`);
    process.exit(1);
  }

  console.log("Connected to ZooKeeper.");
});

Expected behavior I expected either client.connect to call the callback with err set, or the EventHandler to emit an error event.

I expected timeout to close the connection and not hang the app indefinitely.

I expected all events to be documented or a link provided in the documentation.

Thanks I just wanted to say thanks to all those who maintain this library! Cheers 🍻

[joelnet]

I am able to mock the behavior I expected using setTimeout and manually emitting an error event on the client. (see comments below for augmented code)

import ZooKeeper from "zookeeper";

const invalidHost = "localhost:2181";
const noPortHost = "localhost";

const timeout = 1000;

const client = new ZooKeeper({
  connect: invalidHost,
  timeout,
});

client.on("error", (err) => {
  console.log(`EVENT error: ${err}`);
});

client.on("connect", () => {
  console.log('EVENT: "connect"');
});

client.on("close", () => {
  console.log('EVENT: "close"');
});

// use setTimeout to obey the `timeout` set above.
const connectTimeout = setTimeout(() => {
  client.emit("error", new Error("ZooKeeper connection timeout"));
}, timeout);

client.connect((err) => {
  // clear the timeout, it is no longer necessary.
  clearTimeout(connectTimeout);

  if (err) {
    console.error(`ERR: ${err}`);
    process.exit(1);
  }

  console.log("Connected to ZooKeeper.");
});

With the setTimeout in place, the error event is emitted and the client.connect callback is called with err set.

EVENT error: Error: ZooKeeper connection timeout
ERR: Error: ZooKeeper connection timeout

I am hoping to mimic this behavior with the Zookeeper client directly instead of using the setTimeout hack.

[DavidVujic]

Hi @joelnet, thank you for reporting and providing very useful code examples!

In this case, I think the client will try to connect until explicitly shut down. The timeout is used once having a connection and telling the server when to delete ephemeral nodes and such if loosing connection.

There's also a connecting event available that is important to keep track on when connected, and when disconnected. I think that your setTimeout example is actually a proper way of handling it, even if it might feel like a hack. It should probably be activated when entering the connecting state.

Something like:

client.on('connecting', () => {
  // set the timeout here, that should be cleared once the `connect` is triggered.
});

[joelnet]

Ahh okay. That is good to know. I'll proceed with the setTimeout method then.

Oh, I am also unable to get the connecting event to fire. I'm using this example below and I never see the event.

import ZooKeeper from "zookeeper";

const invalidHost = "localhost:2181";
const noPortHost = "localhost";

const timeout = 1_000;

const client = new ZooKeeper({
  connect: invalidHost,
  timeout,
});

client.on("connecting", () => {
  console.log('EVENT: "connecting"');
});

client.connect((err) => {
  if (err) {
    console.error(`ERR: ${err}`);
    process.exit(1);
  }

  console.log("Connected to ZooKeeper.");
});

Do you have an example where the connecting event is working? Or were you suggesting that I manually emit a connecting event?

[DavidVujic]

The connecting event will be fired if the client would loose an existing connection from a ZooKeeper server.

I have an example from the examples folder here - even if the setTimeout thing isn't implemented there yet (but it should).

Once connected, but losing the connection: the connecting event will trigger. When the connection is established again, the connect event will trigger (and there you would clear the timeout). The naming of the events isn't perfect, but I hope this clear things out.

[joelnet]

The connecting event will be fired if the client would loose an existing connection from a ZooKeeper server.

I see. I had a misunderstanding that it would also fire when client.connect was called.

I'll have to play around with this a bit more. Thanks!

[joelnet]

Ok, I was able to get your suggestions working for my case.

Here are the things I have done:

1. Added client.emit("connecting") after calling client.connect because I also want this to fire on the initial connect.
2. connecting event creates the timeout
3. connect and close events clear the timeout. error will call close, so indirectly, error will also clear.

import ZooKeeper from "zookeeper";

const host = "localhost:2181";
const timeout = 2_000;

let connectTimeout;

const client = new ZooKeeper({
  connect: host,
  timeout,
});

client.on("error", (err) => {
  console.log(`EVENT error: ${err}`);
  client.close();
});

client.on("connect", () => {
  console.log('EVENT: "connect"');
  clearTimeout(connectTimeout);
});

client.on("close", () => {
  console.log('EVENT: "close"');
  clearTimeout(connectTimeout);
});

client.on("connecting", () => {
  console.log('EVENT: "connecting"');

  clearTimeout(connectTimeout);
  connectTimeout = setTimeout(() => {
    client.emit("error", new Error("ZooKeeper connection timeout"));
  }, timeout);
});

client.connect((err) => {
  if (err) {
    console.error(`ERR: ${err}`);
    process.exit(1);
  }

  console.log("Connected to ZooKeeper.");
});

client.emit("connecting");

[DavidVujic]

Great @joelnet!

Would it be okay for me to borrow your code when updating the connect-part of the examples in this repo?

[joelnet]

Would it be okay for me to borrow your code when updating the connect-part of the examples in this repo?

Of course! Let me know where I can help out.

[DavidVujic]

I have updated the example code using setTimeout: https://github.com/yfinkelstein/node-zookeeper/blob/master/examples/wrapper.js#L25

Closing this, please open a new issue (or re-open) if there's something I missed out on that should be done.