From 3eaaf39efffc95327b4ff440fc4a412486f94311 Mon Sep 17 00:00:00 2001 From: murgatroid99 Date: Mon, 14 Mar 2016 11:53:42 -0700 Subject: [PATCH 1/3] Properly mark proc used in call credentials for garbage collection --- src/ruby/ext/grpc/rb_call_credentials.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/ruby/ext/grpc/rb_call_credentials.c b/src/ruby/ext/grpc/rb_call_credentials.c index 2426f106a99..402becf6aac 100644 --- a/src/ruby/ext/grpc/rb_call_credentials.c +++ b/src/ruby/ext/grpc/rb_call_credentials.c @@ -57,6 +57,10 @@ typedef struct grpc_rb_call_credentials { /* Holder of ruby objects involved in contructing the credentials */ VALUE mark; + /* The proc called when getting the credentials. Same pointer as + wrapped->state */ + VALUE proc; + /* The actual credentials */ grpc_call_credentials *wrapped; } grpc_rb_call_credentials; @@ -164,11 +168,11 @@ 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); } + rb_gc_mark(wrapper->proc); } static rb_data_type_t grpc_rb_call_credentials_data_type = { @@ -187,6 +191,7 @@ static rb_data_type_t grpc_rb_call_credentials_data_type = { static VALUE grpc_rb_call_credentials_alloc(VALUE cls) { grpc_rb_call_credentials *wrapper = ALLOC(grpc_rb_call_credentials); wrapper->wrapped = NULL; + wrapper->proc = Qnil; wrapper->mark = Qnil; return TypedData_Wrap_Struct(cls, &grpc_rb_call_credentials_data_type, wrapper); } @@ -267,6 +272,7 @@ static VALUE grpc_rb_call_credentials_init(VALUE self, VALUE proc) { return Qnil; } + wrapper->proc = proc; wrapper->wrapped = creds; rb_ivar_set(self, id_callback, proc); From 05afaa838aad09ca65c8da5108492e63f15ae3f9 Mon Sep 17 00:00:00 2001 From: murgatroid99 Date: Mon, 14 Mar 2016 13:01:00 -0700 Subject: [PATCH 2/3] Add GC marking for composite credentials --- src/ruby/ext/grpc/rb_call_credentials.c | 32 ++++++++-------------- src/ruby/ext/grpc/rb_channel_credentials.c | 23 ++++++++-------- 2 files changed, 23 insertions(+), 32 deletions(-) diff --git a/src/ruby/ext/grpc/rb_call_credentials.c b/src/ruby/ext/grpc/rb_call_credentials.c index 402becf6aac..7c4b69f7f0d 100644 --- a/src/ruby/ext/grpc/rb_call_credentials.c +++ b/src/ruby/ext/grpc/rb_call_credentials.c @@ -50,17 +50,13 @@ * 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; - /* The proc called when getting the credentials. Same pointer as - wrapped->state */ - VALUE proc; - /* The actual credentials */ grpc_call_credentials *wrapped; } grpc_rb_call_credentials; @@ -150,13 +146,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); } @@ -168,11 +159,9 @@ 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); } - rb_gc_mark(wrapper->proc); } static rb_data_type_t grpc_rb_call_credentials_data_type = { @@ -191,7 +180,6 @@ static rb_data_type_t grpc_rb_call_credentials_data_type = { static VALUE grpc_rb_call_credentials_alloc(VALUE cls) { grpc_rb_call_credentials *wrapper = ALLOC(grpc_rb_call_credentials); wrapper->wrapped = NULL; - wrapper->proc = Qnil; wrapper->mark = Qnil; return TypedData_Wrap_Struct(cls, &grpc_rb_call_credentials_data_type, wrapper); } @@ -199,7 +187,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) { @@ -209,6 +197,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; } @@ -272,7 +261,7 @@ static VALUE grpc_rb_call_credentials_init(VALUE self, VALUE proc) { return Qnil; } - wrapper->proc = proc; + wrapper->mark = proc; wrapper->wrapped = creds; rb_ivar_set(self, id_callback, proc); @@ -283,15 +272,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() { diff --git a/src/ruby/ext/grpc/rb_channel_credentials.c b/src/ruby/ext/grpc/rb_channel_credentials.c index 8c6fc3b7eb6..f6490843113 100644 --- a/src/ruby/ext/grpc/rb_channel_credentials.c +++ b/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() { From 8c9edc2a57badc215f0266ae770a973416ad4c62 Mon Sep 17 00:00:00 2001 From: murgatroid99 Date: Mon, 14 Mar 2016 15:51:56 -0700 Subject: [PATCH 3/3] Make channels and calls properly mark references to their credentials --- src/ruby/ext/grpc/rb_call.c | 6 ++++++ src/ruby/ext/grpc/rb_channel.c | 22 ++++++++-------------- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/src/ruby/ext/grpc/rb_call.c b/src/ruby/ext/grpc/rb_call.c index af05ddf6e78..cd0aa6aaf20 100644 --- a/src/ruby/ext/grpc/rb_call.c +++ b/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")); diff --git a/src/ruby/ext/grpc/rb_channel.c b/src/ruby/ext/grpc/rb_channel.c index 0e6badbdaf0..e1aaa539db3 100644 --- a/src/ruby/ext/grpc/rb_channel.c +++ b/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;