Resolve various races in QPS sync client by vjpai · Pull Request #13576 · grpc/grpc

vjpai · 2017-11-30T20:13:02Z

Fixes #13122 and various related issues

This resolves several different kinds of races in the QPS sync client:

Check if a specific RPC client thread needs to be shutdown before it is even started (analogous to Fix TSAN json_run_localhost flake #13409 for the async client)
Move end-of-test stream cancellation to DestroyMultithreading (which is called before the client has to shutdown) rather than at the client destructor. Test cases such as streaming_from_server could end up getting stuck forever otherwise if a client thread was still doing a Read op and could never escape it to do the thread join, creating a deadlock
Don't let an RPC completion trigger a new RPC if the thread has needed to be shutdown in between
Don't let an RPC finishing trigger a refresh of the ClientContext if the thread has needed to be shutdown meanwhile
Protect stream-changing operations with a lock so that a cancellation in a different thread doesn't conflict with a regular stream operation in the common thread for handling a stream

Additionally, this PR refactors a common code path by which a "cleanup" operation is applied to all streams at different points.

grpc-testing · 2017-11-30T20:16:16Z

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

libgrpc.so

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

  [ = ]       0        0  [ = ]


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

libgrpc++.so

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

  [ = ]       0        0  [ = ]

grpc-testing · 2017-11-30T20:29:36Z

[trickle] No significant performance differences

grpc-testing · 2017-11-30T21:08:01Z

[microbenchmarks] No significant performance differences

grpc-testing · 2017-11-30T22:07:49Z

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

libgrpc.so

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

  [ = ]       0        0  [ = ]


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

libgrpc++.so

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

  [ = ]       0        0  [ = ]

grpc-testing · 2017-11-30T22:19:59Z

[trickle] No significant performance differences

grpc-testing · 2017-11-30T22:42:46Z

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

libgrpc.so

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

  [ = ]       0        0  [ = ]


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

libgrpc++.so

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

  [ = ]       0        0  [ = ]

grpc-testing · 2017-11-30T22:55:34Z

[trickle] No significant performance differences

grpc-testing · 2017-11-30T22:59:19Z

[microbenchmarks] No significant performance differences

grpc-testing · 2017-11-30T23:35:42Z

[microbenchmarks] No significant performance differences

grpc-testing · 2017-12-01T17:00:47Z

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

libgrpc.so

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

  [ = ]       0        0  [ = ]


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

libgrpc++.so

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

  [ = ]       0        0  [ = ]

grpc-testing · 2017-12-01T17:13:58Z

[trickle] No significant performance differences

grpc-testing · 2017-12-01T17:52:25Z

[microbenchmarks] No significant performance differences

grpc-testing · 2017-12-04T08:44:07Z

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

libgrpc.so

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

  [ = ]       0        0  [ = ]


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

libgrpc++.so

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

  [ = ]       0        0  [ = ]

grpc-testing · 2017-12-04T08:54:21Z

[trickle] No significant performance differences

grpc-testing · 2017-12-04T09:02:23Z

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

libgrpc.so

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

  [ = ]       0        0  [ = ]


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

libgrpc++.so

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

  [ = ]       0        0  [ = ]

grpc-testing · 2017-12-04T09:15:12Z

[trickle] No significant performance differences

grpc-testing · 2017-12-04T09:35:19Z

[microbenchmarks] No significant performance differences

grpc-testing · 2017-12-04T09:43:27Z

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

libgrpc.so

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

  [ = ]       0        0  [ = ]


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

libgrpc++.so

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

  [ = ]       0        0  [ = ]

grpc-testing · 2017-12-04T09:53:42Z

[microbenchmarks] No significant performance differences

grpc-testing · 2017-12-04T09:53:47Z

[trickle] No significant performance differences

grpc-testing · 2017-12-04T10:35:57Z

[microbenchmarks] No significant performance differences

vjpai · 2017-12-04T14:26:52Z

The flakes on this PR have nothing to do with QPS testing. This is ready to review.

kpayson64

Do you mind adding a quick comment in the PR description as to the nature of the races (Handle Shutdown before initialization should be fine if I understand this correctly).

Also what do you think about changing the cleaner lambda to a overrideable function? Feel free to say "lambda's are lit, get with the times old man".

kpayson64 · 2017-12-04T15:42:35Z

test/cpp/qps/client_sync.cc

+      cleanup_threads.emplace_back([this, i, cleaner]() {
+        std::lock_guard<std::mutex> l(stream_mu_[i]);
+        if (stream_[i]) {
+          shutdown_[i].val = cleaner(&context_[i], stream_[i].get());


I'm a little confused by this. What happens if stream is not initialized by the time we call OnAllStreams? Then shutdown would never get set. It looks like this PR addresses a race condition where we shutdown before we finish initialization, but if looks like its impossible for that to happen.

Good point, I guess that case wasn't happening yet in practice. I've resolved this.

kpayson64 · 2017-12-04T15:45:43Z

test/cpp/qps/client_sync.cc

+      cleanup_threads.emplace_back([this, i, cleaner]() {
+        std::lock_guard<std::mutex> l(stream_mu_[i]);
+        if (stream_[i]) {
+          shutdown_[i].val = cleaner(&context_[i], stream_[i].get());


It looks like all the lambdas return true. Can we just assign shutdown_[i].val = true for simplicity? (I was confused by what happens if the cleaner fails at first).

Sounds good. My original intention was to make this a general map-operation-on-stream but there is no need for such generality now (or possibly ever). Changed.

…rtual

vjpai · 2017-12-04T18:01:43Z

I've also edited the PR description with details about the races being resolved and changed the lambdas to a virtual function. I was originally resistant about doing that because the base class SynchronousStreamingClient actually called OnAllStreams in two different places: from DestroyMultithreading and also from the destructor. So I offloaded the part from the destructor down to the derived classes and only kept the version that gets called when destroying multithreading. Again, it's not the most general thing to do but it should be ok for this.

grpc-testing · 2017-12-04T18:01:51Z

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

libgrpc.so

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

  [ = ]       0        0  [ = ]


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

libgrpc++.so

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

  [ = ]       0        0  [ = ]

grpc-testing · 2017-12-04T18:02:59Z

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

libgrpc.so

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

  [ = ]       0        0  [ = ]


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

libgrpc++.so

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

  [ = ]       0        0  [ = ]

vjpai · 2017-12-04T18:03:33Z

Wait, my last change was dumb. I should have tested it locally first, will fix.

grpc-testing · 2017-12-04T18:11:49Z

[trickle] No significant performance differences

grpc-testing · 2017-12-04T18:16:36Z

[trickle] No significant performance differences

vjpai · 2017-12-04T18:16:38Z

I've changed my mind. Lambdas are lit. This is ready for re-review. I think the new form of the lambdas is better.

grpc-testing · 2017-12-04T18:20:44Z

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

libgrpc.so

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

  [ = ]       0        0  [ = ]


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

libgrpc++.so

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

  [ = ]       0        0  [ = ]

grpc-testing · 2017-12-04T18:32:13Z

[trickle] No significant performance differences

grpc-testing · 2017-12-04T18:54:23Z

[microbenchmarks] No significant performance differences

grpc-testing · 2017-12-04T18:54:36Z

[microbenchmarks] No significant performance differences

grpc-testing · 2017-12-04T19:12:06Z

Corrupt JSON data (indicates timeout or crash): 
    bm_fullstack_unary_ping_pong.BM_UnaryPingPong_MinInProcessCHTTP2_NoOpMutator_NoOpMutator__1_0.counters.old: 1


[microbenchmarks] No significant performance differences

vjpai · 2017-12-04T19:59:25Z

Again, no QPS-related flakes!

vjpai added the disposition/Work In Progress label Nov 30, 2017

vjpai mentioned this pull request Dec 1, 2017

json_run_localhost:cpp_protobuf_sync_streaming_from_server_qps_unconstrained_secure_low_thread_count timeout (on ASAN and TSAN) #13122

Closed

vjpai force-pushed the qps_killer branch from d03683e to 8560b64 Compare December 4, 2017 08:40

Adjust stream cancellation point and fix races in sync client

6389457

vjpai force-pushed the qps_killer branch from 8560b64 to 6389457 Compare December 4, 2017 08:58

vjpai changed the title ~~Attempt to resolve sync qps test timeouts~~ Resolve various races in QPS sync client Dec 4, 2017

vjpai removed the disposition/Work In Progress label Dec 4, 2017

Remove some unneeded and spammy logs

c8dd4c5

vjpai requested a review from kpayson64 December 4, 2017 09:38

vjpai assigned kpayson64 Dec 4, 2017

kpayson64 suggested changes Dec 4, 2017

View reviewed changes

vjpai added 2 commits December 4, 2017 09:56

Make all-streams op about cleanup only and replace a lambda with a vi…

083b9be

…rtual

clang-format

f230ffd

Reintroduce lambdas

c6587ca

kpayson64 approved these changes Dec 4, 2017

View reviewed changes

vjpai merged commit c3b1e55 into grpc:master Dec 4, 2017

vjpai deleted the qps_killer branch December 4, 2017 20:03

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

Conversation

vjpai commented Nov 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

grpc-testing commented Nov 30, 2017

Uh oh!

grpc-testing commented Nov 30, 2017

Uh oh!

grpc-testing commented Nov 30, 2017

Uh oh!

grpc-testing commented Nov 30, 2017

Uh oh!

grpc-testing commented Nov 30, 2017

Uh oh!

grpc-testing commented Nov 30, 2017

Uh oh!

grpc-testing commented Nov 30, 2017

Uh oh!

grpc-testing commented Nov 30, 2017

Uh oh!

grpc-testing commented Nov 30, 2017

Uh oh!

grpc-testing commented Dec 1, 2017

Uh oh!

grpc-testing commented Dec 1, 2017

Uh oh!

grpc-testing commented Dec 1, 2017

Uh oh!

grpc-testing commented Dec 4, 2017

Uh oh!

grpc-testing commented Dec 4, 2017

Uh oh!

grpc-testing commented Dec 4, 2017

Uh oh!

grpc-testing commented Dec 4, 2017

Uh oh!

grpc-testing commented Dec 4, 2017

Uh oh!

grpc-testing commented Dec 4, 2017

Uh oh!

grpc-testing commented Dec 4, 2017

Uh oh!

grpc-testing commented Dec 4, 2017

Uh oh!

grpc-testing commented Dec 4, 2017

Uh oh!

vjpai commented Dec 4, 2017

Uh oh!

kpayson64 left a comment

Choose a reason for hiding this comment

Uh oh!

kpayson64 Dec 4, 2017

Choose a reason for hiding this comment

Uh oh!

vjpai Dec 4, 2017

Choose a reason for hiding this comment

Uh oh!

kpayson64 Dec 4, 2017

Choose a reason for hiding this comment

Uh oh!

vjpai Dec 4, 2017

Choose a reason for hiding this comment

Uh oh!

vjpai commented Dec 4, 2017

Uh oh!

grpc-testing commented Dec 4, 2017

Uh oh!

grpc-testing commented Dec 4, 2017

Uh oh!

vjpai commented Dec 4, 2017

Uh oh!

grpc-testing commented Dec 4, 2017

Uh oh!

grpc-testing commented Dec 4, 2017

Uh oh!

vjpai commented Dec 4, 2017

Uh oh!

grpc-testing commented Dec 4, 2017

Uh oh!

grpc-testing commented Dec 4, 2017

vjpai commented Nov 30, 2017 •

edited

Loading