Move parse frame array to the Map object #3560

tenderlove · 2017-08-24T05:04:37Z

This makes the frame stack per-parser, and per-thread. Fixes #3250

grpc-kokoro · 2017-08-24T05:04:38Z

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

grpc-kokoro · 2017-08-24T05:04:38Z

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

bazel-io · 2017-08-24T05:04:39Z

Can one of the admins verify this patch?

googlebot · 2017-08-24T05:04:41Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

tenderlove · 2017-08-24T05:06:04Z

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

I signed it!

googlebot · 2017-08-24T05:06:06Z

CLAs look good, thanks!

grpc-kokoro · 2017-08-24T23:50:48Z

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

grpc-kokoro · 2017-08-24T23:50:48Z

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

bazel-io · 2017-08-24T23:50:48Z

Can one of the admins verify this patch?

liujisi · 2017-08-24T23:51:05Z

ok to test

tenderlove · 2017-08-28T16:56:28Z

I see that some builds failed, but I can't see the details. Do I need to make sure that all of them are green before this can be applied?

liujisi · 2017-08-28T17:42:34Z

I believe it's just flaky. The tests result look good. @haberman could you take a look at the PR?

haberman · 2017-08-29T19:26:42Z

Thanks very much for the fix! Can you help me understand what could cause concurrent decodes though? I know Ruby can have multiple threads, but I was under the impression that Ruby has a GVL that prevents Ruby code or C extensions from running concurrently (unless you use rb_thread_call_without_gvl): https://github.com/ruby/ruby/blob/trunk/doc/extension.rdoc#threading

tenderlove · 2017-08-29T19:55:59Z

Thanks very much for the fix! Can you help me understand what could cause concurrent decodes though?

Ruby can switch threads when the GC is invoked. Array manipulations with rb_ary_push / rb_ary_pop may cause the GC to be invoked and allow threads to switch. If the array was implemented with a non-Ruby data structure, then the VM would not be given an opportunity to switch threads.

I know Ruby can have multiple threads, but I was under the impression that Ruby has a GVL that prevents Ruby code or C extensions from running concurrently

It sounds like you might be confusing concurrency and parallelism. The GVL prevents parallelism (two Ruby threads running at the same time), but it does not prevent concurrency (two Ruby threads running interleaved).

The bug that we are hitting here is that we have two decodes running interleaved. Interleaving thread execution happened because the calls to rb_ary_* allowed a different thread to be scheduled. These interleaved executions both mutated a shared array, and that leads to a SEGV or "strange values for a key in the ruby object".

I hope this helps. If there's anything I can do to better explain, please let me know!

NOTE: It may not have been the rb_ary_* functions that allowed the thread to switch, they are just the ones that stuck out to me. Many rb_* calls allow threads to switch (especially if they hit the GC or do IO operations).

haberman · 2017-08-30T17:13:11Z

ruby/ext/google/protobuf_c/encode_decode.c

