jeremycx / node-LDAP

LDAP binding for node.js
MIT License
221 stars 43 forks source link

Unset handler on disconnect #119

Open pfree opened 4 years ago

pfree commented 4 years ago

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.

jjhoughton commented 4 years ago

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.