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

Unset handler on disconnect #119

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

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.

None yet

2 participants