From cb36e9f95874fc12516a87521abeca716d4f55a8 Mon Sep 17 00:00:00 2001
From: Craig Tiller <ctiller@google.com>
Date: Fri, 29 May 2015 11:20:21 -0700
Subject: [PATCH] Fix memory leak, add debug
---
src/core/channel/client_setup.c | 39 ++++++++++++++++--------
src/core/channel/client_setup.h | 6 ++--
src/core/surface/channel_create.c | 8 ++---
src/core/surface/secure_channel_create.c | 8 ++---
4 files changed, 37 insertions(+), 24 deletions(-)
diff --git a/src/core/channel/client_setup.c b/src/core/channel/client_setup.c
index df6d0b6ece..edad1f0dac 100644
--- a/src/core/channel/client_setup.c
+++ b/src/core/channel/client_setup.c
@@ -93,6 +93,8 @@ static void setup_initiate(grpc_transport_setup *sp) {
grpc_client_setup_request *r = gpr_malloc(sizeof(grpc_client_setup_request));
int in_alarm = 0;
+ gpr_log(GPR_DEBUG, "setup_initiate: %p", r);
+
r->setup = s;
grpc_pollset_set_init(&r->interested_parties);
/* TODO(klempner): Actually set a deadline */
@@ -168,6 +170,7 @@ static void setup_cancel(grpc_transport_setup *sp) {
if (s->in_alarm) {
cancel_alarm = 1;
}
+ gpr_log(GPR_DEBUG, "%p setup_cancel: refs=%d", s, s->refs);
if (--s->refs == 0) {
gpr_mu_unlock(&s->mu);
destroy_setup(s);
@@ -179,7 +182,8 @@ static void setup_cancel(grpc_transport_setup *sp) {
}
}
-int grpc_client_setup_cb_begin(grpc_client_setup_request *r) {
+int grpc_client_setup_cb_begin(grpc_client_setup_request *r, const char *reason) {
+ gpr_log(GPR_DEBUG, "setup_cb_begin: %p %s", r, reason);
gpr_mu_lock(&r->setup->mu);
if (r->setup->cancelled) {
gpr_mu_unlock(&r->setup->mu);
@@ -190,7 +194,8 @@ int grpc_client_setup_cb_begin(grpc_client_setup_request *r) {
return 1;
}
-void grpc_client_setup_cb_end(grpc_client_setup_request *r) {
+void grpc_client_setup_cb_end(grpc_client_setup_request *r, const char *reason) {
+ gpr_log(GPR_DEBUG, "setup_cb_end: %p %s", r, reason);
gpr_mu_lock(&r->setup->mu);
r->setup->in_cb--;
if (r->setup->cancelled) gpr_cv_signal(&r->setup->cv);
@@ -209,6 +214,8 @@ void grpc_client_setup_create_and_attach(
void (*done)(void *user_data), void *user_data) {
grpc_client_setup *s = gpr_malloc(sizeof(grpc_client_setup));
+ gpr_log(GPR_DEBUG, "%p setup_create", s);
+
s->base.vtable = &setup_vtable;
gpr_mu_init(&s->mu);
gpr_cv_init(&s->cv);
@@ -227,14 +234,16 @@ void grpc_client_setup_create_and_attach(
grpc_client_channel_set_transport_setup(newly_minted_channel, &s->base);
}
-int grpc_client_setup_request_should_continue(grpc_client_setup_request *r) {
+int grpc_client_setup_request_should_continue(grpc_client_setup_request *r, const char *reason) {
int result;
if (gpr_time_cmp(gpr_now(), r->deadline) > 0) {
- return 0;
+ result = 0;
+ } else {
+ gpr_mu_lock(&r->setup->mu);
+ result = r->setup->active_request == r;
+ gpr_mu_unlock(&r->setup->mu);
}
- gpr_mu_lock(&r->setup->mu);
- result = r->setup->active_request == r;
- gpr_mu_unlock(&r->setup->mu);
+ gpr_log(GPR_DEBUG, "should_continue: %p result=%d %s", r, result, reason);
return result;
}
@@ -266,20 +275,22 @@ void grpc_client_setup_request_finish(grpc_client_setup_request *r,
int retry = !was_successful;
grpc_client_setup *s = r->setup;
+ gpr_log(GPR_DEBUG, "setup_request_finish: %p success=%d retry0=%d", r, was_successful, retry);
+
gpr_mu_lock(&s->mu);
if (s->active_request == r) {
s->active_request = NULL;
} else {
retry = 0;
}
+
+ gpr_log(GPR_DEBUG, "retry=%d", retry);
+
if (!retry && 0 == --s->refs) {
gpr_mu_unlock(&s->mu);
destroy_setup(s);
destroy_request(r);
- return;
- }
-
- if (retry) {
+ } else if (retry) {
/* TODO(klempner): Replace these values with further consideration. 2x is
probably too aggressive of a backoff. */
gpr_timespec max_backoff = gpr_time_from_minutes(2);
@@ -293,9 +304,11 @@ void grpc_client_setup_request_finish(grpc_client_setup_request *r,
if (gpr_time_cmp(s->current_backoff_interval, max_backoff) > 0) {
s->current_backoff_interval = max_backoff;
}
+ gpr_mu_unlock(&s->mu);
+ } else {
+ gpr_mu_unlock(&s->mu);
+ destroy_request(r);
}
-
- gpr_mu_unlock(&s->mu);
}
const grpc_channel_args *grpc_client_setup_get_channel_args(
diff --git a/src/core/channel/client_setup.h b/src/core/channel/client_setup.h
index 24e7517c83..cbabb510b1 100644
--- a/src/core/channel/client_setup.h
+++ b/src/core/channel/client_setup.h
@@ -52,7 +52,7 @@ void grpc_client_setup_create_and_attach(
/* Check that r is the active request: needs to be performed at each callback.
If this races, we'll have two connection attempts running at once and the
old one will get cleaned up in due course, which is fine. */
-int grpc_client_setup_request_should_continue(grpc_client_setup_request *r);
+int grpc_client_setup_request_should_continue(grpc_client_setup_request *r, const char *reason);
void grpc_client_setup_request_finish(grpc_client_setup_request *r,
int was_successful);
const grpc_channel_args *grpc_client_setup_get_channel_args(
@@ -61,8 +61,8 @@ const grpc_channel_args *grpc_client_setup_get_channel_args(
/* Call before calling back into the setup listener, and call only if
this function returns 1. If it returns 1, also promise to call
grpc_client_setup_cb_end */
-int grpc_client_setup_cb_begin(grpc_client_setup_request *r);
-void grpc_client_setup_cb_end(grpc_client_setup_request *r);
+int grpc_client_setup_cb_begin(grpc_client_setup_request *r, const char *reason);
+void grpc_client_setup_cb_end(grpc_client_setup_request *r, const char *reason);
/* Get the deadline for a request passed in to initiate. Implementations should
make a best effort to honor this deadline. */
diff --git a/src/core/surface/channel_create.c b/src/core/surface/channel_create.c
index b2d3a1923b..061669ec6e 100644
--- a/src/core/surface/channel_create.c
+++ b/src/core/surface/channel_create.c
@@ -90,7 +90,7 @@ static void done(request *r, int was_successful) {
static void on_connect(void *rp, grpc_endpoint *tcp) {
request *r = rp;
- if (!grpc_client_setup_request_should_continue(r->cs_request)) {
+ if (!grpc_client_setup_request_should_continue(r->cs_request, "on_connect")) {
if (tcp) {
grpc_endpoint_shutdown(tcp);
grpc_endpoint_destroy(tcp);
@@ -106,12 +106,12 @@ static void on_connect(void *rp, grpc_endpoint *tcp) {
} else {
return;
}
- } else if (grpc_client_setup_cb_begin(r->cs_request)) {
+ } else if (grpc_client_setup_cb_begin(r->cs_request, "on_connect")) {
grpc_create_chttp2_transport(
r->setup->setup_callback, r->setup->setup_user_data,
grpc_client_setup_get_channel_args(r->cs_request), tcp, NULL, 0,
grpc_client_setup_get_mdctx(r->cs_request), 1);
- grpc_client_setup_cb_end(r->cs_request);
+ grpc_client_setup_cb_end(r->cs_request, "on_connect");
done(r, 1);
return;
} else {
@@ -137,7 +137,7 @@ static void on_resolved(void *rp, grpc_resolved_addresses *resolved) {
request *r = rp;
/* if we're not still the active request, abort */
- if (!grpc_client_setup_request_should_continue(r->cs_request)) {
+ if (!grpc_client_setup_request_should_continue(r->cs_request, "on_resolved")) {
if (resolved) {
grpc_resolved_addresses_destroy(resolved);
}
diff --git a/src/core/surface/secure_channel_create.c b/src/core/surface/secure_channel_create.c
index 73f4d51855..dfc4a1920a 100644
--- a/src/core/surface/secure_channel_create.c
+++ b/src/core/surface/secure_channel_create.c
@@ -96,12 +96,12 @@ static void on_secure_transport_setup_done(void *rp,
if (status != GRPC_SECURITY_OK) {
gpr_log(GPR_ERROR, "Secure transport setup failed with error %d.", status);
done(r, 0);
- } else if (grpc_client_setup_cb_begin(r->cs_request)) {
+ } else if (grpc_client_setup_cb_begin(r->cs_request, "on_secure_transport_setup_done")) {
grpc_create_chttp2_transport(
r->setup->setup_callback, r->setup->setup_user_data,
grpc_client_setup_get_channel_args(r->cs_request), secure_endpoint,
NULL, 0, grpc_client_setup_get_mdctx(r->cs_request), 1);
- grpc_client_setup_cb_end(r->cs_request);
+ grpc_client_setup_cb_end(r->cs_request, "on_secure_transport_setup_done");
done(r, 1);
} else {
done(r, 0);
@@ -112,7 +112,7 @@ static void on_secure_transport_setup_done(void *rp,
static void on_connect(void *rp, grpc_endpoint *tcp) {
request *r = rp;
- if (!grpc_client_setup_request_should_continue(r->cs_request)) {
+ if (!grpc_client_setup_request_should_continue(r->cs_request, "on_connect.secure")) {
if (tcp) {
grpc_endpoint_shutdown(tcp);
grpc_endpoint_destroy(tcp);
@@ -152,7 +152,7 @@ static void on_resolved(void *rp, grpc_resolved_addresses *resolved) {
request *r = rp;
/* if we're not still the active request, abort */
- if (!grpc_client_setup_request_should_continue(r->cs_request)) {
+ if (!grpc_client_setup_request_should_continue(r->cs_request, "on_resolved.secure")) {
if (resolved) {
grpc_resolved_addresses_destroy(resolved);
}
--
GitLab