-
Notifications
You must be signed in to change notification settings - Fork 10.6k
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
ruby: register grpc_rb_sStatus as a global variable #36125
Conversation
Ref: https://bugs.ruby-lang.org/issues/20311 C global variable that contain references to Ruby object MUST be declared to the Ruby GC to prevent these objects from being collected or moved. There are a few exceptions to that, such as classes defined using the C APIs such as `rb_define_class` etc. Up to Ruby 3.4 however, there was a bug that caused classes created from Ruby with the `Struct.new("Name")` API to also be marked as immortal and immovable. GRPC has been relying on this bug, which I fixed in Ruby 3.4, and now GRPC is crashing when `Struct::Status` is moved around by the GC.
NB: I grepped to see if some others Strucs may be impacted, but didn't find any other in this case. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fix! one question
@@ -467,6 +467,7 @@ void Init_grpc_c() { | |||
grpc_rb_mGrpcCore = rb_define_module_under(grpc_rb_mGRPC, "Core"); | |||
grpc_rb_sNewServerRpc = rb_struct_define( | |||
"NewServerRpc", "method", "host", "deadline", "metadata", "call", NULL); | |||
rb_global_variable(&grpc_rb_sStatus); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does grpc_rb_sNewServerRpc
also need this?
Also wondering about grpc_rb_sBatchResult
in rb_call.c
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No because rb_struct_define
, like rb_define_class
all basically all the C API functions that end up creating a class or module, do "root" that object, which means the GC will never collect nor move it.
grpc_rb_sStatus
need it only because that class is defined in Ruby, and grabbed in C via rb_const_get
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An alternative could be to define that struct in C like the other (and Struct.new("Status")
should probably be avoided altogether as it might clash with other gems), but I went for the fix the the minimal amount of changes.
Another quick note. It's currently fixed in 3.4-dev, but the bug is marked as needing backport, so eventually it will make it into point release of 3.3, 3.2 and 3.1. |
Ref: https://bugs.ruby-lang.org/issues/20311 C global variable that contain references to Ruby object MUST be declared to the Ruby GC to prevent these objects from being collected or moved. There are a few exceptions to that, such as classes defined using the C APIs such as `rb_define_class` etc. Up to Ruby 3.4 however, there was a bug that caused classes created from Ruby with the `Struct.new("Name")` API to also be marked as immortal and immovable. GRPC has been relying on this bug, which I fixed in Ruby 3.4, and now GRPC is crashing when `Struct::Status` is moved around by the GC. ``` -- C level backtrace information ------------------------------------------- ruby(rb_print_backtrace+0x14) [0x5577db219d41] ruby-3.4.0/vm_dump.c:820 ruby(rb_vm_bugreport) ruby-3.4.0/vm_dump.c:1151 ruby(rb_bug_for_fatal_signal+0xfc) [0x5577db3cc60c] ruby-3.4.0/error.c:1066 ruby(sigsegv+0x4d) [0x5577db16358d] ruby-3.4.0/signal.c:926 libc.so.6(0x7f5bacd32520) [0x7f5bacd32520] ruby(rb_class_superclass+0x32) [0x5577db0a9152] ruby-3.4.0/object.c:2239 ruby(struct_ivar_get+0x2a) [0x5577db194e02] ruby-3.4.0/struct.c:49 ruby(struct_ivar_get) ruby-3.4.0/struct.c:40 ruby(num_members) ruby-3.4.0/struct.c:705 ruby(rb_struct_new+0x56) [0x5577db19a9d6] ruby-3.4.0/struct.c:848 lib/grpc/grpc_c.so(grpc_run_batch_stack_build_result+0xe6) [0x7f5b84bb6b96] /tmp/bundle/ruby/3.4.0+0/bundler/gems/grpc-5ed33ee673e3/src/ruby/ext/grpc/rb_call.c:780 lib/grpc/grpc_c.so(grpc_rb_call_run_batch_try) /tmp/bundle/ruby/3.4.0+0/bundler/gems/grpc-5ed33ee673e3/src/ruby/ext/grpc/rb_call.c:839 ruby(rb_ensure+0x10c) [0x5577db007d3c] ruby-3.4.0/eval.c:1000 /tmp/bundle/ruby/3.4.0+0/bundler/gems/grpc-5ed33ee673e3/src/ruby/lib/grpc/grpc_c.so(grpc_rb_call_run_batch+0xca) [0x7f5b84bb595a] /tmp/bundle/ruby/3.4.0+0/bundler/gems/grpc-5ed33ee673e3/src/ruby/ext/grpc/rb_call.c:893 ruby(vm_call_cfunc_with_frame_+0x117) [0x5577db1f4c77] ruby-3.4.0/vm_insnhelper.c:3524 ``` cc @apolcyn @peterzhu2118 @k0kubun Closes grpc#36125 COPYBARA_INTEGRATE_REVIEW=grpc#36125 from Shopify:fix-ruby-3.4-compat 7a12759 PiperOrigin-RevId: 616152904
Just want to add that the fix was also backported to (and released in) Ruby 3.3.1. |
Ref: https://bugs.ruby-lang.org/issues/20311 C global variable that contain references to Ruby object MUST be declared to the Ruby GC to prevent these objects from being collected or moved. There are a few exceptions to that, such as classes defined using the C APIs such as `rb_define_class` etc. Up to Ruby 3.4 however, there was a bug that caused classes created from Ruby with the `Struct.new("Name")` API to also be marked as immortal and immovable. GRPC has been relying on this bug, which I fixed in Ruby 3.4, and now GRPC is crashing when `Struct::Status` is moved around by the GC. ``` -- C level backtrace information ------------------------------------------- ruby(rb_print_backtrace+0x14) [0x5577db219d41] ruby-3.4.0/vm_dump.c:820 ruby(rb_vm_bugreport) ruby-3.4.0/vm_dump.c:1151 ruby(rb_bug_for_fatal_signal+0xfc) [0x5577db3cc60c] ruby-3.4.0/error.c:1066 ruby(sigsegv+0x4d) [0x5577db16358d] ruby-3.4.0/signal.c:926 libc.so.6(0x7f5bacd32520) [0x7f5bacd32520] ruby(rb_class_superclass+0x32) [0x5577db0a9152] ruby-3.4.0/object.c:2239 ruby(struct_ivar_get+0x2a) [0x5577db194e02] ruby-3.4.0/struct.c:49 ruby(struct_ivar_get) ruby-3.4.0/struct.c:40 ruby(num_members) ruby-3.4.0/struct.c:705 ruby(rb_struct_new+0x56) [0x5577db19a9d6] ruby-3.4.0/struct.c:848 lib/grpc/grpc_c.so(grpc_run_batch_stack_build_result+0xe6) [0x7f5b84bb6b96] /tmp/bundle/ruby/3.4.0+0/bundler/gems/grpc-5ed33ee673e3/src/ruby/ext/grpc/rb_call.c:780 lib/grpc/grpc_c.so(grpc_rb_call_run_batch_try) /tmp/bundle/ruby/3.4.0+0/bundler/gems/grpc-5ed33ee673e3/src/ruby/ext/grpc/rb_call.c:839 ruby(rb_ensure+0x10c) [0x5577db007d3c] ruby-3.4.0/eval.c:1000 /tmp/bundle/ruby/3.4.0+0/bundler/gems/grpc-5ed33ee673e3/src/ruby/lib/grpc/grpc_c.so(grpc_rb_call_run_batch+0xca) [0x7f5b84bb595a] /tmp/bundle/ruby/3.4.0+0/bundler/gems/grpc-5ed33ee673e3/src/ruby/ext/grpc/rb_call.c:893 ruby(vm_call_cfunc_with_frame_+0x117) [0x5577db1f4c77] ruby-3.4.0/vm_insnhelper.c:3524 ``` cc @apolcyn @peterzhu2118 @k0kubun Closes grpc#36125 COPYBARA_INTEGRATE_REVIEW=grpc#36125 from Shopify:fix-ruby-3.4-compat 7a12759 PiperOrigin-RevId: 616152904
Ref: https://bugs.ruby-lang.org/issues/20311 C global variable that contain references to Ruby object MUST be declared to the Ruby GC to prevent these objects from being collected or moved. There are a few exceptions to that, such as classes defined using the C APIs such as `rb_define_class` etc. Up to Ruby 3.4 however, there was a bug that caused classes created from Ruby with the `Struct.new("Name")` API to also be marked as immortal and immovable. GRPC has been relying on this bug, which I fixed in Ruby 3.4, and now GRPC is crashing when `Struct::Status` is moved around by the GC. ``` -- C level backtrace information ------------------------------------------- ruby(rb_print_backtrace+0x14) [0x5577db219d41] ruby-3.4.0/vm_dump.c:820 ruby(rb_vm_bugreport) ruby-3.4.0/vm_dump.c:1151 ruby(rb_bug_for_fatal_signal+0xfc) [0x5577db3cc60c] ruby-3.4.0/error.c:1066 ruby(sigsegv+0x4d) [0x5577db16358d] ruby-3.4.0/signal.c:926 libc.so.6(0x7f5bacd32520) [0x7f5bacd32520] ruby(rb_class_superclass+0x32) [0x5577db0a9152] ruby-3.4.0/object.c:2239 ruby(struct_ivar_get+0x2a) [0x5577db194e02] ruby-3.4.0/struct.c:49 ruby(struct_ivar_get) ruby-3.4.0/struct.c:40 ruby(num_members) ruby-3.4.0/struct.c:705 ruby(rb_struct_new+0x56) [0x5577db19a9d6] ruby-3.4.0/struct.c:848 lib/grpc/grpc_c.so(grpc_run_batch_stack_build_result+0xe6) [0x7f5b84bb6b96] /tmp/bundle/ruby/3.4.0+0/bundler/gems/grpc-5ed33ee673e3/src/ruby/ext/grpc/rb_call.c:780 lib/grpc/grpc_c.so(grpc_rb_call_run_batch_try) /tmp/bundle/ruby/3.4.0+0/bundler/gems/grpc-5ed33ee673e3/src/ruby/ext/grpc/rb_call.c:839 ruby(rb_ensure+0x10c) [0x5577db007d3c] ruby-3.4.0/eval.c:1000 /tmp/bundle/ruby/3.4.0+0/bundler/gems/grpc-5ed33ee673e3/src/ruby/lib/grpc/grpc_c.so(grpc_rb_call_run_batch+0xca) [0x7f5b84bb595a] /tmp/bundle/ruby/3.4.0+0/bundler/gems/grpc-5ed33ee673e3/src/ruby/ext/grpc/rb_call.c:893 ruby(vm_call_cfunc_with_frame_+0x117) [0x5577db1f4c77] ruby-3.4.0/vm_insnhelper.c:3524 ``` cc @apolcyn @peterzhu2118 Closes grpc#36125 COPYBARA_INTEGRATE_REVIEW=grpc#36125 from Shopify:fix-ruby-3.4-compat 7a12759 PiperOrigin-RevId: 616152904
Ref: https://bugs.ruby-lang.org/issues/20311
C global variable that contain references to Ruby object MUST be declared to the Ruby GC to prevent these objects from being collected or moved.
There are a few exceptions to that, such as classes defined using the C APIs such as
rb_define_class
etc.Up to Ruby 3.4 however, there was a bug that caused classes created from Ruby with the
Struct.new("Name")
API to also be marked as immortal and immovable.GRPC has been relying on this bug, which I fixed in Ruby 3.4, and now GRPC is crashing when
Struct::Status
is moved around by the GC.cc @apolcyn @peterzhu2118 @k0kubun