From cb3b1216332ded5195d509ecad28d691e5beb429 Mon Sep 17 00:00:00 2001 From: Jean Boussier Date: Mon, 13 Mar 2023 13:03:22 +0100 Subject: [PATCH 1/2] Use RUBY_ASSERT when available It should be present from Ruby 2.7 onwards. The advantage over a raw `assert` is that it will trigger MRI's crash report on failure, making it much easier to debug at a glance. For context I'm trying to debug an assertion failure we've been seeing on Ruby nigthly builds: ``` ruby: protobuf.c:404: ObjectCache_Add: Assertion `ObjectCache_Get(key) == val' failed. ``` --- ruby/ext/google/protobuf_c/protobuf.h | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/ruby/ext/google/protobuf_c/protobuf.h b/ruby/ext/google/protobuf_c/protobuf.h index c6af98fa4727..a4e31d157dd6 100644 --- a/ruby/ext/google/protobuf_c/protobuf.h +++ b/ruby/ext/google/protobuf_c/protobuf.h @@ -110,8 +110,12 @@ extern VALUE cTypeError; do { \ } while (false && (expr)) #else +#ifdef RUBY_ASSERT +#define PBRUBY_ASSERT RUBY_ASSERT +#else #define PBRUBY_ASSERT(expr) assert(expr) -#endif +#endif // RUBY_ASSERT +#endif // NDEBUG #define PBRUBY_MAX(x, y) (((x) > (y)) ? (x) : (y)) From e834ad39237dee4fe96c31031077324b75ce1d2d Mon Sep 17 00:00:00 2001 From: Jean Boussier Date: Mon, 13 Mar 2023 23:23:07 +0100 Subject: [PATCH 2/2] Repro for ObjectCache_Get(key) != val" --- ruby/ext/google/protobuf_c/protobuf.c | 3 +++ ruby/ext/google/protobuf_c/repeated_field.c | 2 +- ruby/tests/basic.rb | 13 +++++++++++++ 3 files changed, 17 insertions(+), 1 deletion(-) diff --git a/ruby/ext/google/protobuf_c/protobuf.c b/ruby/ext/google/protobuf_c/protobuf.c index bee8ac288f71..bc318366ad22 100644 --- a/ruby/ext/google/protobuf_c/protobuf.c +++ b/ruby/ext/google/protobuf_c/protobuf.c @@ -403,6 +403,9 @@ void ObjectCache_Add(const void *key, VALUE val) { #if USE_SECONDARY_MAP rb_mutex_unlock(secondary_map_mutex); #endif + if (ObjectCache_Get(key) != val) { + rb_bug("ObjectCache_Get(key) != val"); + } PBRUBY_ASSERT(ObjectCache_Get(key) == val); } diff --git a/ruby/ext/google/protobuf_c/repeated_field.c b/ruby/ext/google/protobuf_c/repeated_field.c index 700ca16f38a6..2be23f4242f3 100644 --- a/ruby/ext/google/protobuf_c/repeated_field.c +++ b/ruby/ext/google/protobuf_c/repeated_field.c @@ -432,7 +432,7 @@ static VALUE RepeatedField_dup(VALUE _self) { */ VALUE RepeatedField_to_ary(VALUE _self) { RepeatedField* self = ruby_to_RepeatedField(_self); - int size = upb_Array_Size(self->array); + int size = self->array ? upb_Array_Size(self->array) : 0; VALUE ary = rb_ary_new2(size); int i; diff --git a/ruby/tests/basic.rb b/ruby/tests/basic.rb index 7351b9cd33e1..22ceab71f190 100755 --- a/ruby/tests/basic.rb +++ b/ruby/tests/basic.rb @@ -467,6 +467,19 @@ def test_map_wrappers_with_no_value run_asserts.call(m4) end + def test_gc_stress + GC.stress = true + o = Outer.new + o.items[0] = Inner.new + raw = Outer.encode(o) + + 10.times do + assert_equal o, Outer.decode(raw) + end + ensure + GC.stress = false + end + def test_concurrent_decoding o = Outer.new o.items[0] = Inner.new