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

Unset handler on disconnect

Open pfree opened this issue 5 years ago • 1 comments

This fixes a bug where uv_poll_stop was called twice on a handle.

Explanation of the bug: In the OnDisconnect callback, it calls uv_poll_stop to stop watching the handle. Later, in OnConnect, it sees that the handle is non-NULL and calls uv_poll_stop again.

pfree avatar Jun 16 '20 21:06 pfree

Nice, I had the same issue when i rewrote this library. This is how i solved it

static void
on_disconnect (LDAP * ld, Sockbuf * sb, struct ldap_conncb *ctx)
{
  struct ldap_cnx *ldap_cnx = (struct ldap_cnx *) ctx->lc_arg;
  napi_status status;

  napi_env env = ldap_cnx->env;
  napi_handle_scope scope;
  napi_value js_cb, this;

  // For whatever reason this function seems to get called twice on disconnect.
  // Therefore we'll assume that if the event looop is not running then this
  // function has already been called.
  if (ldap_cnx->handle && !uv_loop_alive (ldap_cnx->handle->loop))
    return;

  if (ldap_cnx->handle)
    uv_poll_stop (ldap_cnx->handle);

If i where to apply the same fix to jeremy childs code I guess it would look like this

void LDAPCnx::OnDisconnect(LDAP *ld, Sockbuf *sb,
                      struct ldap_conncb *ctx) {
  // this fires when the connection closes
  LDAPCnx * lc = (LDAPCnx *)ctx->lc_arg;

  // For whatever reason this function seems to get called twice on disconnect.
  // Therefore we'll assume that if the event looop is not running then this
  // function has already been called.
  if (lc->handle && !uv_loop_alive (lc->handle->loop))
    return;

  if (lc->handle) {
    uv_poll_stop(lc->handle);
  }
  lc->disconnect_callback->Call(0, NULL);
}

As a general comment. This variable

    lc->handle = new uv_poll_t;

is allocated on the heap but i can't see where it get freed. Ideally you'd free it before setting it to null and also free it once the class gets destroyed.

I noticed my fix doesn't call the callback when disconnect gets fired for the second time but yours does. I'm not sure what the correct behaviour is here.

jjhoughton avatar Aug 16 '20 12:08 jjhoughton