ruby: register grpc_rb_sStatus as a global variable#36125
Closed
casperisfine wants to merge 1 commit intogrpc:masterfrom
Closed
ruby: register grpc_rb_sStatus as a global variable#36125casperisfine wants to merge 1 commit intogrpc:masterfrom
casperisfine wants to merge 1 commit intogrpc:masterfrom
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.
Contributor
Author
|
NB: I grepped to see if some others Strucs may be impacted, but didn't find any other in this case. |
apolcyn
approved these changes
Mar 14, 2024
Contributor
apolcyn
left a comment
There was a problem hiding this comment.
Thanks for the fix! one question
| 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); |
Contributor
There was a problem hiding this comment.
does grpc_rb_sNewServerRpc also need this?
Also wondering about grpc_rb_sBatchResult in rb_call.c.
Contributor
Author
There was a problem hiding this comment.
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.
Contributor
Author
There was a problem hiding this comment.
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.
Contributor
Author
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. |
apolcyn
approved these changes
Mar 15, 2024
eugeneo
pushed a commit
to eugeneo/grpc
that referenced
this pull request
Mar 18, 2024
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. |
apolcyn
pushed a commit
to apolcyn/grpc
that referenced
this pull request
May 2, 2024
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
apolcyn
pushed a commit
to apolcyn/grpc
that referenced
this pull request
May 21, 2024
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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_classetc.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::Statusis moved around by the GC.cc @apolcyn @peterzhu2118 @k0kubun