From 7b01878736840395f13244bd68edaf0d88b4c3e4 Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Fri, 16 Jan 2015 09:53:39 -0800 Subject: [PATCH] Fix returned status The client should return status deadline exceeded when the deadline is exceeded (not cancelled status) --- src/core/surface/call.c | 5 ++ test/core/end2end/dualstack_socket_test.c | 5 +- test/core/end2end/no_server_test.c | 4 +- test/core/end2end/tests/cancel_after_accept.c | 22 +++----- .../cancel_after_accept_and_writes_closed.c | 22 +++----- test/core/end2end/tests/cancel_after_invoke.c | 22 +++----- .../core/end2end/tests/cancel_before_invoke.c | 4 -- test/core/end2end/tests/cancel_in_a_vacuum.c | 18 ++----- test/core/end2end/tests/cancel_test_helpers.h | 52 +++++++++++++++++++ test/cpp/end2end/end2end_test.cc | 4 +- 10 files changed, 89 insertions(+), 69 deletions(-) create mode 100644 test/core/end2end/tests/cancel_test_helpers.h diff --git a/src/core/surface/call.c b/src/core/surface/call.c index db6dbe04e4c..fd4e590aad5 100644 --- a/src/core/surface/call.c +++ b/src/core/surface/call.c @@ -897,8 +897,13 @@ grpc_metadata_buffer *grpc_call_get_metadata_buffer(grpc_call *call) { static void call_alarm(void *arg, int success) { grpc_call *call = arg; if (success) { + if (call->is_client) { + grpc_call_cancel_with_status(call, GRPC_STATUS_DEADLINE_EXCEEDED, + "Deadline Exceeded"); + } else { grpc_call_cancel(call); } + } grpc_call_internal_unref(call); } diff --git a/test/core/end2end/dualstack_socket_test.c b/test/core/end2end/dualstack_socket_test.c index 88a6acdd7d9..4327b912987 100644 --- a/test/core/end2end/dualstack_socket_test.c +++ b/test/core/end2end/dualstack_socket_test.c @@ -154,8 +154,9 @@ void test_connect(const char *server_host, const char *client_host, int port, /* Check for a failed connection. */ cq_expect_invoke_accepted(v_client, tag(1), GRPC_OP_ERROR); cq_expect_client_metadata_read(v_client, tag(2), NULL); - cq_expect_finished_with_status(v_client, tag(3), GRPC_STATUS_CANCELLED, - NULL, NULL); + cq_expect_finished_with_status(v_client, tag(3), + GRPC_STATUS_DEADLINE_EXCEEDED, + "Deadline Exceeded", NULL); cq_verify(v_client); grpc_call_destroy(c); diff --git a/test/core/end2end/no_server_test.c b/test/core/end2end/no_server_test.c index ba6349c109b..b9660f14b30 100644 --- a/test/core/end2end/no_server_test.c +++ b/test/core/end2end/no_server_test.c @@ -62,8 +62,8 @@ int main(int argc, char **argv) { /* verify that all tags get completed */ cq_expect_invoke_accepted(cqv, tag(1), GRPC_OP_ERROR); cq_expect_client_metadata_read(cqv, tag(2), NULL); - cq_expect_finished_with_status(cqv, tag(3), GRPC_STATUS_CANCELLED, NULL, - NULL); + cq_expect_finished_with_status(cqv, tag(3), GRPC_STATUS_DEADLINE_EXCEEDED, + "Deadline Exceeded", NULL); cq_verify(cqv); grpc_completion_queue_shutdown(cq); diff --git a/test/core/end2end/tests/cancel_after_accept.c b/test/core/end2end/tests/cancel_after_accept.c index 85686051970..cfbb4796aae 100644 --- a/test/core/end2end/tests/cancel_after_accept.c +++ b/test/core/end2end/tests/cancel_after_accept.c @@ -43,14 +43,7 @@ #include #include #include "test/core/end2end/cq_verifier.h" - -/* allow cancellation by either grpc_call_cancel, or by wait_for_deadline (which - * does nothing) */ -typedef grpc_call_error (*canceller)(grpc_call *call); - -static grpc_call_error wait_for_deadline(grpc_call *call) { - return GRPC_CALL_OK; -} +#include "test/core/end2end/tests/cancel_test_helpers.h" enum { TIMEOUT = 200000 }; @@ -112,7 +105,7 @@ static void end_test(grpc_end2end_test_fixture *f) { /* Cancel after accept, no payload */ static void test_cancel_after_accept(grpc_end2end_test_config config, - canceller call_cancel) { + cancellation_mode mode) { grpc_call *c; grpc_call *s; grpc_end2end_test_fixture f = begin_test(config, __FUNCTION__, NULL, NULL); @@ -138,10 +131,10 @@ static void test_cancel_after_accept(grpc_end2end_test_config config, cq_expect_client_metadata_read(v_client, tag(2), NULL); cq_verify(v_client); - GPR_ASSERT(GRPC_CALL_OK == call_cancel(c)); + GPR_ASSERT(GRPC_CALL_OK == mode.initiate_cancel(c)); - cq_expect_finished_with_status(v_client, tag(3), GRPC_STATUS_CANCELLED, NULL, - NULL); + cq_expect_finished_with_status(v_client, tag(3), mode.expect_status, + mode.expect_details, NULL); cq_verify(v_client); cq_expect_finished_with_status(v_server, tag(102), GRPC_STATUS_CANCELLED, @@ -159,9 +152,8 @@ static void test_cancel_after_accept(grpc_end2end_test_config config, void grpc_end2end_tests(grpc_end2end_test_config config) { int i; - canceller cancellers[2] = {grpc_call_cancel, wait_for_deadline}; - for (i = 0; i < GPR_ARRAY_SIZE(cancellers); i++) { - test_cancel_after_accept(config, cancellers[i]); + for (i = 0; i < GPR_ARRAY_SIZE(cancellation_modes); i++) { + test_cancel_after_accept(config, cancellation_modes[i]); } } diff --git a/test/core/end2end/tests/cancel_after_accept_and_writes_closed.c b/test/core/end2end/tests/cancel_after_accept_and_writes_closed.c index 798051f01b2..74670bdc916 100644 --- a/test/core/end2end/tests/cancel_after_accept_and_writes_closed.c +++ b/test/core/end2end/tests/cancel_after_accept_and_writes_closed.c @@ -43,14 +43,7 @@ #include #include #include "test/core/end2end/cq_verifier.h" - -/* allow cancellation by either grpc_call_cancel, or by wait_for_deadline (which - * does nothing) */ -typedef grpc_call_error (*canceller)(grpc_call *call); - -static grpc_call_error wait_for_deadline(grpc_call *call) { - return GRPC_CALL_OK; -} +#include "test/core/end2end/tests/cancel_test_helpers.h" enum { TIMEOUT = 200000 }; @@ -112,7 +105,7 @@ static void end_test(grpc_end2end_test_fixture *f) { /* Cancel after accept with a writes closed, no payload */ static void test_cancel_after_accept_and_writes_closed( - grpc_end2end_test_config config, canceller call_cancel) { + grpc_end2end_test_config config, cancellation_mode mode) { grpc_call *c; grpc_call *s; grpc_end2end_test_fixture f = begin_test(config, __FUNCTION__, NULL, NULL); @@ -146,10 +139,10 @@ static void test_cancel_after_accept_and_writes_closed( cq_expect_empty_read(v_server, tag(101)); cq_verify(v_server); - GPR_ASSERT(GRPC_CALL_OK == call_cancel(c)); + GPR_ASSERT(GRPC_CALL_OK == mode.initiate_cancel(c)); - cq_expect_finished_with_status(v_client, tag(3), GRPC_STATUS_CANCELLED, NULL, - NULL); + cq_expect_finished_with_status(v_client, tag(3), mode.expect_status, + mode.expect_details, NULL); cq_verify(v_client); cq_expect_finished_with_status(v_server, tag(102), GRPC_STATUS_CANCELLED, @@ -167,9 +160,8 @@ static void test_cancel_after_accept_and_writes_closed( void grpc_end2end_tests(grpc_end2end_test_config config) { int i; - canceller cancellers[2] = {grpc_call_cancel, wait_for_deadline}; - for (i = 0; i < GPR_ARRAY_SIZE(cancellers); i++) { - test_cancel_after_accept_and_writes_closed(config, cancellers[i]); + for (i = 0; i < GPR_ARRAY_SIZE(cancellation_modes); i++) { + test_cancel_after_accept_and_writes_closed(config, cancellation_modes[i]); } } diff --git a/test/core/end2end/tests/cancel_after_invoke.c b/test/core/end2end/tests/cancel_after_invoke.c index 3c9122944bd..d4cb5e4f133 100644 --- a/test/core/end2end/tests/cancel_after_invoke.c +++ b/test/core/end2end/tests/cancel_after_invoke.c @@ -43,14 +43,7 @@ #include #include #include "test/core/end2end/cq_verifier.h" - -/* allow cancellation by either grpc_call_cancel, or by wait_for_deadline (which - * does nothing) */ -typedef grpc_call_error (*canceller)(grpc_call *call); - -static grpc_call_error wait_for_deadline(grpc_call *call) { - return GRPC_CALL_OK; -} +#include "test/core/end2end/tests/cancel_test_helpers.h" enum { TIMEOUT = 200000 }; @@ -112,7 +105,7 @@ static void end_test(grpc_end2end_test_fixture *f) { /* Cancel after invoke, no payload */ static void test_cancel_after_invoke(grpc_end2end_test_config config, - canceller call_cancel) { + cancellation_mode mode) { grpc_call *c; grpc_end2end_test_fixture f = begin_test(config, __FUNCTION__, NULL, NULL); gpr_timespec deadline = five_seconds_time(); @@ -126,11 +119,11 @@ static void test_cancel_after_invoke(grpc_end2end_test_config config, cq_expect_invoke_accepted(v_client, tag(1), GRPC_OP_OK); cq_verify(v_client); - GPR_ASSERT(GRPC_CALL_OK == call_cancel(c)); + GPR_ASSERT(GRPC_CALL_OK == mode.initiate_cancel(c)); cq_expect_client_metadata_read(v_client, tag(2), NULL); - cq_expect_finished_with_status(v_client, tag(3), GRPC_STATUS_CANCELLED, NULL, - NULL); + cq_expect_finished_with_status(v_client, tag(3), mode.expect_status, + mode.expect_details, NULL); cq_verify(v_client); grpc_call_destroy(c); @@ -142,9 +135,8 @@ static void test_cancel_after_invoke(grpc_end2end_test_config config, void grpc_end2end_tests(grpc_end2end_test_config config) { int i; - canceller cancellers[2] = {grpc_call_cancel, wait_for_deadline}; - for (i = 0; i < GPR_ARRAY_SIZE(cancellers); i++) { - test_cancel_after_invoke(config, cancellers[i]); + for (i = 0; i < GPR_ARRAY_SIZE(cancellation_modes); i++) { + test_cancel_after_invoke(config, cancellation_modes[i]); } } diff --git a/test/core/end2end/tests/cancel_before_invoke.c b/test/core/end2end/tests/cancel_before_invoke.c index d5edcd4ac17..f799cba71d6 100644 --- a/test/core/end2end/tests/cancel_before_invoke.c +++ b/test/core/end2end/tests/cancel_before_invoke.c @@ -44,10 +44,6 @@ #include #include "test/core/end2end/cq_verifier.h" -/* allow cancellation by either grpc_call_cancel, or by wait_for_deadline (which - * does nothing) */ -typedef grpc_call_error (*canceller)(grpc_call *call); - enum { TIMEOUT = 200000 }; static void *tag(gpr_intptr t) { return (void *)t; } diff --git a/test/core/end2end/tests/cancel_in_a_vacuum.c b/test/core/end2end/tests/cancel_in_a_vacuum.c index 0c684acf0a9..e4f9deecd1b 100644 --- a/test/core/end2end/tests/cancel_in_a_vacuum.c +++ b/test/core/end2end/tests/cancel_in_a_vacuum.c @@ -43,14 +43,7 @@ #include #include #include "test/core/end2end/cq_verifier.h" - -/* allow cancellation by either grpc_call_cancel, or by wait_for_deadline (which - * does nothing) */ -typedef grpc_call_error (*canceller)(grpc_call *call); - -static grpc_call_error wait_for_deadline(grpc_call *call) { - return GRPC_CALL_OK; -} +#include "test/core/end2end/tests/cancel_test_helpers.h" enum { TIMEOUT = 200000 }; @@ -110,7 +103,7 @@ static void end_test(grpc_end2end_test_fixture *f) { /* Cancel and do nothing */ static void test_cancel_in_a_vacuum(grpc_end2end_test_config config, - canceller call_cancel) { + cancellation_mode mode) { grpc_call *c; grpc_end2end_test_fixture f = begin_test(config, __FUNCTION__, NULL, NULL); gpr_timespec deadline = five_seconds_time(); @@ -119,7 +112,7 @@ static void test_cancel_in_a_vacuum(grpc_end2end_test_config config, c = grpc_channel_create_call(f.client, "/foo", "test.google.com", deadline); GPR_ASSERT(c); - GPR_ASSERT(GRPC_CALL_OK == call_cancel(c)); + GPR_ASSERT(GRPC_CALL_OK == mode.initiate_cancel(c)); grpc_call_destroy(c); @@ -130,9 +123,8 @@ static void test_cancel_in_a_vacuum(grpc_end2end_test_config config, void grpc_end2end_tests(grpc_end2end_test_config config) { int i; - canceller cancellers[2] = {grpc_call_cancel, wait_for_deadline}; - for (i = 0; i < GPR_ARRAY_SIZE(cancellers); i++) { - test_cancel_in_a_vacuum(config, cancellers[i]); + for (i = 0; i < GPR_ARRAY_SIZE(cancellation_modes); i++) { + test_cancel_in_a_vacuum(config, cancellation_modes[i]); } } diff --git a/test/core/end2end/tests/cancel_test_helpers.h b/test/core/end2end/tests/cancel_test_helpers.h new file mode 100644 index 00000000000..bc6bfa94248 --- /dev/null +++ b/test/core/end2end/tests/cancel_test_helpers.h @@ -0,0 +1,52 @@ +/* + * + * Copyright 2014, Google Inc. + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are + * met: + * + * * Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * * Redistributions in binary form must reproduce the above + * copyright notice, this list of conditions and the following disclaimer + * in the documentation and/or other materials provided with the + * distribution. + * * Neither the name of Google Inc. nor the names of its + * contributors may be used to endorse or promote products derived from + * this software without specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + * + */ + +#ifndef __GRPC_TEST_END2END_TESTS_CANCEL_TEST_HELPERS_H__ +#define __GRPC_TEST_END2END_TESTS_CANCEL_TEST_HELPERS_H__ + +typedef struct { + grpc_call_error (*initiate_cancel)(grpc_call *call); + grpc_status_code expect_status; + const char *expect_details; +} cancellation_mode; + +static grpc_call_error wait_for_deadline(grpc_call *call) { + return GRPC_CALL_OK; +} + +static const cancellation_mode cancellation_modes[] = { + {grpc_call_cancel, GRPC_STATUS_CANCELLED, NULL}, + {wait_for_deadline, GRPC_STATUS_DEADLINE_EXCEEDED, "Deadline Exceeded"}, +}; + +#endif diff --git a/test/cpp/end2end/end2end_test.cc b/test/cpp/end2end/end2end_test.cc index 3a1da68e47f..4dea77ea811 100644 --- a/test/cpp/end2end/end2end_test.cc +++ b/test/cpp/end2end/end2end_test.cc @@ -210,9 +210,7 @@ TEST_F(End2endTest, RpcDeadlineExpires) { std::chrono::system_clock::now() + std::chrono::microseconds(10); context.set_absolute_deadline(deadline); Status s = stub_->Echo(&context, request, &response); - // TODO(yangg) use correct error code when b/18793983 is fixed. - // EXPECT_EQ(StatusCode::DEADLINE_EXCEEDED, s.code()); - EXPECT_EQ(StatusCode::CANCELLED, s.code()); + EXPECT_EQ(StatusCode::DEADLINE_EXCEEDED, s.code()); } // Set a long but finite deadline.