From 307a7207a00b1ea34f6f5edbfd3e46faf7222aaf Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Sat, 2 Apr 2016 07:40:39 -0700 Subject: Add a failing test due to a head of line blocking bug in the server --- test/core/bad_client/gen_build_yaml.py | 1 + test/core/bad_client/tests/head_of_line_blocking.c | 151 +++++++++++++++++++++ 2 files changed, 152 insertions(+) create mode 100644 test/core/bad_client/tests/head_of_line_blocking.c (limited to 'test') diff --git a/test/core/bad_client/gen_build_yaml.py b/test/core/bad_client/gen_build_yaml.py index ca26eda170..d49858ed6d 100755 --- a/test/core/bad_client/gen_build_yaml.py +++ b/test/core/bad_client/gen_build_yaml.py @@ -44,6 +44,7 @@ BAD_CLIENT_TESTS = { 'connection_prefix': default_test_options._replace(cpu_cost=0.2), '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, 'server_registered_method': default_test_options, 'simple_request': default_test_options, 'window_overflow': default_test_options, diff --git a/test/core/bad_client/tests/head_of_line_blocking.c b/test/core/bad_client/tests/head_of_line_blocking.c new file mode 100644 index 0000000000..53cd4537b2 --- /dev/null +++ b/test/core/bad_client/tests/head_of_line_blocking.c @@ -0,0 +1,151 @@ +/* + * + * Copyright 2015, 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. + * + */ + +#include "test/core/bad_client/bad_client.h" + +#include + +#include + +#include "src/core/lib/surface/server.h" +#include "test/core/end2end/cq_verifier.h" + +static const char prefix[] = + "PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n" + // settings frame + "\x00\x00\x00\x04\x00\x00\x00\x00\x00" + // stream 1 headers: generated from server_registered_method.headers in this + // directory + "\x00\x00\xd0\x01\x04\x00\x00\x00\x01" + "\x10\x05:path\x0f/registered/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)" + // data frame for stream 1: advertise a 10000 byte payload (that we won't + // fulfill) + "\x00\x00\x05\x00\x00\x00\x00\x00\x01" + "\x01\x00\x00\x27\x10" + // stream 3 headers: generated from server_registered_method.headers in this + // directory + "\x00\x00\xd0\x01\x04\x00\x00\x00\x03" + "\x10\x05:path\x0f/registered/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)" + // data frame for stream 3: advertise a 10000 byte payload (that we will + // fulfill) + "\x00\x00\x05\x00\x00\x00\x00\x00\x03" + "\x01\x00\x00\x27\x10" + ""; + +static void *tag(intptr_t t) { return (void *)t; } + +static void verifier(grpc_server *server, grpc_completion_queue *cq, + void *registered_method) { + grpc_call_error error; + grpc_call *s; + cq_verifier *cqv = cq_verifier_create(cq); + grpc_metadata_array request_metadata_recv; + gpr_timespec deadline; + grpc_byte_buffer *payload = NULL; + + grpc_metadata_array_init(&request_metadata_recv); + + error = grpc_server_request_registered_call(server, registered_method, &s, + &deadline, &request_metadata_recv, + &payload, cq, cq, tag(101)); + GPR_ASSERT(GRPC_CALL_OK == error); + cq_expect_completion(cqv, tag(101), 1); + cq_verify(cqv); + + GPR_ASSERT(payload != NULL); + + grpc_metadata_array_destroy(&request_metadata_recv); + grpc_call_destroy(s); + grpc_byte_buffer_destroy(payload); + cq_verifier_destroy(cqv); +} + +char *g_buffer; +size_t g_cap = 0; +size_t g_count = 0; + +static void addbuf(const void *data, size_t len) { + if (g_count + len > g_cap) { + g_cap = GPR_MAX(g_count + len, g_cap * 2); + g_buffer = gpr_realloc(g_buffer, g_cap); + } + memcpy(g_buffer + g_count, data, len); + g_count += len; +} + +int main(int argc, char **argv) { + int i; + grpc_test_init(argc, argv); + +#define NUM_FRAMES 10 +#define FRAME_SIZE 1000 + + addbuf(prefix, sizeof(prefix) - 1); + for (i = 0; i < NUM_FRAMES; i++) { + uint8_t hdr[9] = {(uint8_t)(FRAME_SIZE >> 16), + (uint8_t)(FRAME_SIZE >> 8), + (uint8_t)FRAME_SIZE, + 0, + 0, + 0, + 0, + 0, + 3}; + uint8_t msg[FRAME_SIZE]; + memset(msg, 'a', sizeof(msg)); + addbuf(hdr, sizeof(hdr)); + addbuf(msg, FRAME_SIZE); + } + grpc_run_bad_client_test(verifier, g_buffer, g_count, 0); + gpr_free(g_buffer); + + return 0; +} -- cgit v1.2.3 From 06cb1a9d938b566f79e59e5e9ba31979e13274ba Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Mon, 4 Apr 2016 08:10:47 -0700 Subject: Initial interface rework to allow knowing whether to pull payload at registration, not at request time --- include/grpc/grpc.h | 15 +++- include/grpc/impl/codegen/grpc_types.h | 4 +- src/core/lib/surface/server.c | 139 ++++++++++++++++++--------------- src/cpp/server/server.cc | 18 ++++- test/core/bad_client/bad_client.c | 6 +- test/core/surface/server_test.c | 20 +++-- 6 files changed, 121 insertions(+), 81 deletions(-) (limited to 'test') diff --git a/include/grpc/grpc.h b/include/grpc/grpc.h index 8b460722e2..5c868aece3 100644 --- a/include/grpc/grpc.h +++ b/include/grpc/grpc.h @@ -289,6 +289,14 @@ GRPCAPI grpc_call_error grpc_server_request_call( grpc_completion_queue *cq_bound_to_call, grpc_completion_queue *cq_for_notification, void *tag_new); +/** How to handle payloads for a registered method */ +typedef enum { + /** Don't try to read the payload */ + GRPC_SRM_PAYLOAD_NONE, + /** Read the initial payload as a byte buffer */ + GRPC_SRM_PAYLOAD_READ_INITIAL_BYTE_BUFFER +} grpc_server_register_method_payload_handling; + /** Registers a method in the server. Methods to this (host, method) pair will not be reported by grpc_server_request_call, but instead be reported by @@ -296,9 +304,10 @@ GRPCAPI grpc_call_error grpc_server_request_call( registered_method (as returned by this function). Must be called before grpc_server_start. Returns NULL on failure. */ -GRPCAPI void *grpc_server_register_method(grpc_server *server, - const char *method, const char *host, - uint32_t flags); +GRPCAPI void *grpc_server_register_method( + grpc_server *server, const char *method, const char *host, + grpc_server_register_method_payload_handling payload_handling, + uint32_t flags); /** Request notification of a new pre-registered call. 'cq_for_notification' must have been registered to the server via diff --git a/include/grpc/impl/codegen/grpc_types.h b/include/grpc/impl/codegen/grpc_types.h index b09b1cdf44..38da99b4d7 100644 --- a/include/grpc/impl/codegen/grpc_types.h +++ b/include/grpc/impl/codegen/grpc_types.h @@ -185,7 +185,9 @@ typedef enum grpc_call_error { server */ GRPC_CALL_ERROR_NOT_SERVER_COMPLETION_QUEUE, /** this batch of operations leads to more operations than allowed */ - GRPC_CALL_ERROR_BATCH_TOO_BIG + GRPC_CALL_ERROR_BATCH_TOO_BIG, + /** payload type requested is not the type registered */ + GRPC_CALL_ERROR_PAYLOAD_TYPE_MISMATCH } grpc_call_error; /* Write Flags: */ diff --git a/src/core/lib/surface/server.c b/src/core/lib/surface/server.c index 1898bee1c1..ac5a2b4160 100644 --- a/src/core/lib/surface/server.c +++ b/src/core/lib/surface/server.c @@ -173,6 +173,7 @@ struct request_matcher { struct registered_method { char *method; char *host; + grpc_server_register_method_payload_handling payload_handling; uint32_t flags; request_matcher request_matcher; registered_method *next; @@ -417,6 +418,69 @@ static void destroy_channel(grpc_exec_ctx *exec_ctx, channel_data *chand) { &op); } +static void publish_registered_or_batch(grpc_exec_ctx *exec_ctx, + void *user_data, bool success); + +static void cpstr(char **dest, size_t *capacity, grpc_mdstr *value) { + gpr_slice slice = value->slice; + size_t len = GPR_SLICE_LENGTH(slice); + + if (len + 1 > *capacity) { + *capacity = GPR_MAX(len + 1, *capacity * 2); + *dest = gpr_realloc(*dest, *capacity); + } + memcpy(*dest, grpc_mdstr_as_c_string(value), len + 1); +} + +static void begin_call(grpc_exec_ctx *exec_ctx, grpc_server *server, + call_data *calld, requested_call *rc) { + grpc_op ops[1]; + grpc_op *op = ops; + + memset(ops, 0, sizeof(ops)); + + /* called once initial metadata has been read by the call, but BEFORE + the ioreq to fetch it out of the call has been executed. + This means metadata related fields can be relied on in calld, but to + fill in the metadata array passed by the client, we need to perform + an ioreq op, that should complete immediately. */ + + grpc_call_set_completion_queue(exec_ctx, calld->call, rc->cq_bound_to_call); + grpc_closure_init(&rc->publish, publish_registered_or_batch, rc); + *rc->call = calld->call; + calld->cq_new = rc->cq_for_notification; + GPR_SWAP(grpc_metadata_array, *rc->initial_metadata, calld->initial_metadata); + switch (rc->type) { + case BATCH_CALL: + GPR_ASSERT(calld->host != NULL); + GPR_ASSERT(calld->path != NULL); + cpstr(&rc->data.batch.details->host, + &rc->data.batch.details->host_capacity, calld->host); + cpstr(&rc->data.batch.details->method, + &rc->data.batch.details->method_capacity, calld->path); + rc->data.batch.details->deadline = calld->deadline; + rc->data.batch.details->flags = + 0 | (calld->recv_idempotent_request + ? GRPC_INITIAL_METADATA_IDEMPOTENT_REQUEST + : 0); + break; + case REGISTERED_CALL: + *rc->data.registered.deadline = calld->deadline; + if (rc->data.registered.optional_payload) { + op->op = GRPC_OP_RECV_MESSAGE; + op->data.recv_message = rc->data.registered.optional_payload; + op++; + } + break; + default: + GPR_UNREACHABLE_CODE(return ); + } + + GRPC_CALL_INTERNAL_REF(calld->call, "server"); + grpc_call_start_batch_and_execute(exec_ctx, calld->call, ops, + (size_t)(op - ops), &rc->publish); +} + static void finish_start_new_rpc(grpc_exec_ctx *exec_ctx, grpc_server *server, grpc_call_element *elem, request_matcher *rm) { call_data *calld = elem->call_data; @@ -840,8 +904,10 @@ static int streq(const char *a, const char *b) { return 0 == strcmp(a, b); } -void *grpc_server_register_method(grpc_server *server, const char *method, - const char *host, uint32_t flags) { +void *grpc_server_register_method( + grpc_server *server, const char *method, const char *host, + grpc_server_register_method_payload_handling payload_handling, + uint32_t flags) { registered_method *m; GRPC_API_TRACE( "grpc_server_register_method(server=%p, method=%s, host=%s, " @@ -1209,6 +1275,12 @@ grpc_call_error grpc_server_request_registered_call( error = GRPC_CALL_ERROR_NOT_SERVER_COMPLETION_QUEUE; goto done; } + if ((optional_payload == NULL) != + (rm->payload_handling == GRPC_SRM_PAYLOAD_NONE)) { + gpr_free(rc); + error = GRPC_CALL_ERROR_PAYLOAD_TYPE_MISMATCH; + goto done; + } grpc_cq_begin_op(cq_for_notification, tag); rc->type = REGISTERED_CALL; rc->server = server; @@ -1226,69 +1298,6 @@ done: return error; } -static void publish_registered_or_batch(grpc_exec_ctx *exec_ctx, - void *user_data, bool success); - -static void cpstr(char **dest, size_t *capacity, grpc_mdstr *value) { - gpr_slice slice = value->slice; - size_t len = GPR_SLICE_LENGTH(slice); - - if (len + 1 > *capacity) { - *capacity = GPR_MAX(len + 1, *capacity * 2); - *dest = gpr_realloc(*dest, *capacity); - } - memcpy(*dest, grpc_mdstr_as_c_string(value), len + 1); -} - -static void begin_call(grpc_exec_ctx *exec_ctx, grpc_server *server, - call_data *calld, requested_call *rc) { - grpc_op ops[1]; - grpc_op *op = ops; - - memset(ops, 0, sizeof(ops)); - - /* called once initial metadata has been read by the call, but BEFORE - the ioreq to fetch it out of the call has been executed. - This means metadata related fields can be relied on in calld, but to - fill in the metadata array passed by the client, we need to perform - an ioreq op, that should complete immediately. */ - - grpc_call_set_completion_queue(exec_ctx, calld->call, rc->cq_bound_to_call); - grpc_closure_init(&rc->publish, publish_registered_or_batch, rc); - *rc->call = calld->call; - calld->cq_new = rc->cq_for_notification; - GPR_SWAP(grpc_metadata_array, *rc->initial_metadata, calld->initial_metadata); - switch (rc->type) { - case BATCH_CALL: - GPR_ASSERT(calld->host != NULL); - GPR_ASSERT(calld->path != NULL); - cpstr(&rc->data.batch.details->host, - &rc->data.batch.details->host_capacity, calld->host); - cpstr(&rc->data.batch.details->method, - &rc->data.batch.details->method_capacity, calld->path); - rc->data.batch.details->deadline = calld->deadline; - rc->data.batch.details->flags = - 0 | (calld->recv_idempotent_request - ? GRPC_INITIAL_METADATA_IDEMPOTENT_REQUEST - : 0); - break; - case REGISTERED_CALL: - *rc->data.registered.deadline = calld->deadline; - if (rc->data.registered.optional_payload) { - op->op = GRPC_OP_RECV_MESSAGE; - op->data.recv_message = rc->data.registered.optional_payload; - op++; - } - break; - default: - GPR_UNREACHABLE_CODE(return ); - } - - GRPC_CALL_INTERNAL_REF(calld->call, "server"); - grpc_call_start_batch_and_execute(exec_ctx, calld->call, ops, - (size_t)(op - ops), &rc->publish); -} - static void done_request_event(grpc_exec_ctx *exec_ctx, void *req, grpc_cq_completion *c) { requested_call *rc = req; diff --git a/src/cpp/server/server.cc b/src/cpp/server/server.cc index 3e44c502fc..fafe31e84c 100644 --- a/src/cpp/server/server.cc +++ b/src/cpp/server/server.cc @@ -321,6 +321,19 @@ void Server::SetGlobalCallbacks(GlobalCallbacks* callbacks) { g_callbacks.reset(callbacks); } +static grpc_server_register_method_payload_handling PayloadHandlingForMethod( + RpcServiceMethod* method) { + switch (method->method_type()) { + case RpcMethod::NORMAL_RPC: + case RpcMethod::SERVER_STREAMING: + return GRPC_SRM_PAYLOAD_READ_INITIAL_BYTE_BUFFER; + case RpcMethod::CLIENT_STREAMING: + case RpcMethod::BIDI_STREAMING: + return GRPC_SRM_PAYLOAD_NONE; + } + GPR_UNREACHABLE_CODE(return GRPC_SRM_PAYLOAD_NONE;); +} + bool Server::RegisterService(const grpc::string* host, Service* service) { bool has_async_methods = service->has_async_methods(); if (has_async_methods) { @@ -334,8 +347,9 @@ bool Server::RegisterService(const grpc::string* host, Service* service) { continue; } RpcServiceMethod* method = it->get(); - void* tag = grpc_server_register_method(server_, method->name(), - host ? host->c_str() : nullptr, 0); + void* tag = grpc_server_register_method( + server_, method->name(), host ? host->c_str() : nullptr, + PayloadHandlingForMethod(method), 0); if (tag == nullptr) { gpr_log(GPR_DEBUG, "Attempt to register %s multiple times", method->name()); diff --git a/test/core/bad_client/bad_client.c b/test/core/bad_client/bad_client.c index 433ecf69df..1e1b227159 100644 --- a/test/core/bad_client/bad_client.c +++ b/test/core/bad_client/bad_client.c @@ -107,9 +107,9 @@ void grpc_run_bad_client_test(grpc_bad_client_server_side_validator validator, gpr_event_init(&a.done_write); a.validator = validator; grpc_server_register_completion_queue(a.server, a.cq, NULL); - a.registered_method = - grpc_server_register_method(a.server, GRPC_BAD_CLIENT_REGISTERED_METHOD, - GRPC_BAD_CLIENT_REGISTERED_HOST, 0); + a.registered_method = grpc_server_register_method( + a.server, GRPC_BAD_CLIENT_REGISTERED_METHOD, + GRPC_BAD_CLIENT_REGISTERED_HOST, GRPC_SRM_PAYLOAD_NONE, 0); grpc_server_start(a.server); transport = grpc_create_chttp2_transport(&exec_ctx, NULL, sfd.server, 0); server_setup_transport(&a, transport); diff --git a/test/core/surface/server_test.c b/test/core/surface/server_test.c index 4c62d8caad..7bb45434f4 100644 --- a/test/core/surface/server_test.c +++ b/test/core/surface/server_test.c @@ -42,19 +42,25 @@ void test_register_method_fail(void) { grpc_server *server = grpc_server_create(NULL, NULL); void *method; void *method_old; - method = grpc_server_register_method(server, NULL, NULL, 0); + method = + grpc_server_register_method(server, NULL, NULL, GRPC_SRM_PAYLOAD_NONE, 0); GPR_ASSERT(method == NULL); - method_old = grpc_server_register_method(server, "m", "h", 0); + method_old = + grpc_server_register_method(server, "m", "h", GRPC_SRM_PAYLOAD_NONE, 0); GPR_ASSERT(method_old != NULL); - method = grpc_server_register_method(server, "m", "h", 0); + method = grpc_server_register_method( + server, "m", "h", GRPC_SRM_PAYLOAD_READ_INITIAL_BYTE_BUFFER, 0); GPR_ASSERT(method == NULL); - method_old = grpc_server_register_method( - server, "m2", "h2", GRPC_INITIAL_METADATA_IDEMPOTENT_REQUEST); + method_old = + grpc_server_register_method(server, "m2", "h2", GRPC_SRM_PAYLOAD_NONE, + GRPC_INITIAL_METADATA_IDEMPOTENT_REQUEST); GPR_ASSERT(method_old != NULL); - method = grpc_server_register_method(server, "m2", "h2", 0); + method = + grpc_server_register_method(server, "m2", "h2", GRPC_SRM_PAYLOAD_NONE, 0); GPR_ASSERT(method == NULL); method = grpc_server_register_method( - server, "m2", "h2", GRPC_INITIAL_METADATA_IDEMPOTENT_REQUEST); + server, "m2", "h2", GRPC_SRM_PAYLOAD_READ_INITIAL_BYTE_BUFFER, + GRPC_INITIAL_METADATA_IDEMPOTENT_REQUEST); GPR_ASSERT(method == NULL); grpc_server_destroy(server); } -- cgit v1.2.3 From 2839edd222d980c3735d1ae1fa5c1baef1446d6c Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Mon, 4 Apr 2016 08:37:59 -0700 Subject: Fix registration in test --- test/core/bad_client/bad_client.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) (limited to 'test') diff --git a/test/core/bad_client/bad_client.c b/test/core/bad_client/bad_client.c index 1e1b227159..79da278b3c 100644 --- a/test/core/bad_client/bad_client.c +++ b/test/core/bad_client/bad_client.c @@ -107,9 +107,10 @@ void grpc_run_bad_client_test(grpc_bad_client_server_side_validator validator, gpr_event_init(&a.done_write); a.validator = validator; grpc_server_register_completion_queue(a.server, a.cq, NULL); - a.registered_method = grpc_server_register_method( - a.server, GRPC_BAD_CLIENT_REGISTERED_METHOD, - GRPC_BAD_CLIENT_REGISTERED_HOST, GRPC_SRM_PAYLOAD_NONE, 0); + a.registered_method = + grpc_server_register_method(a.server, GRPC_BAD_CLIENT_REGISTERED_METHOD, + GRPC_BAD_CLIENT_REGISTERED_HOST, + GRPC_SRM_PAYLOAD_READ_INITIAL_BYTE_BUFFER, 0); grpc_server_start(a.server); transport = grpc_create_chttp2_transport(&exec_ctx, NULL, sfd.server, 0); server_setup_transport(&a, transport); -- cgit v1.2.3