From b21152269dc531a68191dd143084f8f702b5a25c Mon Sep 17 00:00:00 2001 From: apolcyn Date: Wed, 12 Jul 2023 15:50:33 -0700 Subject: [PATCH] [ruby] simplify shutdown; remove unnecessary attempts at grpc_shutdown (#33674) So far, we've structured global C-core init/shutdown as follows: 1) every grpc-ruby object calls `grpc_init` when allocated, and `grpc_shutdown` when finalized 2) grpc-ruby background threads are each wrapped in a `grpc_init/shutdown` pair - for example see https://github.com/grpc/grpc/blob/b32d94de05550ed15f4394b9e4b33bee733dfd89/src/ruby/ext/grpc/rb_event_thread.c#L122 and https://github.com/grpc/grpc/blob/b32d94de05550ed15f4394b9e4b33bee733dfd89/src/ruby/ext/grpc/rb_event_thread.c#L136 But because ruby doesn't join ruby threads when the process is terminating, the `init/shutdown` pairs in 2) are always left open. I.e., thus far we have never been invoking the final call to `grpc_shutdown` which actually does C-core global shutdown. Thus our calls to `grpc_shutdown` are useless. So we might as well keep things simple and not even attempt to call `grpc_shutdown`. Now we just call `grpc_init` before using C-core, and then never even attempt global shutdown (in non-forking situations). As a bonus, this fixes the issue with the event thread's `grpc_ruby_init` racing with `prefork` that is mentioned in https://github.com/grpc/grpc/pull/33666 --- src/ruby/ext/grpc/rb_call_credentials.c | 1 - src/ruby/ext/grpc/rb_channel.c | 7 +------ src/ruby/ext/grpc/rb_channel_credentials.c | 1 - src/ruby/ext/grpc/rb_compression_options.c | 1 - src/ruby/ext/grpc/rb_event_thread.c | 2 -- src/ruby/ext/grpc/rb_grpc.c | 17 ----------------- src/ruby/ext/grpc/rb_grpc.h | 2 -- src/ruby/ext/grpc/rb_server.c | 5 +---- src/ruby/ext/grpc/rb_server_credentials.c | 1 - src/ruby/ext/grpc/rb_xds_channel_credentials.c | 1 - src/ruby/ext/grpc/rb_xds_server_credentials.c | 1 - 11 files changed, 2 insertions(+), 37 deletions(-) diff --git a/src/ruby/ext/grpc/rb_call_credentials.c b/src/ruby/ext/grpc/rb_call_credentials.c index 5f305fa2e12..4d4420cbac4 100644 --- a/src/ruby/ext/grpc/rb_call_credentials.c +++ b/src/ruby/ext/grpc/rb_call_credentials.c @@ -193,7 +193,6 @@ static void grpc_rb_call_credentials_free_internal(void* p) { /* Destroys the credentials instances. */ static void grpc_rb_call_credentials_free(void* p) { grpc_rb_call_credentials_free_internal(p); - grpc_ruby_shutdown(); } /* Protects the mark object from GC */ diff --git a/src/ruby/ext/grpc/rb_channel.c b/src/ruby/ext/grpc/rb_channel.c index 3b659bbf345..aa8fb8a6f42 100644 --- a/src/ruby/ext/grpc/rb_channel.c +++ b/src/ruby/ext/grpc/rb_channel.c @@ -169,10 +169,7 @@ static void grpc_rb_channel_free_internal(void* p) { } /* Destroys Channel instances. */ -static void grpc_rb_channel_free(void* p) { - grpc_rb_channel_free_internal(p); - grpc_ruby_shutdown(); -} +static void grpc_rb_channel_free(void* p) { grpc_rb_channel_free_internal(p); } /* Protects the mark object from GC */ static void grpc_rb_channel_mark(void* p) { @@ -759,10 +756,8 @@ static VALUE run_poll_channels_loop(VALUE arg) { gpr_log( GPR_DEBUG, "GRPC_RUBY: run_poll_channels_loop - create connection polling thread"); - grpc_ruby_init(); rb_thread_call_without_gvl(run_poll_channels_loop_no_gil, NULL, run_poll_channels_loop_unblocking_func, NULL); - grpc_ruby_shutdown(); return Qnil; } diff --git a/src/ruby/ext/grpc/rb_channel_credentials.c b/src/ruby/ext/grpc/rb_channel_credentials.c index 368ad1440e0..8382fb1f904 100644 --- a/src/ruby/ext/grpc/rb_channel_credentials.c +++ b/src/ruby/ext/grpc/rb_channel_credentials.c @@ -63,7 +63,6 @@ static void grpc_rb_channel_credentials_free_internal(void* p) { /* Destroys the credentials instances. */ static void grpc_rb_channel_credentials_free(void* p) { grpc_rb_channel_credentials_free_internal(p); - grpc_ruby_shutdown(); } /* Protects the mark object from GC */ diff --git a/src/ruby/ext/grpc/rb_compression_options.c b/src/ruby/ext/grpc/rb_compression_options.c index 28d6d308616..155c5dafe37 100644 --- a/src/ruby/ext/grpc/rb_compression_options.c +++ b/src/ruby/ext/grpc/rb_compression_options.c @@ -70,7 +70,6 @@ static void grpc_rb_compression_options_free_internal(void* p) { * wrapped grpc compression options. */ static void grpc_rb_compression_options_free(void* p) { grpc_rb_compression_options_free_internal(p); - grpc_ruby_shutdown(); } /* Ruby recognized data type for the CompressionOptions class. */ diff --git a/src/ruby/ext/grpc/rb_event_thread.c b/src/ruby/ext/grpc/rb_event_thread.c index eaba8ff67e6..49ac61c52f0 100644 --- a/src/ruby/ext/grpc/rb_event_thread.c +++ b/src/ruby/ext/grpc/rb_event_thread.c @@ -119,7 +119,6 @@ static void grpc_rb_event_unblocking_func(void* arg) { static VALUE grpc_rb_event_thread(VALUE arg) { grpc_rb_event* event; (void)arg; - grpc_ruby_init(); while (true) { event = (grpc_rb_event*)rb_thread_call_without_gvl( grpc_rb_wait_for_event_no_gil, NULL, grpc_rb_event_unblocking_func, @@ -133,7 +132,6 @@ static VALUE grpc_rb_event_thread(VALUE arg) { } } grpc_rb_event_queue_destroy(); - grpc_ruby_shutdown(); return Qnil; } diff --git a/src/ruby/ext/grpc/rb_grpc.c b/src/ruby/ext/grpc/rb_grpc.c index df95918f0d0..e6b4df18b79 100644 --- a/src/ruby/ext/grpc/rb_grpc.c +++ b/src/ruby/ext/grpc/rb_grpc.c @@ -355,23 +355,6 @@ void grpc_ruby_init() { g_enable_fork_support, g_grpc_ruby_init_count++); } -void grpc_ruby_shutdown() { - GPR_ASSERT(g_grpc_ruby_init_count > 0); - // Avoid calling grpc_shutdown if we've enabled forking, in order to - // avoid the hypothetical case where grpc_shutdown spawns a detached thread - // that remains alive at fork, potentially causing undefined behavior in - // the child process. - if (g_enable_fork_support) return; - // TODO(apolcyn): call, or don't call, grpc_shutdown() unconditionally - // at this point? So far we've been skipping when forking without fork - // support enabled. - if (grpc_ruby_initial_pid()) grpc_shutdown(); - gpr_log( - GPR_DEBUG, - "GRPC_RUBY: grpc_ruby_shutdown - prev g_grpc_ruby_init_count:%" PRId64, - g_grpc_ruby_init_count--); -} - // fork APIs, useable on linux with env var: GRPC_ENABLE_FORK_SUPPORT=1 // // Must be called once and only once before forking. Must be called on the diff --git a/src/ruby/ext/grpc/rb_grpc.h b/src/ruby/ext/grpc/rb_grpc.h index db509278664..ea5f7196474 100644 --- a/src/ruby/ext/grpc/rb_grpc.h +++ b/src/ruby/ext/grpc/rb_grpc.h @@ -80,6 +80,4 @@ void grpc_rb_fork_unsafe_end(); void grpc_ruby_init(); -void grpc_ruby_shutdown(); - #endif /* GRPC_RB_H_ */ diff --git a/src/ruby/ext/grpc/rb_server.c b/src/ruby/ext/grpc/rb_server.c index 3e5eccd04ac..387dcff2d27 100644 --- a/src/ruby/ext/grpc/rb_server.c +++ b/src/ruby/ext/grpc/rb_server.c @@ -106,10 +106,7 @@ static void grpc_rb_server_free_internal(void* p) { } /* Destroys server instances. */ -static void grpc_rb_server_free(void* p) { - grpc_rb_server_free_internal(p); - grpc_ruby_shutdown(); -} +static void grpc_rb_server_free(void* p) { grpc_rb_server_free_internal(p); } static const rb_data_type_t grpc_rb_server_data_type = { "grpc_server", diff --git a/src/ruby/ext/grpc/rb_server_credentials.c b/src/ruby/ext/grpc/rb_server_credentials.c index ccd8a28d23d..42c259015cd 100644 --- a/src/ruby/ext/grpc/rb_server_credentials.c +++ b/src/ruby/ext/grpc/rb_server_credentials.c @@ -62,7 +62,6 @@ static void grpc_rb_server_credentials_free_internal(void* p) { /* Destroys the server credentials instances. */ static void grpc_rb_server_credentials_free(void* p) { grpc_rb_server_credentials_free_internal(p); - grpc_ruby_shutdown(); } /* Protects the mark object from GC */ diff --git a/src/ruby/ext/grpc/rb_xds_channel_credentials.c b/src/ruby/ext/grpc/rb_xds_channel_credentials.c index 71f94afc86c..db3d6dbdac4 100644 --- a/src/ruby/ext/grpc/rb_xds_channel_credentials.c +++ b/src/ruby/ext/grpc/rb_xds_channel_credentials.c @@ -62,7 +62,6 @@ static void grpc_rb_xds_channel_credentials_free_internal(void* p) { /* Destroys the credentials instances. */ static void grpc_rb_xds_channel_credentials_free(void* p) { grpc_rb_xds_channel_credentials_free_internal(p); - grpc_ruby_shutdown(); } /* Protects the mark object from GC */ diff --git a/src/ruby/ext/grpc/rb_xds_server_credentials.c b/src/ruby/ext/grpc/rb_xds_server_credentials.c index 4d877d15473..3d4ec5afe53 100644 --- a/src/ruby/ext/grpc/rb_xds_server_credentials.c +++ b/src/ruby/ext/grpc/rb_xds_server_credentials.c @@ -63,7 +63,6 @@ static void grpc_rb_xds_server_credentials_free_internal(void* p) { /* Destroys the server credentials instances. */ static void grpc_rb_xds_server_credentials_free(void* p) { grpc_rb_xds_server_credentials_free_internal(p); - grpc_ruby_shutdown(); } /* Protects the mark object from GC */