Fixes refcounting issue by kpayson64 · Pull Request #14315 · grpc/grpc

kpayson64 · 2018-02-05T18:53:22Z

Fixes issue #13327.

De-referencing the stack in the cancellation callback is unsafe. There may be closures on the executor that need the call stack, and cancellation can happen before they are run.

grpc-testing · 2018-02-05T18:56:27Z

****************************************************************

libgrpc.so

     VM SIZE              FILE SIZE
 ++++++++++++++ GROWIN ++++++++++++++

 -------------- SHRINK --------------
  [ = ]       0 [None]     -16  -0.0%

  [ = ]       0 TOTAL      -16  -0.0%


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

grpc-testing · 2018-02-05T19:04:53Z

[trickle] No significant performance differences

grpc-testing · 2018-02-05T19:48:12Z

[microbenchmarks] No significant performance differences

grpc-testing · 2018-02-05T20:12:29Z

****************************************************************

libgrpc.so

     VM SIZE                                                             FILE SIZE
 ++++++++++++++ GROWING                                               ++++++++++++++
  +0.1%     +16 src/core/ext/filters/client_channel/client_channel.cc     +16  +0.1%
       +13%     +11 pick_callback_done_locked                                 +11   +13%
      +1.8%      +5 [Unmapped]                                                 +5  +1.8%

 -------------- SHRINKING                                             --------------
  -0.0%     -32 [None]                                                   -176  -0.0%

  -0.0%     -16 TOTAL                                                    -160  -0.0%


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

grpc-testing · 2018-02-05T20:21:57Z

[trickle] No significant performance differences

dgquintas

Very nice, thanks!

grpc-testing · 2018-02-05T21:54:38Z

Corrupt JSON data (indicates timeout or crash): 
    bm_fullstack_streaming_pump.BM_PumpStreamServerToClient_SockPair__0.opt.old: 1


[microbenchmarks] Performance differences noted:
Benchmark                                                              cpu_time    real_time
---------------------------------------------------------------------  ----------  -----------
BM_StreamingPingPong<MinInProcess, NoOpMutator, NoOpMutator>/262144/2  -4%         -4%
BM_UnaryPingPong<InProcess, NoOpMutator, NoOpMutator>/0/262144         -4%         -4%

kpayson64 · 2018-02-05T22:19:49Z

#14291

markdroth · 2018-02-06T15:43:22Z

Why is this fix necessary? All of the callbacks that this adds refs for run as part of handling a batch sent down from the surface, and the surface maintains a ref to the call stack for each pending batch. The batch can't return until these callbacks return, so there should be no way for the call stack to be unreffed before the batches return. What am I missing here?

Also, can we add a test that triggers the problem that this solves, so that we won't have regressions in the future?

kpayson64 · 2018-02-06T18:32:53Z

@markdroth
Here is a ref count for a call stack when a segfault occurs:

