Fix race condition in grpc_tsi_alts_shutdown by yihuazhang · Pull Request #14296 · grpc/grpc

yihuazhang · 2018-02-02T17:56:40Z

In the original implementation, cq is destroyed before being drained which resulted in a race condition between TSI and main threads. That is, it was possible for the main thread to invoke grpc_completion_queue_destroy() on cq while the TSI thread is still actively using it. The correct implementation is cq should be drained before grpc_completion_queue_destroy() gets invoked. We need to call grpc_completion_queue_shutdown(), wait for TSI thread to finish working on it, then signal the main thread to continue with the destroy.

grpc-testing · 2018-02-02T18:00:01Z

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

libgrpc.so

     VM SIZE                                               FILE SIZE
 ++++++++++++++ GROWING                                 ++++++++++++++
  +0.0%    +196 [None]                                     +884  +0.0%
      +0.0%    +132 [Unmapped]                                 +884  +0.0%
       +88%     +56 g_alts_resource                               0  [ = ]
      +0.7%      +8 [None]                                        0  [ = ]
  +161%    +188 src/core/tsi/alts_transport_security.cc    +188  +161%
      +133%     +92 grpc_tsi_alts_shutdown                      +92  +133%
      [NEW]     +51 grpc_tsi_alts_signal_for_cq_destroy         +51  [NEW]
      +312%     +25 [Unmapped]                                  +25  +312%
       +62%     +20 grpc_tsi_alts_init                          +20   +62%

  +0.0%    +384 TOTAL                                   +1.05Ki  +0.0%


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

libgrpc++.so

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

  [ = ]       0        0  [ = ]

grpc-testing · 2018-02-02T18:14:09Z

[trickle] No significant performance differences

grpc-testing · 2018-02-02T18:51:43Z

[microbenchmarks] No significant performance differences

jiangtaoli2016 · 2018-02-02T19:19:22Z

src/core/tsi/alts_transport_security.cc

why not use the following to avoid goto.
if (g_alts_resource.cq != nullptr) {
...
}

grpc-testing · 2018-02-02T23:01:21Z

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

libgrpc.so

     VM SIZE                                               FILE SIZE
 ++++++++++++++ GROWING                                 ++++++++++++++
  +0.0%    +196 [None]                                     +852  +0.0%
      +0.0%    +132 [Unmapped]                                 +852  +0.0%
       +88%     +56 g_alts_resource                               0  [ = ]
      +0.7%      +8 [None]                                        0  [ = ]
  +161%    +188 src/core/tsi/alts_transport_security.cc    +188  +161%
      +133%     +92 grpc_tsi_alts_shutdown                      +92  +133%
      [NEW]     +51 grpc_tsi_alts_signal_for_cq_destroy         +51  [NEW]
      +312%     +25 [Unmapped]                                  +25  +312%
       +62%     +20 grpc_tsi_alts_init                          +20   +62%

  +0.0%    +384 TOTAL                                   +1.02Ki  +0.0%


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

libgrpc++.so

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

  [ = ]       0        0  [ = ]

jiangtaoli2016

LGTM. Could you squash the commits?

grpc-testing · 2018-02-02T23:11:56Z

[trickle] No significant performance differences

grpc-testing · 2018-02-02T23:33:19Z

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

libgrpc.so

     VM SIZE                                               FILE SIZE
 ++++++++++++++ GROWING                                 ++++++++++++++
  +0.0%    +196 [None]                                     +852  +0.0%
      +0.0%    +132 [Unmapped]                                 +852  +0.0%
       +88%     +56 g_alts_resource                               0  [ = ]
      +0.7%      +8 [None]                                        0  [ = ]
  +161%    +188 src/core/tsi/alts_transport_security.cc    +188  +161%
      +133%     +92 grpc_tsi_alts_shutdown                      +92  +133%
      [NEW]     +51 grpc_tsi_alts_signal_for_cq_destroy         +51  [NEW]
      +312%     +25 [Unmapped]                                  +25  +312%
       +62%     +20 grpc_tsi_alts_init                          +20   +62%

  +0.0%    +384 TOTAL                                   +1.02Ki  +0.0%


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

libgrpc++.so

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

  [ = ]       0        0  [ = ]

grpc-testing · 2018-02-02T23:42:23Z

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

libgrpc.so

     VM SIZE                                               FILE SIZE
 ++++++++++++++ GROWING                                 ++++++++++++++
  +0.0%    +196 [None]                                     +852  +0.0%
      +0.0%    +132 [Unmapped]                                 +852  +0.0%
       +88%     +56 g_alts_resource                               0  [ = ]
      +0.7%      +8 [None]                                        0  [ = ]
  +161%    +188 src/core/tsi/alts_transport_security.cc    +188  +161%
      +133%     +92 grpc_tsi_alts_shutdown                      +92  +133%
      [NEW]     +51 grpc_tsi_alts_signal_for_cq_destroy         +51  [NEW]
      +312%     +25 [Unmapped]                                  +25  +312%
       +62%     +20 grpc_tsi_alts_init                          +20   +62%

  +0.0%    +384 TOTAL                                   +1.02Ki  +0.0%


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

libgrpc++.so

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

  [ = ]       0        0  [ = ]

grpc-testing · 2018-02-02T23:46:27Z

[trickle] No significant performance differences

grpc-testing · 2018-02-02T23:52:37Z

