From 1791df0a72cd6457f152172b65515196df6663fc Mon Sep 17 00:00:00 2001 From: Yash Tibrewal Date: Fri, 19 Jan 2018 18:23:20 -0800 Subject: [PATCH 1/9] Bad client changes to make large metadata test to work again --- CMakeLists.txt | 32 +++ Makefile | 24 ++ test/core/bad_client/bad_client.cc | 258 ++++++++++++------ test/core/bad_client/bad_client.h | 38 ++- test/core/bad_client/gen_build_yaml.py | 2 +- test/core/bad_client/generate_tests.bzl | 2 +- .../bad_client/tests/head_of_line_blocking.cc | 3 +- test/core/bad_client/tests/large_metadata.cc | 80 ++---- test/core/bad_client/tests/window_overflow.cc | 4 +- .../generated/sources_and_headers.json | 18 ++ tools/run_tests/generated/tests.json | 26 ++ 11 files changed, 332 insertions(+), 155 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index eed12052685..35f2c8c0fe3 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -388,6 +388,7 @@ add_dependencies(buildtests_c connection_prefix_bad_client_test) add_dependencies(buildtests_c head_of_line_blocking_bad_client_test) add_dependencies(buildtests_c headers_bad_client_test) add_dependencies(buildtests_c initial_settings_frame_bad_client_test) +add_dependencies(buildtests_c large_metadata_bad_client_test) add_dependencies(buildtests_c server_registered_method_bad_client_test) add_dependencies(buildtests_c simple_request_bad_client_test) add_dependencies(buildtests_c unknown_frame_bad_client_test) @@ -12820,6 +12821,37 @@ target_link_libraries(initial_settings_frame_bad_client_test endif (gRPC_BUILD_TESTS) if (gRPC_BUILD_TESTS) +add_executable(large_metadata_bad_client_test + test/core/bad_client/tests/large_metadata.cc +) + + +target_include_directories(large_metadata_bad_client_test + PRIVATE ${CMAKE_CURRENT_SOURCE_DIR} + PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/include + PRIVATE ${_gRPC_SSL_INCLUDE_DIR} + PRIVATE ${PROTOBUF_ROOT_DIR}/src + PRIVATE ${BENCHMARK_ROOT_DIR}/include + PRIVATE ${ZLIB_ROOT_DIR} + PRIVATE ${CMAKE_CURRENT_BINARY_DIR}/third_party/zlib + PRIVATE ${CARES_INCLUDE_DIR} + PRIVATE ${CMAKE_CURRENT_BINARY_DIR}/third_party/cares/cares + PRIVATE ${CMAKE_CURRENT_BINARY_DIR}/third_party/gflags/include +) + +target_link_libraries(large_metadata_bad_client_test + ${_gRPC_SSL_LIBRARIES} + ${_gRPC_ALLTARGETS_LIBRARIES} + bad_client_test + grpc_test_util_unsecure + grpc_unsecure + gpr_test_util + gpr +) + +endif (gRPC_BUILD_TESTS) +if (gRPC_BUILD_TESTS) + add_executable(server_registered_method_bad_client_test test/core/bad_client/tests/server_registered_method.cc ) diff --git a/Makefile b/Makefile index 38b40804d64..04f91d965db 100644 --- a/Makefile +++ b/Makefile @@ -1228,6 +1228,7 @@ connection_prefix_bad_client_test: $(BINDIR)/$(CONFIG)/connection_prefix_bad_cli head_of_line_blocking_bad_client_test: $(BINDIR)/$(CONFIG)/head_of_line_blocking_bad_client_test headers_bad_client_test: $(BINDIR)/$(CONFIG)/headers_bad_client_test initial_settings_frame_bad_client_test: $(BINDIR)/$(CONFIG)/initial_settings_frame_bad_client_test +large_metadata_bad_client_test: $(BINDIR)/$(CONFIG)/large_metadata_bad_client_test server_registered_method_bad_client_test: $(BINDIR)/$(CONFIG)/server_registered_method_bad_client_test simple_request_bad_client_test: $(BINDIR)/$(CONFIG)/simple_request_bad_client_test unknown_frame_bad_client_test: $(BINDIR)/$(CONFIG)/unknown_frame_bad_client_test @@ -1480,6 +1481,7 @@ buildtests_c: privatelibs_c \ $(BINDIR)/$(CONFIG)/head_of_line_blocking_bad_client_test \ $(BINDIR)/$(CONFIG)/headers_bad_client_test \ $(BINDIR)/$(CONFIG)/initial_settings_frame_bad_client_test \ + $(BINDIR)/$(CONFIG)/large_metadata_bad_client_test \ $(BINDIR)/$(CONFIG)/server_registered_method_bad_client_test \ $(BINDIR)/$(CONFIG)/simple_request_bad_client_test \ $(BINDIR)/$(CONFIG)/unknown_frame_bad_client_test \ @@ -2015,6 +2017,8 @@ test_c: buildtests_c $(Q) $(BINDIR)/$(CONFIG)/headers_bad_client_test || ( echo test headers_bad_client_test failed ; exit 1 ) $(E) "[RUN] Testing initial_settings_frame_bad_client_test" $(Q) $(BINDIR)/$(CONFIG)/initial_settings_frame_bad_client_test || ( echo test initial_settings_frame_bad_client_test failed ; exit 1 ) + $(E) "[RUN] Testing large_metadata_bad_client_test" + $(Q) $(BINDIR)/$(CONFIG)/large_metadata_bad_client_test || ( echo test large_metadata_bad_client_test failed ; exit 1 ) $(E) "[RUN] Testing server_registered_method_bad_client_test" $(Q) $(BINDIR)/$(CONFIG)/server_registered_method_bad_client_test || ( echo test server_registered_method_bad_client_test failed ; exit 1 ) $(E) "[RUN] Testing simple_request_bad_client_test" @@ -18647,6 +18651,26 @@ ifneq ($(NO_DEPS),true) endif +LARGE_METADATA_BAD_CLIENT_TEST_SRC = \ + test/core/bad_client/tests/large_metadata.cc \ + +LARGE_METADATA_BAD_CLIENT_TEST_OBJS = $(addprefix $(OBJDIR)/$(CONFIG)/, $(addsuffix .o, $(basename $(LARGE_METADATA_BAD_CLIENT_TEST_SRC)))) + + +$(BINDIR)/$(CONFIG)/large_metadata_bad_client_test: $(LARGE_METADATA_BAD_CLIENT_TEST_OBJS) $(LIBDIR)/$(CONFIG)/libbad_client_test.a $(LIBDIR)/$(CONFIG)/libgrpc_test_util_unsecure.a $(LIBDIR)/$(CONFIG)/libgrpc_unsecure.a $(LIBDIR)/$(CONFIG)/libgpr_test_util.a $(LIBDIR)/$(CONFIG)/libgpr.a + $(E) "[LD] Linking $@" + $(Q) mkdir -p `dirname $@` + $(Q) $(LD) $(LDFLAGS) $(LARGE_METADATA_BAD_CLIENT_TEST_OBJS) $(LIBDIR)/$(CONFIG)/libbad_client_test.a $(LIBDIR)/$(CONFIG)/libgrpc_test_util_unsecure.a $(LIBDIR)/$(CONFIG)/libgrpc_unsecure.a $(LIBDIR)/$(CONFIG)/libgpr_test_util.a $(LIBDIR)/$(CONFIG)/libgpr.a $(LDLIBS) -o $(BINDIR)/$(CONFIG)/large_metadata_bad_client_test + +$(OBJDIR)/$(CONFIG)/test/core/bad_client/tests/large_metadata.o: $(LIBDIR)/$(CONFIG)/libbad_client_test.a $(LIBDIR)/$(CONFIG)/libgrpc_test_util_unsecure.a $(LIBDIR)/$(CONFIG)/libgrpc_unsecure.a $(LIBDIR)/$(CONFIG)/libgpr_test_util.a $(LIBDIR)/$(CONFIG)/libgpr.a + +deps_large_metadata_bad_client_test: $(LARGE_METADATA_BAD_CLIENT_TEST_OBJS:.o=.dep) + +ifneq ($(NO_DEPS),true) +-include $(LARGE_METADATA_BAD_CLIENT_TEST_OBJS:.o=.dep) +endif + + SERVER_REGISTERED_METHOD_BAD_CLIENT_TEST_SRC = \ test/core/bad_client/tests/server_registered_method.cc \ diff --git a/test/core/bad_client/bad_client.cc b/test/core/bad_client/bad_client.cc index 4c1642aa5d4..07f340bcd19 100644 --- a/test/core/bad_client/bad_client.cc +++ b/test/core/bad_client/bad_client.cc @@ -34,25 +34,41 @@ #include "src/core/lib/support/string.h" #include "src/core/lib/surface/completion_queue.h" #include "src/core/lib/surface/server.h" +#include "test/core/end2end/cq_verifier.h" +#define MIN_HTTP2_FRAME_SIZE 9 + +/* connection preface and settings frame to be sent by the client */ +#define CONNECTION_PREFACE_FROM_CLIENT \ + "PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n" \ + "\x00\x00\x00\x04\x00\x00\x00\x00\x00" + +grpc_bad_client_arg connection_preface_arg = { + client_connection_preface_validator, CONNECTION_PREFACE_FROM_CLIENT, + sizeof(CONNECTION_PREFACE_FROM_CLIENT) - 1}; + +/* Args to provide to thread running server side validator */ typedef struct { grpc_server* server; grpc_completion_queue* cq; grpc_bad_client_server_side_validator validator; void* registered_method; gpr_event done_thd; - gpr_event done_write; } thd_args; +/* Run the server side validator and set done_thd once done */ static void thd_func(void* arg) { thd_args* a = (thd_args*)arg; - a->validator(a->server, a->cq, a->registered_method); + if (a->validator) { + a->validator(a->server, a->cq, a->registered_method); + } gpr_event_set(&a->done_thd, (void*)1); } -static void done_write(void* arg, grpc_error* error) { - thd_args* a = (thd_args*)arg; - gpr_event_set(&a->done_write, (void*)1); +/* Sets the done_write event */ +static void set_done_write(void* arg, grpc_error* error) { + gpr_event* set_done = (gpr_event*)arg; + gpr_event_set(set_done, (void*)1); } static void server_setup_transport(void* ts, grpc_transport* transport) { @@ -62,40 +78,108 @@ static void server_setup_transport(void* ts, grpc_transport* transport) { grpc_server_get_channel_args(a->server)); } -static void read_done(void* arg, grpc_error* error) { +/* Sets the read_done event */ +static void set_read_done(void* arg, grpc_error* error) { gpr_event* read_done = (gpr_event*)arg; gpr_event_set(read_done, (void*)1); } -void grpc_run_bad_client_test( - grpc_bad_client_server_side_validator server_validator, - grpc_bad_client_client_stream_validator client_validator, - const char* client_payload, size_t client_payload_length, uint32_t flags) { - grpc_endpoint_pair sfd; - thd_args a; - gpr_thd_id id; +/* Runs client side validators */ +void grpc_run_client_side_validators(grpc_bad_client_arg* arg, uint32_t flags, + grpc_endpoint_pair* sfd, + grpc_completion_queue* client_cq) { char* hex; - grpc_transport* transport; - grpc_slice slice = - grpc_slice_from_copied_buffer(client_payload, client_payload_length); - grpc_slice_buffer outgoing; - grpc_closure done_write_closure; - grpc_core::ExecCtx exec_ctx; - grpc_completion_queue* shutdown_cq; - - if (client_payload_length < 4 * 1024) { - hex = gpr_dump(client_payload, client_payload_length, + gpr_event done_write; + if (arg->client_payload_length < 4 * 1024) { + hex = gpr_dump(arg->client_payload, arg->client_payload_length, GPR_DUMP_HEX | GPR_DUMP_ASCII); - /* Add a debug log */ gpr_log(GPR_INFO, "TEST: %s", hex); - gpr_free(hex); } else { gpr_log(GPR_INFO, "TEST: (%" PRIdPTR " byte long string)", - client_payload_length); + arg->client_payload_length); } + grpc_slice slice = grpc_slice_from_copied_buffer(arg->client_payload, + arg->client_payload_length); + grpc_slice_buffer outgoing; + grpc_closure done_write_closure; + gpr_event_init(&done_write); + + grpc_slice_buffer_init(&outgoing); + grpc_slice_buffer_add(&outgoing, slice); + GRPC_CLOSURE_INIT(&done_write_closure, set_done_write, &done_write, + grpc_schedule_on_exec_ctx); + + /* Write data */ + grpc_endpoint_write(sfd->client, &outgoing, &done_write_closure); + grpc_core::ExecCtx::Get()->Flush(); + + /* Await completion, unless the request is large and write may not finish + * before the peer shuts down. */ + if (!(flags & GRPC_BAD_CLIENT_LARGE_REQUEST)) { + GPR_ASSERT( + gpr_event_wait(&done_write, grpc_timeout_seconds_to_deadline(5))); + } + + if (flags & GRPC_BAD_CLIENT_DISCONNECT) { + grpc_endpoint_shutdown( + sfd->client, GRPC_ERROR_CREATE_FROM_STATIC_STRING("Forced Disconnect")); + grpc_endpoint_destroy(sfd->client); + grpc_core::ExecCtx::Get()->Flush(); + sfd->client = nullptr; + } + + if (sfd->client != nullptr) { + /* Validate client stream, if requested. */ + if (arg->client_validator != nullptr) { + gpr_timespec deadline = grpc_timeout_seconds_to_deadline(5); + grpc_slice_buffer incoming; + grpc_slice_buffer_init(&incoming); + /* We may need to do multiple reads to read the complete server + * response. */ + while (true) { + gpr_event read_done_event; + gpr_event_init(&read_done_event); + grpc_closure read_done_closure; + GRPC_CLOSURE_INIT(&read_done_closure, set_read_done, &read_done_event, + grpc_schedule_on_exec_ctx); + grpc_endpoint_read(sfd->client, &incoming, &read_done_closure); + grpc_core::ExecCtx::Get()->Flush(); + do { + GPR_ASSERT(gpr_time_cmp(deadline, gpr_now(deadline.clock_type)) > 0); + /* Perform a cq next just to provide a thread that can read incoming + bytes on the client fd */ + GPR_ASSERT(grpc_completion_queue_next( + client_cq, grpc_timeout_milliseconds_to_deadline(100), + nullptr) + .type == GRPC_QUEUE_TIMEOUT); + } while (!gpr_event_get(&read_done_event)); + if (arg->client_validator(&incoming)) break; + gpr_log(GPR_INFO, + "client validator failed; trying additional read " + "in case we didn't get all the data"); + } + grpc_slice_buffer_destroy_internal(&incoming); + } + grpc_core::ExecCtx::Get()->Flush(); + } + GPR_ASSERT(gpr_event_wait(&done_write, grpc_timeout_seconds_to_deadline(1))); + grpc_slice_buffer_destroy_internal(&outgoing); + grpc_core::ExecCtx::Get()->Flush(); +} + +void grpc_run_bad_client_test( + grpc_bad_client_server_side_validator server_validator, + grpc_bad_client_arg args[], int num_args, uint32_t flags) { + grpc_endpoint_pair sfd; + thd_args a; + grpc_transport* transport; + grpc_core::ExecCtx exec_ctx; + grpc_completion_queue* shutdown_cq; + grpc_completion_queue* client_cq; + /* Init grpc */ grpc_init(); @@ -105,9 +189,8 @@ void grpc_run_bad_client_test( /* Create server, completion events */ a.server = grpc_server_create(nullptr, nullptr); a.cq = grpc_completion_queue_create_for_next(nullptr); - gpr_event_init(&a.done_thd); - gpr_event_init(&a.done_write); - a.validator = server_validator; + client_cq = grpc_completion_queue_create_for_next(nullptr); + grpc_server_register_completion_queue(a.server, a.cq, nullptr); a.registered_method = grpc_server_register_method(a.server, GRPC_BAD_CLIENT_REGISTERED_METHOD, @@ -118,80 +201,32 @@ void grpc_run_bad_client_test( server_setup_transport(&a, transport); grpc_chttp2_transport_start_reading(transport, nullptr, nullptr); - /* Bind everything into the same pollset */ - grpc_endpoint_add_to_pollset(sfd.client, grpc_cq_pollset(a.cq)); + /* Bind fds to pollsets */ + grpc_endpoint_add_to_pollset(sfd.client, grpc_cq_pollset(client_cq)); grpc_endpoint_add_to_pollset(sfd.server, grpc_cq_pollset(a.cq)); /* Check a ground truth */ GPR_ASSERT(grpc_server_has_open_connections(a.server)); + gpr_thd_id id; + gpr_event_init(&a.done_thd); + a.validator = server_validator; /* Start validator */ gpr_thd_new(&id, "grpc_bad_client", thd_func, &a, nullptr); - - grpc_slice_buffer_init(&outgoing); - grpc_slice_buffer_add(&outgoing, slice); - GRPC_CLOSURE_INIT(&done_write_closure, done_write, &a, - grpc_schedule_on_exec_ctx); - - /* Write data */ - grpc_endpoint_write(sfd.client, &outgoing, &done_write_closure); - grpc_core::ExecCtx::Get()->Flush(); - - /* Await completion, unless the request is large and write may not finish - * before the peer shuts down. */ - if (!(flags & GRPC_BAD_CLIENT_LARGE_REQUEST)) { - GPR_ASSERT( - gpr_event_wait(&a.done_write, grpc_timeout_seconds_to_deadline(5))); + for (int i = 0; i < num_args; i++) { + grpc_run_client_side_validators(&args[i], flags, &sfd, client_cq); } + /* Wait for server thread to finish */ + GPR_ASSERT(gpr_event_wait(&a.done_thd, grpc_timeout_seconds_to_deadline(1))); - if (flags & GRPC_BAD_CLIENT_DISCONNECT) { - grpc_endpoint_shutdown( - sfd.client, GRPC_ERROR_CREATE_FROM_STATIC_STRING("Forced Disconnect")); - grpc_endpoint_destroy(sfd.client); - grpc_core::ExecCtx::Get()->Flush(); - sfd.client = nullptr; - } - - GPR_ASSERT(gpr_event_wait(&a.done_thd, grpc_timeout_seconds_to_deadline(5))); - + /* Shutdown. */ if (sfd.client != nullptr) { - // Validate client stream, if requested. - if (client_validator != nullptr) { - gpr_timespec deadline = grpc_timeout_seconds_to_deadline(5); - grpc_slice_buffer incoming; - grpc_slice_buffer_init(&incoming); - // We may need to do multiple reads to read the complete server response. - while (true) { - gpr_event read_done_event; - gpr_event_init(&read_done_event); - grpc_closure read_done_closure; - GRPC_CLOSURE_INIT(&read_done_closure, read_done, &read_done_event, - grpc_schedule_on_exec_ctx); - grpc_endpoint_read(sfd.client, &incoming, &read_done_closure); - grpc_core::ExecCtx::Get()->Flush(); - do { - GPR_ASSERT(gpr_time_cmp(deadline, gpr_now(deadline.clock_type)) > 0); - GPR_ASSERT( - grpc_completion_queue_next( - a.cq, grpc_timeout_milliseconds_to_deadline(100), nullptr) - .type == GRPC_QUEUE_TIMEOUT); - } while (!gpr_event_get(&read_done_event)); - if (client_validator(&incoming)) break; - gpr_log(GPR_INFO, - "client validator failed; trying additional read " - "in case we didn't get all the data"); - } - grpc_slice_buffer_destroy_internal(&incoming); - } - // Shutdown. grpc_endpoint_shutdown( sfd.client, GRPC_ERROR_CREATE_FROM_STATIC_STRING("Test Shutdown")); grpc_endpoint_destroy(sfd.client); grpc_core::ExecCtx::Get()->Flush(); } - GPR_ASSERT( - gpr_event_wait(&a.done_write, grpc_timeout_seconds_to_deadline(1))); shutdown_cq = grpc_completion_queue_create_for_pluck(nullptr); grpc_server_shutdown_and_notify(a.server, shutdown_cq, nullptr); GPR_ASSERT(grpc_completion_queue_pluck(shutdown_cq, nullptr, @@ -201,7 +236,52 @@ void grpc_run_bad_client_test( grpc_completion_queue_destroy(shutdown_cq); grpc_server_destroy(a.server); grpc_completion_queue_destroy(a.cq); - grpc_slice_buffer_destroy_internal(&outgoing); - + grpc_completion_queue_destroy(client_cq); grpc_shutdown(); } + +bool client_connection_preface_validator(grpc_slice_buffer* incoming) { + if (incoming->count < 1) { + return false; + } + grpc_slice slice = incoming->slices[0]; + /* There should be atleast a settings frame present */ + if (GRPC_SLICE_LENGTH(slice) < MIN_HTTP2_FRAME_SIZE) { + return false; + } + const uint8_t* p = GRPC_SLICE_START_PTR(slice); + /* Check the frame type (SETTINGS) */ + if (*(p + 3) != 4) { + return false; + } + return true; +} + +static void* tag(intptr_t t) { return (void*)t; } + +void server_verifier_request_call(grpc_server* server, + grpc_completion_queue* cq, + void* registered_method) { + grpc_call_error error; + grpc_call* s; + grpc_call_details call_details; + cq_verifier* cqv = cq_verifier_create(cq); + grpc_metadata_array request_metadata_recv; + + grpc_call_details_init(&call_details); + grpc_metadata_array_init(&request_metadata_recv); + + error = grpc_server_request_call(server, &s, &call_details, + &request_metadata_recv, cq, cq, tag(101)); + GPR_ASSERT(GRPC_CALL_OK == error); + CQ_EXPECT_COMPLETION(cqv, tag(101), 1); + cq_verify(cqv); + + GPR_ASSERT(0 == grpc_slice_str_cmp(call_details.host, "localhost")); + GPR_ASSERT(0 == grpc_slice_str_cmp(call_details.method, "/foo/bar")); + + grpc_metadata_array_destroy(&request_metadata_recv); + grpc_call_details_destroy(&call_details); + grpc_call_unref(s); + cq_verifier_destroy(cqv); +} diff --git a/test/core/bad_client/bad_client.h b/test/core/bad_client/bad_client.h index d3abfac2aa5..54bf0229c53 100644 --- a/test/core/bad_client/bad_client.h +++ b/test/core/bad_client/bad_client.h @@ -28,30 +28,52 @@ #define GRPC_BAD_CLIENT_REGISTERED_METHOD "/registered/bar" #define GRPC_BAD_CLIENT_REGISTERED_HOST "localhost" +/* The server side validator function to run */ typedef void (*grpc_bad_client_server_side_validator)(grpc_server* server, grpc_completion_queue* cq, void* registered_method); -// Returns false if we need to read more data. +/* Returns false if we need to read more data. */ typedef bool (*grpc_bad_client_client_stream_validator)( grpc_slice_buffer* incoming); +struct grpc_bad_client_arg { + grpc_bad_client_client_stream_validator client_validator; + const char* client_payload; + size_t client_payload_length; +}; + #define GRPC_BAD_CLIENT_DISCONNECT 1 #define GRPC_BAD_CLIENT_LARGE_REQUEST 2 /* Test runner. - Create a server, and send client_payload to it as bytes from a client. - Execute server_validator in a separate thread to assert that the bytes are - handled as expected. */ + Create a server, and for each arg in args send client_payload. For each + payload, run client_validator to make sure that the response is as expected. + Also execute server_validator in a separate thread to assert that the bytes + are handled as expected. */ void grpc_run_bad_client_test( grpc_bad_client_server_side_validator server_validator, - grpc_bad_client_client_stream_validator client_validator, - const char* client_payload, size_t client_payload_length, uint32_t flags); + grpc_bad_client_arg args[], int num_args, uint32_t flags); + +#define COMBINE1(X, Y) X##Y +#define COMBINE(X, Y) COMBINE1(X, Y) #define GRPC_RUN_BAD_CLIENT_TEST(server_validator, client_validator, payload, \ flags) \ - grpc_run_bad_client_test(server_validator, client_validator, payload, \ - sizeof(payload) - 1, flags) + grpc_bad_client_arg COMBINE(bca, __LINE__) = {client_validator, payload, \ + sizeof(payload) - 1}; \ + grpc_run_bad_client_test(server_validator, &COMBINE(bca, __LINE__), 1, flags) +/* Client side validator for connection preface from server */ +bool client_connection_preface_validator(grpc_slice_buffer* incoming); + +/* Sends a connection preface from the client with an empty settings frame */ +extern grpc_bad_client_arg connection_preface_arg; + +/* Server side verifier function that performs a + * single grpc_server_request_call */ +void server_verifier_request_call(grpc_server* server, + grpc_completion_queue* cq, + void* registered_method); #endif /* GRPC_TEST_CORE_BAD_CLIENT_BAD_CLIENT_H */ diff --git a/test/core/bad_client/gen_build_yaml.py b/test/core/bad_client/gen_build_yaml.py index 14c8a27334b..a8fd7772161 100755 --- a/test/core/bad_client/gen_build_yaml.py +++ b/test/core/bad_client/gen_build_yaml.py @@ -30,7 +30,7 @@ BAD_CLIENT_TESTS = { 'headers': default_test_options._replace(cpu_cost=0.2), 'initial_settings_frame': default_test_options._replace(cpu_cost=0.2), 'head_of_line_blocking': default_test_options, - # 'large_metadata': default_test_options, #disabling as per issue #11745 + 'large_metadata': default_test_options, 'server_registered_method': default_test_options, 'simple_request': default_test_options, 'window_overflow': default_test_options, diff --git a/test/core/bad_client/generate_tests.bzl b/test/core/bad_client/generate_tests.bzl index 022edf3ff34..b595defb8c3 100755 --- a/test/core/bad_client/generate_tests.bzl +++ b/test/core/bad_client/generate_tests.bzl @@ -28,7 +28,7 @@ BAD_CLIENT_TESTS = { 'headers': test_options(), 'initial_settings_frame': test_options(), 'head_of_line_blocking': test_options(), - # 'large_metadata': test_options(), # disabling as per issue #11745 + 'large_metadata': test_options(), 'server_registered_method': test_options(), 'simple_request': test_options(), 'window_overflow': test_options(), diff --git a/test/core/bad_client/tests/head_of_line_blocking.cc b/test/core/bad_client/tests/head_of_line_blocking.cc index f56c4d71dd6..7b1940d4803 100644 --- a/test/core/bad_client/tests/head_of_line_blocking.cc +++ b/test/core/bad_client/tests/head_of_line_blocking.cc @@ -131,7 +131,8 @@ int main(int argc, char** argv) { addbuf(hdr, sizeof(hdr)); addbuf(msg, FRAME_SIZE); } - grpc_run_bad_client_test(verifier, nullptr, g_buffer, g_count, 0); + grpc_bad_client_arg bca = {nullptr, g_buffer, g_count}; + grpc_run_bad_client_test(verifier, &bca, 1, 0); gpr_free(g_buffer); grpc_shutdown(); diff --git a/test/core/bad_client/tests/large_metadata.cc b/test/core/bad_client/tests/large_metadata.cc index 1ce0f28967c..600d0006eb0 100644 --- a/test/core/bad_client/tests/large_metadata.cc +++ b/test/core/bad_client/tests/large_metadata.cc @@ -31,23 +31,20 @@ // be longer than the C99 string literal limit. Instead, we dynamically // construct it by adding the large headers one at a time. -#define PFX_TOO_MUCH_METADATA_FROM_CLIENT_PREFIX_STR \ - "PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n" /* settings frame */ \ - "\x00\x00\x00\x04\x00\x00\x00\x00\x00" /* headers: generated from \ - large_metadata.headers in this \ - directory */ \ - "\x00\x00\x00\x04\x01\x00\x00\x00\x00" \ - "\x00" \ - "5{\x01\x05\x00\x00\x00\x01" \ - "\x10\x05:path\x08/foo/bar" \ - "\x10\x07:scheme\x04http" \ - "\x10\x07:method\x04POST" \ - "\x10\x0a:authority\x09localhost" \ - "\x10\x0c" \ - "content-type\x10" \ - "application/grpc" \ - "\x10\x14grpc-accept-encoding\x15identity,deflate,gzip" \ - "\x10\x02te\x08trailers" \ +/* headers: generated from large_metadata.headers in this directory */ +#define PFX_TOO_MUCH_METADATA_FROM_CLIENT_REQUEST \ + "\x00\x00\x00\x04\x01\x00\x00\x00\x00" \ + "\x00" \ + "5{\x01\x05\x00\x00\x00\x01" \ + "\x10\x05:path\x08/foo/bar" \ + "\x10\x07:scheme\x04http" \ + "\x10\x07:method\x04POST" \ + "\x10\x0a:authority\x09localhost" \ + "\x10\x0c" \ + "content-type\x10" \ + "application/grpc" \ + "\x10\x14grpc-accept-encoding\x15identity,deflate,gzip" \ + "\x10\x02te\x08trailers" \ "\x10\x0auser-agent\"bad-client grpc-c/0.12.0.0 (linux)" // Each large-metadata header is constructed from these start and end @@ -65,8 +62,8 @@ // The number of headers we're adding and the total size of the client // payload. #define NUM_HEADERS 46 -#define PFX_TOO_MUCH_METADATA_FROM_CLIENT_PAYLOAD_SIZE \ - ((sizeof(PFX_TOO_MUCH_METADATA_FROM_CLIENT_PREFIX_STR) - 1) + \ +#define TOO_MUCH_METADATA_FROM_CLIENT_REQUEST_SIZE \ + ((sizeof(PFX_TOO_MUCH_METADATA_FROM_CLIENT_REQUEST) - 1) + \ (NUM_HEADERS * PFX_TOO_MUCH_METADATA_FROM_CLIENT_HEADER_SIZE) + 1) #define PFX_TOO_MUCH_METADATA_FROM_SERVER_STR \ @@ -95,32 +92,6 @@ static void* tag(intptr_t t) { return (void*)t; } -static void server_verifier(grpc_server* server, grpc_completion_queue* cq, - void* registered_method) { - grpc_call_error error; - grpc_call* s; - grpc_call_details call_details; - cq_verifier* cqv = cq_verifier_create(cq); - grpc_metadata_array request_metadata_recv; - - grpc_call_details_init(&call_details); - grpc_metadata_array_init(&request_metadata_recv); - - error = grpc_server_request_call(server, &s, &call_details, - &request_metadata_recv, cq, cq, tag(101)); - GPR_ASSERT(GRPC_CALL_OK == error); - CQ_EXPECT_COMPLETION(cqv, tag(101), 1); - cq_verify(cqv); - - GPR_ASSERT(0 == grpc_slice_str_cmp(call_details.host, "localhost")); - GPR_ASSERT(0 == grpc_slice_str_cmp(call_details.method, "/foo/bar")); - - grpc_metadata_array_destroy(&request_metadata_recv); - grpc_call_details_destroy(&call_details); - grpc_call_unref(s); - cq_verifier_destroy(cqv); -} - static void server_verifier_sends_too_much_metadata(grpc_server* server, grpc_completion_queue* cq, void* registered_method) { @@ -222,19 +193,22 @@ int main(int argc, char** argv) { size_t headers_len; const char* client_headers = gpr_strvec_flatten(&headers, &headers_len); gpr_strvec_destroy(&headers); - char client_payload[PFX_TOO_MUCH_METADATA_FROM_CLIENT_PAYLOAD_SIZE] = - PFX_TOO_MUCH_METADATA_FROM_CLIENT_PREFIX_STR; - memcpy( - client_payload + sizeof(PFX_TOO_MUCH_METADATA_FROM_CLIENT_PREFIX_STR) - 1, - client_headers, headers_len); - GRPC_RUN_BAD_CLIENT_TEST(server_verifier, client_validator, client_payload, - 0); + char client_payload[TOO_MUCH_METADATA_FROM_CLIENT_REQUEST_SIZE] = + PFX_TOO_MUCH_METADATA_FROM_CLIENT_REQUEST; + memcpy(client_payload + sizeof(PFX_TOO_MUCH_METADATA_FROM_CLIENT_REQUEST) - 1, + client_headers, headers_len); + grpc_bad_client_arg args[2]; + args[0] = connection_preface_arg; + args[1].client_validator = client_validator; + args[1].client_payload = client_payload; + args[1].client_payload_length = sizeof(client_payload) - 1; + + grpc_run_bad_client_test(server_verifier_request_call, args, 2, 0); gpr_free((void*)client_headers); // Test sending more metadata than the client will accept. GRPC_RUN_BAD_CLIENT_TEST(server_verifier_sends_too_much_metadata, client_validator, PFX_TOO_MUCH_METADATA_FROM_SERVER_STR, 0); - return 0; } diff --git a/test/core/bad_client/tests/window_overflow.cc b/test/core/bad_client/tests/window_overflow.cc index ed8279c9519..3c3b733f3f6 100644 --- a/test/core/bad_client/tests/window_overflow.cc +++ b/test/core/bad_client/tests/window_overflow.cc @@ -92,8 +92,8 @@ int main(int argc, char** argv) { addbuf(message, sizeof(message)); } } - grpc_run_bad_client_test(verifier, nullptr, g_buffer, g_count, - GRPC_BAD_CLIENT_LARGE_REQUEST); + grpc_bad_client_arg bca = {nullptr, g_buffer, g_count}; + grpc_run_bad_client_test(verifier, &bca, 1, GRPC_BAD_CLIENT_LARGE_REQUEST); gpr_free(g_buffer); grpc_shutdown(); diff --git a/tools/run_tests/generated/sources_and_headers.json b/tools/run_tests/generated/sources_and_headers.json index d432bd0e537..0f14b10a188 100644 --- a/tools/run_tests/generated/sources_and_headers.json +++ b/tools/run_tests/generated/sources_and_headers.json @@ -4957,6 +4957,24 @@ "third_party": false, "type": "target" }, + { + "deps": [ + "bad_client_test", + "gpr", + "gpr_test_util", + "grpc_test_util_unsecure", + "grpc_unsecure" + ], + "headers": [], + "is_filegroup": false, + "language": "c", + "name": "large_metadata_bad_client_test", + "src": [ + "test/core/bad_client/tests/large_metadata.cc" + ], + "third_party": false, + "type": "target" + }, { "deps": [ "bad_client_test", diff --git a/tools/run_tests/generated/tests.json b/tools/run_tests/generated/tests.json index 98517cba2e7..92b71da7579 100644 --- a/tools/run_tests/generated/tests.json +++ b/tools/run_tests/generated/tests.json @@ -4680,6 +4680,32 @@ ], "uses_polling": true }, + { + "args": [], + "benchmark": false, + "ci_platforms": [ + "linux", + "mac", + "posix", + "windows" + ], + "cpu_cost": 1.0, + "exclude_configs": [], + "exclude_iomgrs": [ + "uv" + ], + "flaky": false, + "gtest": false, + "language": "c", + "name": "large_metadata_bad_client_test", + "platforms": [ + "linux", + "mac", + "posix", + "windows" + ], + "uses_polling": true + }, { "args": [], "benchmark": false, From eea1d9ba70b5371ccc0131ff534f7ed4fefded91 Mon Sep 17 00:00:00 2001 From: Yash Tibrewal Date: Mon, 22 Jan 2018 12:14:56 -0800 Subject: [PATCH 2/9] nullptr check --- test/core/bad_client/bad_client.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/core/bad_client/bad_client.cc b/test/core/bad_client/bad_client.cc index 07f340bcd19..0c4deaf41ce 100644 --- a/test/core/bad_client/bad_client.cc +++ b/test/core/bad_client/bad_client.cc @@ -59,7 +59,7 @@ typedef struct { /* Run the server side validator and set done_thd once done */ static void thd_func(void* arg) { thd_args* a = (thd_args*)arg; - if (a->validator) { + if (a->validator != nullptr) { a->validator(a->server, a->cq, a->registered_method); } gpr_event_set(&a->done_thd, (void*)1); From 0d8b21e1000f6e3e1771234573e8d577762db681 Mon Sep 17 00:00:00 2001 From: Yash Tibrewal Date: Tue, 23 Jan 2018 19:46:42 -0800 Subject: [PATCH 3/9] Add helper function for rst_stream --- test/core/bad_client/bad_client.cc | 67 ++++++++++++++----- test/core/bad_client/bad_client.h | 36 ++++++---- .../bad_client/tests/head_of_line_blocking.cc | 2 +- test/core/bad_client/tests/large_metadata.cc | 41 +----------- test/core/bad_client/tests/window_overflow.cc | 9 +-- 5 files changed, 80 insertions(+), 75 deletions(-) diff --git a/test/core/bad_client/bad_client.cc b/test/core/bad_client/bad_client.cc index 0c4deaf41ce..7db77885b1f 100644 --- a/test/core/bad_client/bad_client.cc +++ b/test/core/bad_client/bad_client.cc @@ -38,15 +38,6 @@ #define MIN_HTTP2_FRAME_SIZE 9 -/* connection preface and settings frame to be sent by the client */ -#define CONNECTION_PREFACE_FROM_CLIENT \ - "PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n" \ - "\x00\x00\x00\x04\x00\x00\x00\x00\x00" - -grpc_bad_client_arg connection_preface_arg = { - client_connection_preface_validator, CONNECTION_PREFACE_FROM_CLIENT, - sizeof(CONNECTION_PREFACE_FROM_CLIENT) - 1}; - /* Args to provide to thread running server side validator */ typedef struct { grpc_server* server; @@ -67,8 +58,9 @@ static void thd_func(void* arg) { /* Sets the done_write event */ static void set_done_write(void* arg, grpc_error* error) { - gpr_event* set_done = (gpr_event*)arg; - gpr_event_set(set_done, (void*)1); + gpr_log(GPR_INFO, "done writing"); + gpr_event* done_write = (gpr_event*)arg; + gpr_event_set(done_write, (void*)1); } static void server_setup_transport(void* ts, grpc_transport* transport) { @@ -84,10 +76,10 @@ static void set_read_done(void* arg, grpc_error* error) { gpr_event_set(read_done, (void*)1); } -/* Runs client side validators */ -void grpc_run_client_side_validators(grpc_bad_client_arg* arg, uint32_t flags, - grpc_endpoint_pair* sfd, - grpc_completion_queue* client_cq) { +/* Runs client side validator */ +void grpc_run_client_side_validator(grpc_bad_client_arg* arg, uint32_t flags, + grpc_endpoint_pair* sfd, + grpc_completion_queue* client_cq) { char* hex; gpr_event done_write; if (arg->client_payload_length < 4 * 1024) { @@ -156,7 +148,7 @@ void grpc_run_client_side_validators(grpc_bad_client_arg* arg, uint32_t flags, nullptr) .type == GRPC_QUEUE_TIMEOUT); } while (!gpr_event_get(&read_done_event)); - if (arg->client_validator(&incoming)) break; + if (arg->client_validator(&incoming, arg->client_validator_arg)) break; gpr_log(GPR_INFO, "client validator failed; trying additional read " "in case we didn't get all the data"); @@ -214,7 +206,7 @@ void grpc_run_bad_client_test( /* Start validator */ gpr_thd_new(&id, "grpc_bad_client", thd_func, &a, nullptr); for (int i = 0; i < num_args; i++) { - grpc_run_client_side_validators(&args[i], flags, &sfd, client_cq); + grpc_run_client_side_validator(&args[i], flags, &sfd, client_cq); } /* Wait for server thread to finish */ GPR_ASSERT(gpr_event_wait(&a.done_thd, grpc_timeout_seconds_to_deadline(1))); @@ -240,7 +232,8 @@ void grpc_run_bad_client_test( grpc_shutdown(); } -bool client_connection_preface_validator(grpc_slice_buffer* incoming) { +bool client_connection_preface_validator(grpc_slice_buffer* incoming, + void* arg) { if (incoming->count < 1) { return false; } @@ -257,6 +250,44 @@ bool client_connection_preface_validator(grpc_slice_buffer* incoming) { return true; } +/* connection preface and settings frame to be sent by the client */ +#define CONNECTION_PREFACE_FROM_CLIENT \ + "PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n" \ + "\x00\x00\x00\x04\x00\x00\x00\x00\x00" + +grpc_bad_client_arg connection_preface_arg = { + client_connection_preface_validator, nullptr, + CONNECTION_PREFACE_FROM_CLIENT, sizeof(CONNECTION_PREFACE_FROM_CLIENT) - 1}; + +bool rst_stream_client_validator(grpc_slice_buffer* incoming, void* arg) { + // Get last frame from incoming slice buffer. + grpc_slice_buffer last_frame_buffer; + grpc_slice_buffer_init(&last_frame_buffer); + grpc_slice_buffer_trim_end(incoming, 13, &last_frame_buffer); + GPR_ASSERT(last_frame_buffer.count == 1); + grpc_slice last_frame = last_frame_buffer.slices[0]; + + const uint8_t* p = GRPC_SLICE_START_PTR(last_frame); + bool success = + // Length == 4 + *p++ != 0 || *p++ != 0 || *p++ != 4 || + // Frame type (RST_STREAM) + *p++ != 3 || + // Flags + *p++ != 0 || + // Stream ID. + *p++ != 0 || *p++ != 0 || *p++ != 0 || *p++ != 1 || + // Payload (error code) + *p++ == 0 || *p++ == 0 || *p++ == 0 || *p == 0 || *p == 11; + + if (!success) { + gpr_log(GPR_INFO, "client expected RST_STREAM frame, not found"); + } + + grpc_slice_buffer_destroy(&last_frame_buffer); + return success; +} + static void* tag(intptr_t t) { return (void*)t; } void server_verifier_request_call(grpc_server* server, diff --git a/test/core/bad_client/bad_client.h b/test/core/bad_client/bad_client.h index 54bf0229c53..bf73a788866 100644 --- a/test/core/bad_client/bad_client.h +++ b/test/core/bad_client/bad_client.h @@ -35,10 +35,11 @@ typedef void (*grpc_bad_client_server_side_validator)(grpc_server* server, /* Returns false if we need to read more data. */ typedef bool (*grpc_bad_client_client_stream_validator)( - grpc_slice_buffer* incoming); + grpc_slice_buffer* incoming, void* arg); struct grpc_bad_client_arg { grpc_bad_client_client_stream_validator client_validator; + void* client_validator_arg; const char* client_payload; size_t client_payload_length; }; @@ -47,11 +48,12 @@ struct grpc_bad_client_arg { #define GRPC_BAD_CLIENT_LARGE_REQUEST 2 /* Test runner. - - Create a server, and for each arg in args send client_payload. For each - payload, run client_validator to make sure that the response is as expected. - Also execute server_validator in a separate thread to assert that the bytes - are handled as expected. */ + * + * Create a server, and for each arg in \a args send client_payload. For each + * payload, run client_validator to make sure that the response is as expected. + * Also execute \a server_validator in a separate thread to assert that the + * bytes are handled as expected. + */ void grpc_run_bad_client_test( grpc_bad_client_server_side_validator server_validator, grpc_bad_client_arg args[], int num_args, uint32_t flags); @@ -59,20 +61,28 @@ void grpc_run_bad_client_test( #define COMBINE1(X, Y) X##Y #define COMBINE(X, Y) COMBINE1(X, Y) -#define GRPC_RUN_BAD_CLIENT_TEST(server_validator, client_validator, payload, \ - flags) \ - grpc_bad_client_arg COMBINE(bca, __LINE__) = {client_validator, payload, \ - sizeof(payload) - 1}; \ +#define GRPC_RUN_BAD_CLIENT_TEST(server_validator, client_validator, payload, \ + flags) \ + grpc_bad_client_arg COMBINE(bca, __LINE__) = {client_validator, nullptr, \ + payload, sizeof(payload) - 1}; \ grpc_run_bad_client_test(server_validator, &COMBINE(bca, __LINE__), 1, flags) -/* Client side validator for connection preface from server */ -bool client_connection_preface_validator(grpc_slice_buffer* incoming); +/* Helper validator functions */ +/* Client side validator for connection preface from server. \a arg is unused */ +bool client_connection_preface_validator(grpc_slice_buffer* incoming, + void* arg); +/* Client side validator for checking if reset stream is present at the end + * of the buffer. \a arg is unused. + */ +bool rst_stream_client_validator(grpc_slice_buffer* incoming, void* arg); + +/* Helper grpc_bad_client_arg arguments for direct use */ /* Sends a connection preface from the client with an empty settings frame */ extern grpc_bad_client_arg connection_preface_arg; /* Server side verifier function that performs a - * single grpc_server_request_call */ + * single grpc_server_request_call */ void server_verifier_request_call(grpc_server* server, grpc_completion_queue* cq, void* registered_method); diff --git a/test/core/bad_client/tests/head_of_line_blocking.cc b/test/core/bad_client/tests/head_of_line_blocking.cc index 7b1940d4803..8668e091b60 100644 --- a/test/core/bad_client/tests/head_of_line_blocking.cc +++ b/test/core/bad_client/tests/head_of_line_blocking.cc @@ -131,7 +131,7 @@ int main(int argc, char** argv) { addbuf(hdr, sizeof(hdr)); addbuf(msg, FRAME_SIZE); } - grpc_bad_client_arg bca = {nullptr, g_buffer, g_count}; + grpc_bad_client_arg bca = {nullptr, nullptr, g_buffer, g_count}; grpc_run_bad_client_test(verifier, &bca, 1, 0); gpr_free(g_buffer); grpc_shutdown(); diff --git a/test/core/bad_client/tests/large_metadata.cc b/test/core/bad_client/tests/large_metadata.cc index 600d0006eb0..429af8c9558 100644 --- a/test/core/bad_client/tests/large_metadata.cc +++ b/test/core/bad_client/tests/large_metadata.cc @@ -138,43 +138,6 @@ static void server_verifier_sends_too_much_metadata(grpc_server* server, cq_verifier_destroy(cqv); } -static bool client_validator(grpc_slice_buffer* incoming) { - for (size_t i = 0; i < incoming->count; ++i) { - const char* s = (const char*)GRPC_SLICE_START_PTR(incoming->slices[i]); - char* hex = gpr_dump(s, GRPC_SLICE_LENGTH(incoming->slices[i]), - GPR_DUMP_HEX | GPR_DUMP_ASCII); - gpr_log(GPR_INFO, "RESPONSE SLICE %" PRIdPTR ": %s", i, hex); - gpr_free(hex); - } - - // Get last frame from incoming slice buffer. - grpc_slice_buffer last_frame_buffer; - grpc_slice_buffer_init(&last_frame_buffer); - grpc_slice_buffer_trim_end(incoming, 13, &last_frame_buffer); - GPR_ASSERT(last_frame_buffer.count == 1); - grpc_slice last_frame = last_frame_buffer.slices[0]; - - const uint8_t* p = GRPC_SLICE_START_PTR(last_frame); - bool success = - // Length == 4 - *p++ != 0 || *p++ != 0 || *p++ != 4 || - // Frame type (RST_STREAM) - *p++ != 3 || - // Flags - *p++ != 0 || - // Stream ID. - *p++ != 0 || *p++ != 0 || *p++ != 0 || *p++ != 1 || - // Payload (error code) - *p++ == 0 || *p++ == 0 || *p++ == 0 || *p == 0 || *p == 11; - - if (!success) { - gpr_log(GPR_INFO, "client expected RST_STREAM frame, not found"); - } - - grpc_slice_buffer_destroy(&last_frame_buffer); - return success; -} - int main(int argc, char** argv) { int i; @@ -199,7 +162,7 @@ int main(int argc, char** argv) { client_headers, headers_len); grpc_bad_client_arg args[2]; args[0] = connection_preface_arg; - args[1].client_validator = client_validator; + args[1].client_validator = rst_stream_client_validator; args[1].client_payload = client_payload; args[1].client_payload_length = sizeof(client_payload) - 1; @@ -208,7 +171,7 @@ int main(int argc, char** argv) { // Test sending more metadata than the client will accept. GRPC_RUN_BAD_CLIENT_TEST(server_verifier_sends_too_much_metadata, - client_validator, + rst_stream_client_validator, PFX_TOO_MUCH_METADATA_FROM_SERVER_STR, 0); return 0; } diff --git a/test/core/bad_client/tests/window_overflow.cc b/test/core/bad_client/tests/window_overflow.cc index 3c3b733f3f6..3fda2e27b42 100644 --- a/test/core/bad_client/tests/window_overflow.cc +++ b/test/core/bad_client/tests/window_overflow.cc @@ -26,8 +26,7 @@ #include "src/core/lib/surface/server.h" #define PFX_STR \ - "PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n" \ - "\x00\x00\x00\x04\x00\x00\x00\x00\x00" /* settings frame */ \ + "\x00\x00\x00\x04\x01\x00\x00\x00\x00" \ "\x00\x00\xc9\x01\x04\x00\x00\x00\x01" /* headers: generated from \ simple_request.headers in this \ directory */ \ @@ -92,8 +91,10 @@ int main(int argc, char** argv) { addbuf(message, sizeof(message)); } } - grpc_bad_client_arg bca = {nullptr, g_buffer, g_count}; - grpc_run_bad_client_test(verifier, &bca, 1, GRPC_BAD_CLIENT_LARGE_REQUEST); + grpc_bad_client_arg bca[2]; + bca[0] = connection_preface_arg; + bca[1] = {rst_stream_client_validator, nullptr, g_buffer, g_count}; + grpc_run_bad_client_test(verifier, bca, 2, GRPC_BAD_CLIENT_LARGE_REQUEST); gpr_free(g_buffer); grpc_shutdown(); From 7cbbee745e84d714f826e54fdbc93499341e7fa3 Mon Sep 17 00:00:00 2001 From: Yash Tibrewal Date: Wed, 24 Jan 2018 10:57:36 -0800 Subject: [PATCH 4/9] remove unnecessary assert --- test/core/bad_client/bad_client.cc | 2 -- 1 file changed, 2 deletions(-) diff --git a/test/core/bad_client/bad_client.cc b/test/core/bad_client/bad_client.cc index 7db77885b1f..140c43fec9c 100644 --- a/test/core/bad_client/bad_client.cc +++ b/test/core/bad_client/bad_client.cc @@ -58,7 +58,6 @@ static void thd_func(void* arg) { /* Sets the done_write event */ static void set_done_write(void* arg, grpc_error* error) { - gpr_log(GPR_INFO, "done writing"); gpr_event* done_write = (gpr_event*)arg; gpr_event_set(done_write, (void*)1); } @@ -157,7 +156,6 @@ void grpc_run_client_side_validator(grpc_bad_client_arg* arg, uint32_t flags, } grpc_core::ExecCtx::Get()->Flush(); } - GPR_ASSERT(gpr_event_wait(&done_write, grpc_timeout_seconds_to_deadline(1))); grpc_slice_buffer_destroy_internal(&outgoing); grpc_core::ExecCtx::Get()->Flush(); } From ea318b6825be34c5a976755de9df03327fbccb0e Mon Sep 17 00:00:00 2001 From: Yash Tibrewal Date: Wed, 24 Jan 2018 10:59:26 -0800 Subject: [PATCH 5/9] Build projects after merge --- CMakeLists.txt | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index f56ec03473e..893525ff00d 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -12448,13 +12448,11 @@ target_include_directories(large_metadata_bad_client_test PRIVATE ${CMAKE_CURRENT_SOURCE_DIR} PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/include PRIVATE ${_gRPC_SSL_INCLUDE_DIR} - PRIVATE ${PROTOBUF_ROOT_DIR}/src - PRIVATE ${BENCHMARK_ROOT_DIR}/include - PRIVATE ${ZLIB_ROOT_DIR} - PRIVATE ${CMAKE_CURRENT_BINARY_DIR}/third_party/zlib - PRIVATE ${CARES_INCLUDE_DIR} - PRIVATE ${CMAKE_CURRENT_BINARY_DIR}/third_party/cares/cares - PRIVATE ${CMAKE_CURRENT_BINARY_DIR}/third_party/gflags/include + PRIVATE ${_gRPC_PROTOBUF_INCLUDE_DIR} + PRIVATE ${_gRPC_ZLIB_INCLUDE_DIR} + PRIVATE ${_gRPC_BENCHMARK_INCLUDE_DIR} + PRIVATE ${_gRPC_CARES_INCLUDE_DIR} + PRIVATE ${_gRPC_GFLAGS_INCLUDE_DIR} ) target_link_libraries(large_metadata_bad_client_test From b9a34339a7109c59eb268d20b91e2bc29952ad9e Mon Sep 17 00:00:00 2001 From: Yash Tibrewal Date: Wed, 24 Jan 2018 14:33:46 -0800 Subject: [PATCH 6/9] Make the thread wait so that write can complete too --- test/core/bad_client/bad_client.cc | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/core/bad_client/bad_client.cc b/test/core/bad_client/bad_client.cc index 0f586f14f1c..f5fb82e5f0b 100644 --- a/test/core/bad_client/bad_client.cc +++ b/test/core/bad_client/bad_client.cc @@ -146,7 +146,8 @@ void grpc_run_client_side_validator(grpc_bad_client_arg* arg, uint32_t flags, client_cq, grpc_timeout_milliseconds_to_deadline(100), nullptr) .type == GRPC_QUEUE_TIMEOUT); - } while (!gpr_event_get(&read_done_event)); + } while (!gpr_event_get(&read_done_event) || + !gpr_event_get(&done_write)); if (arg->client_validator(&incoming, arg->client_validator_arg)) break; gpr_log(GPR_INFO, "client validator failed; trying additional read " @@ -156,6 +157,7 @@ void grpc_run_client_side_validator(grpc_bad_client_arg* arg, uint32_t flags, } grpc_core::ExecCtx::Get()->Flush(); } + grpc_slice_buffer_destroy_internal(&outgoing); grpc_core::ExecCtx::Get()->Flush(); } From 153ebda1ba02cfbe4f39bf3a9025ababb947378c Mon Sep 17 00:00:00 2001 From: Yash Tibrewal Date: Wed, 24 Jan 2018 17:35:54 -0800 Subject: [PATCH 7/9] reduce send size for window overflow test --- test/core/bad_client/tests/window_overflow.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/core/bad_client/tests/window_overflow.cc b/test/core/bad_client/tests/window_overflow.cc index 3fda2e27b42..fe6b05d03a2 100644 --- a/test/core/bad_client/tests/window_overflow.cc +++ b/test/core/bad_client/tests/window_overflow.cc @@ -69,7 +69,7 @@ int main(int argc, char** argv) { #define MAX_FRAME_SIZE 16384 #define MESSAGES_PER_FRAME (MAX_FRAME_SIZE / 5) #define FRAME_SIZE (MESSAGES_PER_FRAME * 5) -#define SEND_SIZE (6 * 1024 * 1024) +#define SEND_SIZE (4 * 1024 * 1024) #define NUM_FRAMES (SEND_SIZE / FRAME_SIZE + 1) grpc_test_init(argc, argv); grpc_init(); From 54f6092eff6348db952ea4eac48888d57c2806e7 Mon Sep 17 00:00:00 2001 From: Yash Tibrewal Date: Tue, 30 Jan 2018 12:17:14 -0800 Subject: [PATCH 8/9] Force the client to shutdown so that the server write can complete --- test/core/bad_client/bad_client.cc | 40 +++++++++++++++++++++--------- test/core/bad_client/bad_client.h | 9 ++++--- 2 files changed, 34 insertions(+), 15 deletions(-) diff --git a/test/core/bad_client/bad_client.cc b/test/core/bad_client/bad_client.cc index f5fb82e5f0b..6a35ffcb6d3 100644 --- a/test/core/bad_client/bad_client.cc +++ b/test/core/bad_client/bad_client.cc @@ -75,6 +75,17 @@ static void set_read_done(void* arg, grpc_error* error) { gpr_event_set(read_done, (void*)1); } +/* shutdown client */ +static void shutdown_client(grpc_endpoint** client_fd) { + if (*client_fd != nullptr) { + grpc_endpoint_shutdown( + *client_fd, GRPC_ERROR_CREATE_FROM_STATIC_STRING("Forced Disconnect")); + grpc_endpoint_destroy(*client_fd); + grpc_core::ExecCtx::Get()->Flush(); + *client_fd = nullptr; + } +} + /* Runs client side validator */ void grpc_run_client_side_validator(grpc_bad_client_arg* arg, uint32_t flags, grpc_endpoint_pair* sfd, @@ -115,11 +126,7 @@ void grpc_run_client_side_validator(grpc_bad_client_arg* arg, uint32_t flags, } if (flags & GRPC_BAD_CLIENT_DISCONNECT) { - grpc_endpoint_shutdown( - sfd->client, GRPC_ERROR_CREATE_FROM_STATIC_STRING("Forced Disconnect")); - grpc_endpoint_destroy(sfd->client); - grpc_core::ExecCtx::Get()->Flush(); - sfd->client = nullptr; + shutdown_client(&sfd->client); } if (sfd->client != nullptr) { @@ -158,6 +165,19 @@ void grpc_run_client_side_validator(grpc_bad_client_arg* arg, uint32_t flags, grpc_core::ExecCtx::Get()->Flush(); } + /* If flags is non-zero, it is time to shutdown the client */ + if (flags) { + shutdown_client(&sfd->client); + } + + /* Make sure that the server is done writing */ + while (!gpr_event_get(&done_write)) { + GPR_ASSERT( + grpc_completion_queue_next( + client_cq, grpc_timeout_milliseconds_to_deadline(100), nullptr) + .type == GRPC_QUEUE_TIMEOUT); + } + grpc_slice_buffer_destroy_internal(&outgoing); grpc_core::ExecCtx::Get()->Flush(); } @@ -206,18 +226,14 @@ void grpc_run_bad_client_test( /* Start validator */ gpr_thd_new(&id, "grpc_bad_client", thd_func, &a, nullptr); for (int i = 0; i < num_args; i++) { - grpc_run_client_side_validator(&args[i], flags, &sfd, client_cq); + grpc_run_client_side_validator(&args[i], i == (num_args - 1) ? flags : 0, + &sfd, client_cq); } /* Wait for server thread to finish */ GPR_ASSERT(gpr_event_wait(&a.done_thd, grpc_timeout_seconds_to_deadline(1))); /* Shutdown. */ - if (sfd.client != nullptr) { - grpc_endpoint_shutdown( - sfd.client, GRPC_ERROR_CREATE_FROM_STATIC_STRING("Test Shutdown")); - grpc_endpoint_destroy(sfd.client); - grpc_core::ExecCtx::Get()->Flush(); - } + shutdown_client(&sfd.client); shutdown_cq = grpc_completion_queue_create_for_pluck(nullptr); grpc_server_shutdown_and_notify(a.server, shutdown_cq, nullptr); diff --git a/test/core/bad_client/bad_client.h b/test/core/bad_client/bad_client.h index bf73a788866..46f25269caa 100644 --- a/test/core/bad_client/bad_client.h +++ b/test/core/bad_client/bad_client.h @@ -49,15 +49,18 @@ struct grpc_bad_client_arg { /* Test runner. * - * Create a server, and for each arg in \a args send client_payload. For each - * payload, run client_validator to make sure that the response is as expected. - * Also execute \a server_validator in a separate thread to assert that the + * Create a server, and for each arg in \a args send client_payload. For each + * payload, run client_validator to make sure that the response is as expected. + * Also execute \a server_validator in a separate thread to assert that the * bytes are handled as expected. */ void grpc_run_bad_client_test( grpc_bad_client_server_side_validator server_validator, grpc_bad_client_arg args[], int num_args, uint32_t flags); +/* A hack to let old tests work as before. In these tests, instead of an array, + * the tests provide a single client_validator and payload + */ #define COMBINE1(X, Y) X##Y #define COMBINE(X, Y) COMBINE1(X, Y) From 83ad85e2faa0e980ed51b1979c4e6486ec1afd3b Mon Sep 17 00:00:00 2001 From: Yash Tibrewal Date: Tue, 30 Jan 2018 15:25:05 -0800 Subject: [PATCH 9/9] Remove unnecessary check and few more comments --- test/core/bad_client/bad_client.cc | 10 +++++----- test/core/bad_client/bad_client.h | 4 ++++ 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/test/core/bad_client/bad_client.cc b/test/core/bad_client/bad_client.cc index 6a35ffcb6d3..df803d16a2e 100644 --- a/test/core/bad_client/bad_client.cc +++ b/test/core/bad_client/bad_client.cc @@ -153,8 +153,7 @@ void grpc_run_client_side_validator(grpc_bad_client_arg* arg, uint32_t flags, client_cq, grpc_timeout_milliseconds_to_deadline(100), nullptr) .type == GRPC_QUEUE_TIMEOUT); - } while (!gpr_event_get(&read_done_event) || - !gpr_event_get(&done_write)); + } while (!gpr_event_get(&read_done_event)); if (arg->client_validator(&incoming, arg->client_validator_arg)) break; gpr_log(GPR_INFO, "client validator failed; trying additional read " @@ -165,12 +164,13 @@ void grpc_run_client_side_validator(grpc_bad_client_arg* arg, uint32_t flags, grpc_core::ExecCtx::Get()->Flush(); } - /* If flags is non-zero, it is time to shutdown the client */ - if (flags) { + /* If the request was too large, then we need to forcefully shut down the + * client, so that the write can be considered completed */ + if (flags & GRPC_BAD_CLIENT_LARGE_REQUEST) { shutdown_client(&sfd->client); } - /* Make sure that the server is done writing */ + /* Make sure that the client is done writing */ while (!gpr_event_get(&done_write)) { GPR_ASSERT( grpc_completion_queue_next( diff --git a/test/core/bad_client/bad_client.h b/test/core/bad_client/bad_client.h index 46f25269caa..de7d830cd72 100644 --- a/test/core/bad_client/bad_client.h +++ b/test/core/bad_client/bad_client.h @@ -44,6 +44,7 @@ struct grpc_bad_client_arg { size_t client_payload_length; }; +/* Flags for grpc_run_bad_client_test */ #define GRPC_BAD_CLIENT_DISCONNECT 1 #define GRPC_BAD_CLIENT_LARGE_REQUEST 2 @@ -53,6 +54,9 @@ struct grpc_bad_client_arg { * payload, run client_validator to make sure that the response is as expected. * Also execute \a server_validator in a separate thread to assert that the * bytes are handled as expected. + * + * The flags are only applicable to the last validator in the array. (This can + * be changed in the future if necessary) */ void grpc_run_bad_client_test( grpc_bad_client_server_side_validator server_validator,