D0205 09:10:30.684904707  110967 transport.cc:41]            CALL_STACK 0x62400000d000:0x62400000c7a0   REF 1->2 completion
D0205 09:10:30.685512302  110968 transport.cc:41]            CALL_STACK 0x62400000d000:0x62400000c7a0   REF 2->3 pick_callback
D0205 09:10:30.691657675  110968 transport.cc:41]            CALL_STACK 0x6290000151f0:0x6290000151e0   REF 1->2 chttp2
D0205 09:10:30.691742888  110968 transport.cc:41]            CALL_STACK 0x6290000151f0:0x6290000151e0   REF 2->3 perform_stream_op
D0205 09:10:30.691767368  110968 transport.cc:55]            CALL_STACK 0x62400000d000:0x62400000c7a0 UNREF 3->2 pick_callback
D0205 09:10:30.691886864  110968 transport.cc:41]            CALL_STACK 0x6290000151f0:0x6290000151e0   REF 3->4 chttp2_writing:become
D0205 09:10:30.691918524  110968 transport.cc:55]            CALL_STACK 0x6290000151f0:0x6290000151e0 UNREF 4->3 perform_stream_op
D0205 09:10:30.692147614  110968 transport.cc:55]            CALL_STACK 0x6290000151f0:0x6290000151e0 UNREF 3->2 chttp2_writing:end
D0205 09:10:30.692281862  110967 transport.cc:41]            CALL_STACK 0x629000010140:0x62900000f8e0   REF 1->2 chttp2
D0205 09:10:30.692363535  110967 transport.cc:41]            CALL_STACK 0x629000010140:0x62900000f8e0   REF 2->3 completion
D0205 09:10:30.692616117  110967 transport.cc:41]            CALL_STACK 0x629000010140:0x62900000f8e0   REF 3->4 perform_stream_op
D0205 09:10:30.692658063  110967 transport.cc:55]            CALL_STACK 0x629000010140:0x62900000f8e0 UNREF 4->3 perform_stream_op
D0205 09:10:30.692773627  110967 transport.cc:55]            CALL_STACK 0x629000010140:0x62900000f8e0 UNREF 3->2 completion
D0205 09:10:30.692982439  110967 transport.cc:41]            CALL_STACK 0x629000010140:0x62900000f8e0   REF 2->3 completion
D0205 09:10:30.693005248  110967 transport.cc:41]            CALL_STACK 0x629000010140:0x62900000f8e0   REF 3->4 perform_stream_op
D0205 09:10:30.693039083  110967 transport.cc:55]            CALL_STACK 0x629000010140:0x62900000f8e0 UNREF 4->3 perform_stream_op
D0205 09:10:30.693121078  110967 transport.cc:41]            CALL_STACK 0x629000010140:0x62900000f8e0   REF 3->4 termination
D0205 09:10:30.693200055  110967 transport.cc:41]            CALL_STACK 0x629000010140:0x62900000f8e0   REF 4->5 perform_stream_op
D0205 09:10:30.693555869  110967 transport.cc:55]            CALL_STACK 0x629000010140:0x62900000f8e0 UNREF 5->4 chttp2
D0205 09:10:30.693602032  110967 transport.cc:55]            CALL_STACK 0x629000010140:0x62900000f8e0 UNREF 4->3 perform_stream_op
D0205 09:10:30.693688864  110967 transport.cc:55]            CALL_STACK 0x629000010140:0x62900000f8e0 UNREF 3->2 termination
D0205 09:10:30.697189638  110967 transport.cc:55]            CALL_STACK 0x629000010140:0x62900000f8e0 UNREF 2->1 completion
D0205 09:10:30.697456418  110967 transport.cc:55]            CALL_STACK 0x6290000151f0:0x6290000151e0 UNREF 2->1 chttp2
D0205 09:10:30.697686494  110967 transport.cc:55]            CALL_STACK 0x62400000d000:0x62400000c7a0 UNREF 2->1 completion
D0205 09:10:30.697728854  110967 transport.cc:55]            CALL_STACK 0x62400000d000:0x62400000c7a0 UNREF 1->0 destroy
D0205 09:10:30.697783569  110967 transport.cc:55]            CALL_STACK 0x6290000151f0:0x6290000151e0 UNREF 1->0 client_channel_destroy_call
D0205 09:10:30.697914649  110967 transport.cc:55]            CALL_STACK 0x629000010140:0x62900000f8e0 UNREF 1->0 destroy
D0205 09:10:30.794493674  110967 transport.cc:41]            CALL_STACK 0x624000007000:0x6240000067a0   REF 1->2 completion
D0205 09:10:30.795295874  110968 transport.cc:41]            CALL_STACK 0x624000007000:0x6240000067a0   REF 2->3 pick_callback
D0205 09:10:30.802089706  110968 transport.cc:41]            CALL_STACK 0x62900001f1f0:0x62900001f1e0   REF 1->2 chttp2
D0205 09:10:30.802170067  110968 transport.cc:55]            CALL_STACK 0x624000007000:0x6240000067a0 UNREF 3->2 pick_callback
D0205 09:10:30.802244215  110968 transport.cc:41]            CALL_STACK 0x624000007000:0x6240000067a0   REF 2->3 termination
D0205 09:10:30.819658456  110967 transport.cc:55]            CALL_STACK 0x624000007000:0x6240000067a0 UNREF 3->2 completion
D0205 09:10:30.819682433  110968 transport.cc:41]            CALL_STACK 0x62900001f1f0:0x62900001f1e0   REF 2->3 perform_stream_op
D0205 09:10:30.819743264  110967 transport.cc:55]            CALL_STACK 0x624000007000:0x6240000067a0 UNREF 2->1 destroy
D0205 09:10:30.820131634  110968 transport.cc:55]            CALL_STACK 0x62900001f1f0:0x62900001f1e0 UNREF 3->2 chttp2
D0205 09:10:30.820159440  110968 transport.cc:55]            CALL_STACK 0x62900001f1f0:0x62900001f1e0 UNREF 2->1 perform_stream_op
D0205 09:10:30.832509457  110968 transport.cc:55]            CALL_STACK 0x624000007000:0x6240000067a0 UNREF 1->0 termination
D0205 09:10:30.832601481  110968 transport.cc:55]            CALL_STACK 0x62900001f1f0:0x62900001f1e0 UNREF 1->0 client_channel_destroy_call

