Skip to content

Conversation

@pfree
Copy link

@pfree pfree commented Jun 16, 2020

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.

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
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants