From 19f0b1da674cefe1f2c79133aa4e754d885c7a8a Mon Sep 17 00:00:00 2001 From: Yash Tibrewal Date: Mon, 21 May 2018 22:05:12 -0700 Subject: [PATCH 1/4] Fix for issue #15458 --- test/core/end2end/fuzzers/server_fuzzer.cc | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/test/core/end2end/fuzzers/server_fuzzer.cc b/test/core/end2end/fuzzers/server_fuzzer.cc index 5eb83ede7ad..2de4ee55ed7 100644 --- a/test/core/end2end/fuzzers/server_fuzzer.cc +++ b/test/core/end2end/fuzzers/server_fuzzer.cc @@ -103,15 +103,24 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) { grpc_metadata_array_destroy(&request_metadata1); grpc_server_shutdown_and_notify(server, cq, tag(0xdead)); grpc_server_cancel_all_calls(server); + grpc_millis deadline = grpc_core::ExecCtx::Get()->Now() + 100; for (int i = 0; i <= requested_calls; i++) { - ev = grpc_completion_queue_next(cq, gpr_inf_past(GPR_CLOCK_REALTIME), - nullptr); + do { + ev = grpc_completion_queue_next(cq, gpr_inf_past(GPR_CLOCK_REALTIME), + nullptr); + grpc_core::ExecCtx::Get()->InvalidateNow(); + } while (ev.type != GRPC_OP_COMPLETE && + grpc_core::ExecCtx::Get()->Now() < deadline); GPR_ASSERT(ev.type == GRPC_OP_COMPLETE); } grpc_completion_queue_shutdown(cq); for (int i = 0; i <= requested_calls; i++) { - ev = grpc_completion_queue_next(cq, gpr_inf_past(GPR_CLOCK_REALTIME), - nullptr); + do { + ev = grpc_completion_queue_next(cq, gpr_inf_past(GPR_CLOCK_REALTIME), + nullptr); + grpc_core::ExecCtx::Get()->InvalidateNow(); + } while (ev.type != GRPC_QUEUE_SHUTDOWN && + grpc_core::ExecCtx::Get()->Now() < deadline); GPR_ASSERT(ev.type == GRPC_QUEUE_SHUTDOWN); } grpc_server_destroy(server); From baa3a7090651bfb2d3c41c3d74f80bea8b911f87 Mon Sep 17 00:00:00 2001 From: Yash Tibrewal Date: Tue, 22 May 2018 11:27:10 -0700 Subject: [PATCH 2/4] 5 second deadline for shutting down --- test/core/end2end/fuzzers/server_fuzzer.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/core/end2end/fuzzers/server_fuzzer.cc b/test/core/end2end/fuzzers/server_fuzzer.cc index 2de4ee55ed7..21e99e34d93 100644 --- a/test/core/end2end/fuzzers/server_fuzzer.cc +++ b/test/core/end2end/fuzzers/server_fuzzer.cc @@ -103,7 +103,7 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) { grpc_metadata_array_destroy(&request_metadata1); grpc_server_shutdown_and_notify(server, cq, tag(0xdead)); grpc_server_cancel_all_calls(server); - grpc_millis deadline = grpc_core::ExecCtx::Get()->Now() + 100; + grpc_millis deadline = grpc_core::ExecCtx::Get()->Now() + 5000; for (int i = 0; i <= requested_calls; i++) { do { ev = grpc_completion_queue_next(cq, gpr_inf_past(GPR_CLOCK_REALTIME), From 8aebc7225dc19a5fa679b9cbca1847173566619b Mon Sep 17 00:00:00 2001 From: Yash Tibrewal Date: Tue, 22 May 2018 16:07:49 -0700 Subject: [PATCH 3/4] Add comment on the changes --- test/core/end2end/fuzzers/server_fuzzer.cc | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/test/core/end2end/fuzzers/server_fuzzer.cc b/test/core/end2end/fuzzers/server_fuzzer.cc index 21e99e34d93..d1259259f39 100644 --- a/test/core/end2end/fuzzers/server_fuzzer.cc +++ b/test/core/end2end/fuzzers/server_fuzzer.cc @@ -105,6 +105,13 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) { grpc_server_cancel_all_calls(server); grpc_millis deadline = grpc_core::ExecCtx::Get()->Now() + 5000; for (int i = 0; i <= requested_calls; i++) { + // A single grpc_completion_queue_next might not be sufficient for getting + // the tag from shutdown, because we might potentially get blocked by + // an operation happening on the timer thread. + // For example, the deadline timer might expire, leading to the timer + // thread trying to cancel the RPC and thereby acquiring a few references + // to the call. This will prevent the shutdown to complete till the timer + // thread releases those references. do { ev = grpc_completion_queue_next(cq, gpr_inf_past(GPR_CLOCK_REALTIME), nullptr); From 09335338eb03e2007ab1d2133ca82545c3ffd884 Mon Sep 17 00:00:00 2001 From: Yash Tibrewal Date: Wed, 23 May 2018 12:59:12 -0700 Subject: [PATCH 4/4] Indicate the reasoning for 5 seconds deadline --- test/core/end2end/fuzzers/server_fuzzer.cc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/core/end2end/fuzzers/server_fuzzer.cc b/test/core/end2end/fuzzers/server_fuzzer.cc index d1259259f39..248c34cbc18 100644 --- a/test/core/end2end/fuzzers/server_fuzzer.cc +++ b/test/core/end2end/fuzzers/server_fuzzer.cc @@ -112,6 +112,8 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) { // thread trying to cancel the RPC and thereby acquiring a few references // to the call. This will prevent the shutdown to complete till the timer // thread releases those references. + // As a solution, we are going to keep performing a cq_next for a + // liberal period of 5 seconds for the timer thread to complete its work. do { ev = grpc_completion_queue_next(cq, gpr_inf_past(GPR_CLOCK_REALTIME), nullptr);