In the second call, we can see the "completion" unref happens before the "perform_stream_op". I suspect batches are getting early-completed on cancellation.

kpayson64 · 2018-02-06T18:43:09Z

I suspect grpc_transport_stream_op_batch_finish_with_failure is at fault, it looks like it queues the batch completion while we could still possibly have closures queued up.

kpayson64 added the disposition/DO NOT MERGE label Feb 5, 2018

kpayson64 requested review from a11r, dgquintas and markdroth as code owners February 5, 2018 18:53

Add refcounts for async callbacks in auth and client filters.

f069dd4

kpayson64 force-pushed the fix_refcounting branch from 20a24d0 to f069dd4 Compare February 5, 2018 20:09

kpayson64 added priority/P0 and removed disposition/DO NOT MERGE labels Feb 5, 2018

kpayson64 assigned markdroth Feb 5, 2018

kpayson64 changed the base branch from master to v1.9.x February 5, 2018 20:43

dgquintas approved these changes Feb 5, 2018

View reviewed changes

kpayson64 merged commit 9e620c3 into grpc:v1.9.x Feb 5, 2018

This was referenced Feb 7, 2018

Arm architecture grpc is crashing and BUS error as well as segv #14001

Closed

FAILURE: SEGV in bins/dbg/secure_sync_unary_ping_pong_test #14040

Closed

dgquintas mentioned this pull request Feb 7, 2018

Write a test for the issue fixed in #14315 #14357

Closed

lock bot locked as resolved and limited conversation to collaborators Jan 20, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes refcounting issue#14315

Fixes refcounting issue#14315
kpayson64 merged 1 commit intogrpc:v1.9.xfrom
kpayson64:fix_refcounting

kpayson64 commented Feb 5, 2018 •

edited

Loading

Uh oh!

grpc-testing commented Feb 5, 2018

Uh oh!

grpc-testing commented Feb 5, 2018

Uh oh!

grpc-testing commented Feb 5, 2018

Uh oh!

grpc-testing commented Feb 5, 2018

Uh oh!

grpc-testing commented Feb 5, 2018

Uh oh!

dgquintas left a comment

Uh oh!

grpc-testing commented Feb 5, 2018

Uh oh!

kpayson64 commented Feb 5, 2018

Uh oh!

markdroth commented Feb 6, 2018

Uh oh!

kpayson64 commented Feb 6, 2018

Uh oh!

kpayson64 commented Feb 6, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

kpayson64 commented Feb 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

grpc-testing commented Feb 5, 2018

Uh oh!

grpc-testing commented Feb 5, 2018

Uh oh!

grpc-testing commented Feb 5, 2018

Uh oh!

grpc-testing commented Feb 5, 2018

Uh oh!

grpc-testing commented Feb 5, 2018

Uh oh!

dgquintas left a comment

Choose a reason for hiding this comment

Uh oh!

grpc-testing commented Feb 5, 2018

Uh oh!

kpayson64 commented Feb 5, 2018

Uh oh!

markdroth commented Feb 6, 2018

Uh oh!

kpayson64 commented Feb 6, 2018

Uh oh!

kpayson64 commented Feb 6, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kpayson64 commented Feb 5, 2018 •

edited

Loading