Merge pull request #5744 from murgatroid99/ruby_gc_mark_credentials_callback

Properly mark proc used in call credentials for garbage collection

src/ruby/ext/grpc/rb_call.c

@@ -72,6 +72,10 @@ static ID id_cq;
  * the flags used to create metadata from a Hash */
 static ID id_flags;
 
+/* id_credentials is the name of the hidden ivar that preserves the value
+ * of the credentials added to the call */
+static ID id_credentials;
+
 /* id_input_md is the name of the hidden ivar that preserves the hash used to
  * create metadata, so that references to the strings it contains last as long
  * as the call the metadata is added to. */
@@ -299,6 +303,7 @@ static VALUE grpc_rb_call_set_credentials(VALUE self, VALUE credentials) {
              "grpc_call_set_credentials failed with %s (code=%d)",
              grpc_call_error_detail_of(err), err);
   }
+  rb_ivar_set(self, id_credentials, credentials);
   return Qnil;
 }
 
@@ -859,6 +864,7 @@ void Init_grpc_call() {
   id_cq = rb_intern("__cq");
   id_flags = rb_intern("__flags");
   id_input_md = rb_intern("__input_md");
+  id_credentials = rb_intern("__credentials");
 
   /* Ids used in constructing the batch result. */
   sym_send_message = ID2SYM(rb_intern("send_message"));

src/ruby/ext/grpc/rb_call_credentials.c

@@ -51,9 +51,9 @@
  * grpc_call_credentials */
 static VALUE grpc_rb_cCallCredentials = Qnil;
 
-/* grpc_rb_call_credentials wraps a grpc_call_credentials. It provides a peer
- * ruby object, 'mark' to minimize copying when a credential is created from
- * ruby. */
+/* grpc_rb_call_credentials wraps a grpc_call_credentials. It provides a mark
+ * object that is used to hold references to any objects used to create the
+ * credentials. */
 typedef struct grpc_rb_call_credentials {
   /* Holder of ruby objects involved in contructing the credentials */
   VALUE mark;
@@ -156,13 +156,8 @@ static void grpc_rb_call_credentials_free(void *p) {
     return;
   }
   wrapper = (grpc_rb_call_credentials *)p;
-
-  /* Delete the wrapped object if the mark object is Qnil, which indicates that
-   * no other object is the actual owner. */
-  if (wrapper->wrapped != NULL && wrapper->mark == Qnil) {
-    grpc_call_credentials_release(wrapper->wrapped);
-    wrapper->wrapped = NULL;
-  }
+  grpc_call_credentials_release(wrapper->wrapped);
+  wrapper->wrapped = NULL;
 
   xfree(p);
 }
@@ -174,8 +169,6 @@ static void grpc_rb_call_credentials_mark(void *p) {
     return;
   }
   wrapper = (grpc_rb_call_credentials *)p;
-
-  /* If it's not already cleaned up, mark the mark object */
   if (wrapper->mark != Qnil) {
     rb_gc_mark(wrapper->mark);
   }
@@ -204,7 +197,7 @@ static VALUE grpc_rb_call_credentials_alloc(VALUE cls) {
 /* Creates a wrapping object for a given call credentials. This should only be
  * called with grpc_call_credentials objects that are not already associated
  * with any Ruby object */
-VALUE grpc_rb_wrap_call_credentials(grpc_call_credentials *c) {
+VALUE grpc_rb_wrap_call_credentials(grpc_call_credentials *c, VALUE mark) {
   VALUE rb_wrapper;
   grpc_rb_call_credentials *wrapper;
   if (c == NULL) {
@@ -214,6 +207,7 @@ VALUE grpc_rb_wrap_call_credentials(grpc_call_credentials *c) {
   TypedData_Get_Struct(rb_wrapper, grpc_rb_call_credentials,
                        &grpc_rb_call_credentials_data_type, wrapper);
   wrapper->wrapped = c;
+  wrapper->mark = mark;
   return rb_wrapper;
 }
 
@@ -277,6 +271,7 @@ static VALUE grpc_rb_call_credentials_init(VALUE self, VALUE proc) {
     return Qnil;
   }
 
+  wrapper->mark = proc;
   wrapper->wrapped = creds;
   rb_ivar_set(self, id_callback, proc);
 
@@ -287,15 +282,18 @@ static VALUE grpc_rb_call_credentials_compose(int argc, VALUE *argv,
                                               VALUE self) {
   grpc_call_credentials *creds;
   grpc_call_credentials *other;
+  VALUE mark;
   if (argc == 0) {
     return self;
   }
+  mark = rb_ary_new();
   creds = grpc_rb_get_wrapped_call_credentials(self);
   for (int i = 0; i < argc; i++) {
+    rb_ary_push(mark, argv[i]);
     other = grpc_rb_get_wrapped_call_credentials(argv[i]);
     creds = grpc_composite_call_credentials_create(creds, other, NULL);
   }
-  return grpc_rb_wrap_call_credentials(creds);
+  return grpc_rb_wrap_call_credentials(creds, mark);
 }
 
 void Init_grpc_call_credentials() {

src/ruby/ext/grpc/rb_channel.c

@@ -70,11 +70,10 @@ static VALUE grpc_rb_cChannel = Qnil;
 /* Used during the conversion of a hash to channel args during channel setup */
 static VALUE grpc_rb_cChannelArgs;
 
-/* grpc_rb_channel wraps a grpc_channel.  It provides a peer ruby object,
- * 'mark' to minimize copying when a channel is created from ruby. */
+/* grpc_rb_channel wraps a grpc_channel. */
 typedef struct grpc_rb_channel {
-  /* Holder of ruby objects involved in constructing the channel */
-  VALUE mark;
+  VALUE credentials;
+
   /* The actual channel */
   grpc_channel *wrapped;
 } grpc_rb_channel;
@@ -87,13 +86,8 @@ static void grpc_rb_channel_free(void *p) {
   };
   ch = (grpc_rb_channel *)p;
 
-  /* Deletes the wrapped object if the mark object is Qnil, which indicates
-   * that no other object is the actual owner. */
-  if (ch->wrapped != NULL && ch->mark == Qnil) {
+  if (ch->wrapped != NULL) {
     grpc_channel_destroy(ch->wrapped);
-    rb_warning("channel gc: destroyed the c channel");
-  } else {
-    rb_warning("channel gc: did not destroy the c channel");
   }
 
   xfree(p);
@@ -106,8 +100,8 @@ static void grpc_rb_channel_mark(void *p) {
     return;
   }
   channel = (grpc_rb_channel *)p;
-  if (channel->mark != Qnil) {
-    rb_gc_mark(channel->mark);
+  if (channel->credentials != Qnil) {
+    rb_gc_mark(channel->credentials);
   }
 }
 
@@ -125,7 +119,7 @@ static rb_data_type_t grpc_channel_data_type = {
 static VALUE grpc_rb_channel_alloc(VALUE cls) {
   grpc_rb_channel *wrapper = ALLOC(grpc_rb_channel);
   wrapper->wrapped = NULL;
-  wrapper->mark = Qnil;
+  wrapper->credentials = Qnil;
   return TypedData_Wrap_Struct(cls, &grpc_channel_data_type, wrapper);
 }
 
@@ -162,6 +156,7 @@ static VALUE grpc_rb_channel_init(int argc, VALUE *argv, VALUE self) {
     }
     ch = grpc_insecure_channel_create(target_chars, &args, NULL);
   } else {
+    wrapper->credentials = credentials;
     creds = grpc_rb_get_wrapped_channel_credentials(credentials);
     ch = grpc_secure_channel_create(creds, target_chars, &args, NULL);
   }
@@ -330,7 +325,6 @@ static VALUE grpc_rb_channel_destroy(VALUE self) {
   if (ch != NULL) {
     grpc_channel_destroy(ch);
     wrapper->wrapped = NULL;
-    wrapper->mark = Qnil;
   }
 
   return Qnil;

src/ruby/ext/grpc/rb_channel_credentials.c

@@ -49,8 +49,8 @@
 static VALUE grpc_rb_cChannelCredentials = Qnil;
 
 /* grpc_rb_channel_credentials wraps a grpc_channel_credentials.  It provides a
- * peer ruby object, 'mark' to minimize copying when a credential is
- * created from ruby. */
+ * mark object that is used to hold references to any objects used to create
+ * the credentials. */
 typedef struct grpc_rb_channel_credentials {
   /* Holder of ruby objects involved in constructing the credentials */
   VALUE mark;
@@ -66,13 +66,8 @@ static void grpc_rb_channel_credentials_free(void *p) {
     return;
   };
   wrapper = (grpc_rb_channel_credentials *)p;
-
-  /* Delete the wrapped object if the mark object is Qnil, which indicates that
-   * no other object is the actual owner. */
-  if (wrapper->wrapped != NULL && wrapper->mark == Qnil) {
-    grpc_channel_credentials_release(wrapper->wrapped);
-    wrapper->wrapped = NULL;
-  }
+  grpc_channel_credentials_release(wrapper->wrapped);
+  wrapper->wrapped = NULL;
 
   xfree(p);
 }
@@ -85,7 +80,6 @@ static void grpc_rb_channel_credentials_mark(void *p) {
   }
   wrapper = (grpc_rb_channel_credentials *)p;
 
-  /* If it's not already cleaned up, mark the mark object */
   if (wrapper->mark != Qnil) {
     rb_gc_mark(wrapper->mark);
   }
@@ -114,7 +108,7 @@ static VALUE grpc_rb_channel_credentials_alloc(VALUE cls) {
 /* Creates a wrapping object for a given channel credentials. This should only
  * be called with grpc_channel_credentials objects that are not already
  * associated with any Ruby object. */
-VALUE grpc_rb_wrap_channel_credentials(grpc_channel_credentials *c) {
+VALUE grpc_rb_wrap_channel_credentials(grpc_channel_credentials *c, VALUE mark) {
   VALUE rb_wrapper;
   grpc_rb_channel_credentials *wrapper;
   if (c == NULL) {
@@ -124,6 +118,7 @@ VALUE grpc_rb_wrap_channel_credentials(grpc_channel_credentials *c) {
   TypedData_Get_Struct(rb_wrapper, grpc_rb_channel_credentials,
                        &grpc_rb_channel_credentials_data_type, wrapper);
   wrapper->wrapped = c;
+  wrapper->mark = mark;
   return rb_wrapper;
 }
 
@@ -222,11 +217,15 @@ static VALUE grpc_rb_channel_credentials_compose(int argc, VALUE *argv,
                                                  VALUE self) {
   grpc_channel_credentials *creds;
   grpc_call_credentials *other;
+  VALUE mark;
   if (argc == 0) {
     return self;
   }
+  mark = rb_ary_new();
+  rb_ary_push(mark, self);
   creds = grpc_rb_get_wrapped_channel_credentials(self);
   for (int i = 0; i < argc; i++) {
+    rb_ary_push(mark, argv[i]);
     other = grpc_rb_get_wrapped_call_credentials(argv[i]);
     creds = grpc_composite_channel_credentials_create(creds, other, NULL);
     if (creds == NULL) {
@@ -234,7 +233,7 @@ static VALUE grpc_rb_channel_credentials_compose(int argc, VALUE *argv,
                "Failed to compose channel and call credentials");
     }
   }
-  return grpc_rb_wrap_channel_credentials(creds);
+  return grpc_rb_wrap_channel_credentials(creds, mark);
 }
 
 void Init_grpc_channel_credentials() {