[microbenchmarks] No significant performance differences

grpc-testing · 2018-02-02T23:57:13Z

[trickle] No significant performance differences

grpc-testing · 2018-02-03T00:25:01Z

[microbenchmarks] No significant performance differences

grpc-testing · 2018-02-03T00:34:35Z

[microbenchmarks] No significant performance differences

grpc-testing · 2018-02-03T01:35:28Z

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

libgrpc.so

     VM SIZE                                               FILE SIZE
 ++++++++++++++ GROWING                                 ++++++++++++++
  +0.0%    +164 [None]                                     +844  +0.0%
      +0.0%    +100 [Unmapped]                                 +844  +0.0%
       +88%     +56 g_alts_resource                               0  [ = ]
      +0.7%      +8 [None]                                        0  [ = ]
  +161%    +188 src/core/tsi/alts_transport_security.cc    +188  +161%
      +133%     +92 grpc_tsi_alts_shutdown                      +92  +133%
      [NEW]     +51 grpc_tsi_alts_signal_for_cq_destroy         +51  [NEW]
      +312%     +25 [Unmapped]                                  +25  +312%
       +62%     +20 grpc_tsi_alts_init                          +20   +62%

  +0.0%    +352 TOTAL                                   +1.01Ki  +0.0%


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

libgrpc++.so

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

  [ = ]       0        0  [ = ]

grpc-testing · 2018-02-03T01:47:08Z

[trickle] No significant performance differences

grpc-testing · 2018-02-03T02:27:02Z

[microbenchmarks] No significant performance differences

grpc-testing · 2018-02-05T17:34:27Z

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

libgrpc.so

     VM SIZE                                               FILE SIZE
 ++++++++++++++ GROWING                                 ++++++++++++++
  +0.0%    +164 [None]                                     +852  +0.0%
      +0.0%    +100 [Unmapped]                                 +852  +0.0%
       +88%     +56 g_alts_resource                               0  [ = ]
      +0.7%      +8 [None]                                        0  [ = ]
  +161%    +188 src/core/tsi/alts_transport_security.cc    +188  +161%
      +133%     +92 grpc_tsi_alts_shutdown                      +92  +133%
      [NEW]     +51 grpc_tsi_alts_signal_for_cq_destroy         +51  [NEW]
      +312%     +25 [Unmapped]                                  +25  +312%
       +62%     +20 grpc_tsi_alts_init                          +20   +62%

  +0.0%    +352 TOTAL                                   +1.02Ki  +0.0%


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

libgrpc++.so

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

  [ = ]       0        0  [ = ]

grpc-testing · 2018-02-05T17:42:50Z

[trickle] No significant performance differences

grpc-testing · 2018-02-05T19:22:21Z

Corrupt JSON data (indicates timeout or crash): 
    bm_fullstack_streaming_pump.BM_PumpStreamServerToClient_SockPair__512.opt.new: 1
    bm_fullstack_streaming_pump.BM_PumpStreamServerToClient_SockPair__512.counters.new: 1


[microbenchmarks] No significant performance differences

yihuazhang · 2018-02-07T21:20:17Z

It seems the errors are not related to this PR.

yihuazhang requested a review from jiangtaoli2016 February 2, 2018 17:56

jiangtaoli2016 reviewed Feb 2, 2018

View reviewed changes

jiangtaoli2016 approved these changes Feb 2, 2018

View reviewed changes

yihuazhang force-pushed the FIX_ALTS_SHUTDOWN branch from cbc96fb to f65b6a1 Compare February 2, 2018 23:30

yihuazhang force-pushed the FIX_ALTS_SHUTDOWN branch from f65b6a1 to d173770 Compare February 2, 2018 23:39

jiangtaoli2016 added the kokoro:force-run label Feb 3, 2018

kokoro-team removed the kokoro:force-run label Feb 3, 2018

fix race condition in grpc_tsi_alts_shutdown()

6d55db0

yihuazhang force-pushed the FIX_ALTS_SHUTDOWN branch from d173770 to 6d55db0 Compare February 5, 2018 17:31

jiangtaoli2016 merged commit fdc10b6 into grpc:master Feb 7, 2018

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

Conversation

yihuazhang commented Feb 2, 2018

Uh oh!

grpc-testing commented Feb 2, 2018

Uh oh!

grpc-testing commented Feb 2, 2018

Uh oh!

grpc-testing commented Feb 2, 2018

Uh oh!

jiangtaoli2016 Feb 2, 2018

Choose a reason for hiding this comment

Uh oh!

grpc-testing commented Feb 2, 2018

Uh oh!

jiangtaoli2016 left a comment

Choose a reason for hiding this comment

Uh oh!

grpc-testing commented Feb 2, 2018

Uh oh!

grpc-testing commented Feb 2, 2018

Uh oh!

grpc-testing commented Feb 2, 2018

Uh oh!

grpc-testing commented Feb 2, 2018

Uh oh!

grpc-testing commented Feb 2, 2018

Uh oh!

grpc-testing commented Feb 2, 2018

Uh oh!

grpc-testing commented Feb 3, 2018

Uh oh!

grpc-testing commented Feb 3, 2018

Uh oh!

grpc-testing commented Feb 3, 2018

Uh oh!

grpc-testing commented Feb 3, 2018

Uh oh!

grpc-testing commented Feb 3, 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!

yihuazhang commented Feb 7, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants