Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Use RUBY_ASSERT when available #12216

Closed
wants to merge 2 commits into from
+22 −2
Closed

Use RUBY_ASSERT when available #12216

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cb3b121
Use RUBY_ASSERT when available
byroot Mar 13, 2023
e834ad3
Repro for ObjectCache_Get(key) != val"
byroot Mar 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions ruby/ext/google/protobuf_c/protobuf.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down
6 changes: 5 additions & 1 deletion ruby/ext/google/protobuf_c/protobuf.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))

Expand Down
2 changes: 1 addition & 1 deletion ruby/ext/google/protobuf_c/repeated_field.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This I'll submit individually, with similar fixes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you get a chance to submit a separate PR for this? I didn't see one.

What was the root cause that led you to this fix? I ask because it looks very similar to what I've been debugging in #11968

There is a similar symptom; self->array ends up being NULL. But in that case it's happening in RepeatedField_each. I've isolated it to a race condition involving the object cache.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hum, I was working on it at some point but never submitted it. I think at some point I assumed this case wasn't supposed to happen and was just a fallout of another bug, but I might have been wrong not sure.

I can submit a PR if you want.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this case is supposed to happen, and I do think it is the fallout of another bug. I think this is probably not the right fix.

But I am curious what triggered this bug for you. I'm wondering if it's a similar root cause to #11968 (a race condition on the object cache) or something else.

VALUE ary = rb_ary_new2(size);
int i;

Expand Down
13 changes: 13 additions & 0 deletions ruby/tests/basic.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down