@@ -293,16 +288,12 @@ static map_parse_frame_t* map_push_frame(VALUE map,
  native_slot_init(handlerdata->key_field_type, &frame->key_storage);
  native_slot_init(handlerdata->value_field_type, &frame->value_storage);

-  rb_ary_push(map_parse_frames,
+  Map_push_frame(map,


If you are storing this in the map directly, I don't think it needs to be an array with push and pop. I think it could just be a single value. The array was for when we had a single global stack that needed to be able to handle parsing of nested maps.

Ok, I changed this to store only one value.

haberman · 2017-08-30T17:16:37Z

Got it, thanks for the explanation. I hadn't realized that calls into Ruby APIs could release the GVL; I thought every call into a C extension routine was atomic unless you explicitly released the GVL.

haberman

Implementation looks great. Could you add your test case to our test suite to verify the fix and prevent regressions?

tenderlove · 2017-08-30T18:35:02Z

Implementation looks great. Could you add your test case to our test suite to verify the fix and prevent regressions?

Done. 😊

This makes the frame stack per-parser, and per-thread. Fixes protocolbuffers#3250

haberman

Thanks for the fix!

googlebot added the cla: no label Aug 24, 2017

googlebot added cla: yes and removed cla: no labels Aug 24, 2017

This was referenced Aug 24, 2017

Segfault when ruby-protobuf decodes lots of messages with maps concurrently #3250

Closed

Ruby Segmentation Fault grpc/grpc#11762

Closed

tenderlove force-pushed the thread-safe-map branch from ba09825 to 0a274cc Compare August 24, 2017 15:33

liujisi closed this Aug 24, 2017

liujisi requested a review from haberman August 24, 2017 23:50

liujisi added the kokoro:run label Aug 24, 2017

liujisi reopened this Aug 24, 2017

kokoro-team removed the kokoro:run label Aug 25, 2017

haberman reviewed Aug 30, 2017

View reviewed changes

haberman requested changes Aug 30, 2017

View reviewed changes

tenderlove force-pushed the thread-safe-map branch from b05391b to f5328c6 Compare August 30, 2017 18:34

tenderlove added 2 commits August 30, 2017 11:38

Move parse frame array to the Map object

d6152dd

This makes the frame stack per-parser, and per-thread. Fixes protocolbuffers#3250

Storing the frame on the map means we don't need the array

f55c6ec

tenderlove force-pushed the thread-safe-map branch from f5328c6 to f55c6ec Compare August 30, 2017 18:38

haberman approved these changes Aug 31, 2017

View reviewed changes

haberman merged commit 6699f2c into protocolbuffers:master Aug 31, 2017

This was referenced Sep 13, 2017

Ruby Segmentation Fault #3352

Closed

Another Ruby Segfault #3040

Closed

Ruby Segfault #3039

Closed

[Ruby] Segmentation Fault #2952

Closed

apolcyn mentioned this pull request Jun 7, 2018

Sporadic 'TypeError: wrong argument type Fixnum' in datastore client grpc/grpc#8689

Closed

dentarg mentioned this pull request Jul 17, 2018

Random segfaults with rspec and webmock sporkmonger/addressable#305

Closed

haberman mentioned this pull request Aug 20, 2019

Ruby concurrent map decoding sporadically has empty results #6532

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move parse frame array to the Map object #3560

Move parse frame array to the Map object #3560

tenderlove commented Aug 24, 2017

grpc-kokoro commented Aug 24, 2017

grpc-kokoro commented Aug 24, 2017

bazel-io commented Aug 24, 2017

googlebot commented Aug 24, 2017

tenderlove commented Aug 24, 2017

googlebot commented Aug 24, 2017

grpc-kokoro commented Aug 24, 2017

grpc-kokoro commented Aug 24, 2017

bazel-io commented Aug 24, 2017

liujisi commented Aug 24, 2017

tenderlove commented Aug 28, 2017

liujisi commented Aug 28, 2017

haberman commented Aug 29, 2017

tenderlove commented Aug 29, 2017

haberman Aug 30, 2017

tenderlove Aug 30, 2017

haberman commented Aug 30, 2017

haberman left a comment

tenderlove commented Aug 30, 2017

haberman left a comment

Move parse frame array to the Map object #3560

Move parse frame array to the Map object #3560

Conversation

tenderlove commented Aug 24, 2017

grpc-kokoro commented Aug 24, 2017

grpc-kokoro commented Aug 24, 2017

bazel-io commented Aug 24, 2017

googlebot commented Aug 24, 2017

tenderlove commented Aug 24, 2017

googlebot commented Aug 24, 2017

grpc-kokoro commented Aug 24, 2017

grpc-kokoro commented Aug 24, 2017

bazel-io commented Aug 24, 2017

liujisi commented Aug 24, 2017

tenderlove commented Aug 28, 2017

liujisi commented Aug 28, 2017

haberman commented Aug 29, 2017

tenderlove commented Aug 29, 2017

haberman Aug 30, 2017

Choose a reason for hiding this comment

tenderlove Aug 30, 2017

Choose a reason for hiding this comment

haberman commented Aug 30, 2017

haberman left a comment

Choose a reason for hiding this comment

tenderlove commented Aug 30, 2017

haberman left a comment

Choose a reason for hiding this comment