From 2eedca72d53e9987da3a3a29961736312baf8b6f Mon Sep 17 00:00:00 2001 From: Yuchen Zeng Date: Thu, 5 May 2016 18:20:09 -0700 Subject: [PATCH 01/13] Check the value of Next() in async examples --- examples/cpp/helloworld/greeter_async_client.cc | 4 +++- examples/cpp/helloworld/greeter_async_server.cc | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/examples/cpp/helloworld/greeter_async_client.cc b/examples/cpp/helloworld/greeter_async_client.cc index c1f5eb55f05..33de59fb957 100644 --- a/examples/cpp/helloworld/greeter_async_client.cc +++ b/examples/cpp/helloworld/greeter_async_client.cc @@ -87,7 +87,9 @@ class GreeterClient { void* got_tag; bool ok = false; // Block until the next result is available in the completion queue "cq". - cq.Next(&got_tag, &ok); + // The return value of Next should always be checked. This return value + // tells us whether there is any kind of event or the cq_ is shutting down. + GPR_ASSERT(cq.Next(&got_tag, &ok)); // Verify that the result from "cq" corresponds, by its tag, our previous // request. diff --git a/examples/cpp/helloworld/greeter_async_server.cc b/examples/cpp/helloworld/greeter_async_server.cc index 64e065b1e40..ead44182e57 100644 --- a/examples/cpp/helloworld/greeter_async_server.cc +++ b/examples/cpp/helloworld/greeter_async_server.cc @@ -160,7 +160,9 @@ class ServerImpl final { // Block waiting to read the next event from the completion queue. The // event is uniquely identified by its tag, which in this case is the // memory address of a CallData instance. - cq_->Next(&tag, &ok); + // The return value of Next should always be checked. This return value + // tells us whether there is any kind of event or cq_ is shutting down. + GPR_ASSERT(cq_->Next(&tag, &ok)); GPR_ASSERT(ok); static_cast(tag)->Proceed(); } From c045fd964559eed01e27f8f012cd74776b09ddf3 Mon Sep 17 00:00:00 2001 From: Masood Malekghassemi Date: Mon, 23 May 2016 12:05:54 -0700 Subject: [PATCH 02/13] Add a test for grpc/grpc#6522 --- .../grpcio/tests/unit/_cython/cygrpc_test.py | 284 ++++++++---------- .../tests/unit/_cython/test_utilities.py | 34 ++- 2 files changed, 153 insertions(+), 165 deletions(-) diff --git a/src/python/grpcio/tests/unit/_cython/cygrpc_test.py b/src/python/grpcio/tests/unit/_cython/cygrpc_test.py index 0a511101f0d..4039c1b99bd 100644 --- a/src/python/grpcio/tests/unit/_cython/cygrpc_test.py +++ b/src/python/grpcio/tests/unit/_cython/cygrpc_test.py @@ -143,22 +143,60 @@ class TypeSmokeTest(unittest.TestCase): del completion_queue -class InsecureServerInsecureClient(unittest.TestCase): +class ServerClientMixin(object): - def setUp(self): + def setUpMixin(self, server_credentials, client_credentials, host_override): self.server_completion_queue = cygrpc.CompletionQueue() self.server = cygrpc.Server() self.server.register_completion_queue(self.server_completion_queue) - self.port = self.server.add_http2_port('[::]:0') + if server_credentials: + self.port = self.server.add_http2_port('[::]:0', server_credentials) + else: + self.port = self.server.add_http2_port('[::]:0') self.server.start() self.client_completion_queue = cygrpc.CompletionQueue() - self.client_channel = cygrpc.Channel('localhost:{}'.format(self.port)) - - def tearDown(self): + if client_credentials: + client_channel_arguments = cygrpc.ChannelArgs([ + cygrpc.ChannelArg(cygrpc.ChannelArgKey.ssl_target_name_override, + host_override)]) + self.client_channel = cygrpc.Channel( + 'localhost:{}'.format(self.port), client_channel_arguments, + client_credentials) + else: + self.client_channel = cygrpc.Channel('localhost:{}'.format(self.port)) + if host_override: + self.host_argument = None # default host + self.expected_host = host_override + else: + # arbitrary host name necessitating no further identification + self.host_argument = b'hostess' + self.expected_host = self.host_argument + + def tearDownMixin(self): del self.server del self.client_completion_queue del self.server_completion_queue + def _perform_operations(self, operations, call, queue, deadline, description): + """Perform the list of operations with given call, queue, and deadline. + + Invocation errors are reported with as an exception with `description` in + the message. Performs the operations asynchronously, returning a future. + """ + def performer(): + tag = object() + try: + call_result = call.start_batch(cygrpc.Operations(operations), tag) + self.assertEqual(cygrpc.CallError.ok, call_result) + event = queue.poll(deadline) + self.assertEqual(cygrpc.CompletionType.operation_complete, event.type) + self.assertTrue(event.success) + self.assertIs(tag, event.tag) + except Exception as error: + raise Exception("Error in '{}': {}".format(description, error.message)) + return event + return test_utilities.SimpleFuture(performer) + def testEcho(self): DEADLINE = time.time()+5 DEADLINE_TOLERANCE = 0.25 @@ -175,7 +213,6 @@ class InsecureServerInsecureClient(unittest.TestCase): REQUEST = b'in death a member of project mayhem has a name' RESPONSE = b'his name is robert paulson' METHOD = b'twinkies' - HOST = b'hostess' cygrpc_deadline = cygrpc.Timespec(DEADLINE) @@ -188,7 +225,8 @@ class InsecureServerInsecureClient(unittest.TestCase): client_call_tag = object() client_call = self.client_channel.create_call( - None, 0, self.client_completion_queue, METHOD, HOST, cygrpc_deadline) + None, 0, self.client_completion_queue, METHOD, self.host_argument, + cygrpc_deadline) client_initial_metadata = cygrpc.Metadata([ cygrpc.Metadatum(CLIENT_METADATA_ASCII_KEY, CLIENT_METADATA_ASCII_VALUE), @@ -216,7 +254,8 @@ class InsecureServerInsecureClient(unittest.TestCase): test_common.metadata_transmitted(client_initial_metadata, request_event.request_metadata)) self.assertEqual(METHOD, request_event.request_call_details.method) - self.assertEqual(HOST, request_event.request_call_details.host) + self.assertEqual(self.expected_host, + request_event.request_call_details.host) self.assertLess( abs(DEADLINE - float(request_event.request_call_details.deadline)), DEADLINE_TOLERANCE) @@ -292,172 +331,101 @@ class InsecureServerInsecureClient(unittest.TestCase): del client_call del server_call - -class SecureServerSecureClient(unittest.TestCase): - - def setUp(self): - server_credentials = cygrpc.server_credentials_ssl( - None, [cygrpc.SslPemKeyCertPair(resources.private_key(), - resources.certificate_chain())], False) - channel_credentials = cygrpc.channel_credentials_ssl( - resources.test_root_certificates(), None) - self.server_completion_queue = cygrpc.CompletionQueue() - self.server = cygrpc.Server() - self.server.register_completion_queue(self.server_completion_queue) - self.port = self.server.add_http2_port('[::]:0', server_credentials) - self.server.start() - self.client_completion_queue = cygrpc.CompletionQueue() - client_channel_arguments = cygrpc.ChannelArgs([ - cygrpc.ChannelArg(cygrpc.ChannelArgKey.ssl_target_name_override, - _SSL_HOST_OVERRIDE)]) - self.client_channel = cygrpc.Channel( - 'localhost:{}'.format(self.port), client_channel_arguments, - channel_credentials) - - def tearDown(self): - del self.server - del self.client_completion_queue - del self.server_completion_queue - - def testEcho(self): + def test6522(self): DEADLINE = time.time()+5 DEADLINE_TOLERANCE = 0.25 - CLIENT_METADATA_ASCII_KEY = b'key' - CLIENT_METADATA_ASCII_VALUE = b'val' - CLIENT_METADATA_BIN_KEY = b'key-bin' - CLIENT_METADATA_BIN_VALUE = b'\0'*1000 - SERVER_INITIAL_METADATA_KEY = b'init_me_me_me' - SERVER_INITIAL_METADATA_VALUE = b'whodawha?' - SERVER_TRAILING_METADATA_KEY = b'california_is_in_a_drought' - SERVER_TRAILING_METADATA_VALUE = b'zomg it is' - SERVER_STATUS_CODE = cygrpc.StatusCode.ok - SERVER_STATUS_DETAILS = b'our work is never over' - REQUEST = b'in death a member of project mayhem has a name' - RESPONSE = b'his name is robert paulson' - METHOD = b'/twinkies' - HOST = None # Default host + METHOD = b'twinkies' cygrpc_deadline = cygrpc.Timespec(DEADLINE) + empty_metadata = cygrpc.Metadata([]) server_request_tag = object() - request_call_result = self.server.request_call( + self.server.request_call( self.server_completion_queue, self.server_completion_queue, server_request_tag) + client_call = self.client_channel.create_call( + None, 0, self.client_completion_queue, METHOD, self.host_argument, + cygrpc_deadline) - self.assertEqual(cygrpc.CallError.ok, request_call_result) - - plugin = cygrpc.CredentialsMetadataPlugin(_metadata_plugin_callback, '') - call_credentials = cygrpc.call_credentials_metadata_plugin(plugin) + # Prologue + def perform_client_operations(operations, description): + return self._perform_operations( + operations, client_call, + self.client_completion_queue, cygrpc_deadline, description) - client_call_tag = object() - client_call = self.client_channel.create_call( - None, 0, self.client_completion_queue, METHOD, HOST, cygrpc_deadline) - client_call.set_credentials(call_credentials) - client_initial_metadata = cygrpc.Metadata([ - cygrpc.Metadatum(CLIENT_METADATA_ASCII_KEY, - CLIENT_METADATA_ASCII_VALUE), - cygrpc.Metadatum(CLIENT_METADATA_BIN_KEY, CLIENT_METADATA_BIN_VALUE)]) - client_start_batch_result = client_call.start_batch(cygrpc.Operations([ - cygrpc.operation_send_initial_metadata(client_initial_metadata, - _EMPTY_FLAGS), - cygrpc.operation_send_message(REQUEST, _EMPTY_FLAGS), - cygrpc.operation_send_close_from_client(_EMPTY_FLAGS), - cygrpc.operation_receive_initial_metadata(_EMPTY_FLAGS), - cygrpc.operation_receive_message(_EMPTY_FLAGS), - cygrpc.operation_receive_status_on_client(_EMPTY_FLAGS) - ]), client_call_tag) - self.assertEqual(cygrpc.CallError.ok, client_start_batch_result) - client_event_future = test_utilities.CompletionQueuePollFuture( - self.client_completion_queue, cygrpc_deadline) + client_event_future = perform_client_operations([ + cygrpc.operation_send_initial_metadata(empty_metadata, + _EMPTY_FLAGS), + cygrpc.operation_receive_initial_metadata(_EMPTY_FLAGS), + ], "Client prologue") request_event = self.server_completion_queue.poll(cygrpc_deadline) - self.assertEqual(cygrpc.CompletionType.operation_complete, - request_event.type) - self.assertIsInstance(request_event.operation_call, cygrpc.Call) - self.assertIs(server_request_tag, request_event.tag) - self.assertEqual(0, len(request_event.batch_operations)) - client_metadata_with_credentials = list(client_initial_metadata) + [ - (_CALL_CREDENTIALS_METADATA_KEY, _CALL_CREDENTIALS_METADATA_VALUE)] - self.assertTrue( - test_common.metadata_transmitted(client_metadata_with_credentials, - request_event.request_metadata)) - self.assertEqual(METHOD, request_event.request_call_details.method) - self.assertEqual(_SSL_HOST_OVERRIDE, - request_event.request_call_details.host) - self.assertLess( - abs(DEADLINE - float(request_event.request_call_details.deadline)), - DEADLINE_TOLERANCE) - - server_call_tag = object() server_call = request_event.operation_call - server_initial_metadata = cygrpc.Metadata([ - cygrpc.Metadatum(SERVER_INITIAL_METADATA_KEY, - SERVER_INITIAL_METADATA_VALUE)]) - server_trailing_metadata = cygrpc.Metadata([ - cygrpc.Metadatum(SERVER_TRAILING_METADATA_KEY, - SERVER_TRAILING_METADATA_VALUE)]) - server_start_batch_result = server_call.start_batch([ - cygrpc.operation_send_initial_metadata(server_initial_metadata, - _EMPTY_FLAGS), - cygrpc.operation_receive_message(_EMPTY_FLAGS), - cygrpc.operation_send_message(RESPONSE, _EMPTY_FLAGS), - cygrpc.operation_receive_close_on_server(_EMPTY_FLAGS), - cygrpc.operation_send_status_from_server( - server_trailing_metadata, SERVER_STATUS_CODE, - SERVER_STATUS_DETAILS, _EMPTY_FLAGS) - ], server_call_tag) - self.assertEqual(cygrpc.CallError.ok, server_start_batch_result) - client_event = client_event_future.result() - server_event = self.server_completion_queue.poll(cygrpc_deadline) + def perform_server_operations(operations, description): + return self._perform_operations( + operations, server_call, + self.server_completion_queue, cygrpc_deadline, description) - self.assertEqual(6, len(client_event.batch_operations)) - found_client_op_types = set() - for client_result in client_event.batch_operations: - # we expect each op type to be unique - self.assertNotIn(client_result.type, found_client_op_types) - found_client_op_types.add(client_result.type) - if client_result.type == cygrpc.OperationType.receive_initial_metadata: - self.assertTrue( - test_common.metadata_transmitted(server_initial_metadata, - client_result.received_metadata)) - elif client_result.type == cygrpc.OperationType.receive_message: - self.assertEqual(RESPONSE, client_result.received_message.bytes()) - elif client_result.type == cygrpc.OperationType.receive_status_on_client: - self.assertTrue( - test_common.metadata_transmitted(server_trailing_metadata, - client_result.received_metadata)) - self.assertEqual(SERVER_STATUS_DETAILS, - client_result.received_status_details) - self.assertEqual(SERVER_STATUS_CODE, client_result.received_status_code) - self.assertEqual(set([ - cygrpc.OperationType.send_initial_metadata, - cygrpc.OperationType.send_message, - cygrpc.OperationType.send_close_from_client, - cygrpc.OperationType.receive_initial_metadata, - cygrpc.OperationType.receive_message, - cygrpc.OperationType.receive_status_on_client - ]), found_client_op_types) + server_event_future = perform_server_operations([ + cygrpc.operation_send_initial_metadata(empty_metadata, + _EMPTY_FLAGS), + ], "Server prologue") - self.assertEqual(5, len(server_event.batch_operations)) - found_server_op_types = set() - for server_result in server_event.batch_operations: - self.assertNotIn(client_result.type, found_server_op_types) - found_server_op_types.add(server_result.type) - if server_result.type == cygrpc.OperationType.receive_message: - self.assertEqual(REQUEST, server_result.received_message.bytes()) - elif server_result.type == cygrpc.OperationType.receive_close_on_server: - self.assertFalse(server_result.received_cancelled) - self.assertEqual(set([ - cygrpc.OperationType.send_initial_metadata, - cygrpc.OperationType.receive_message, - cygrpc.OperationType.send_message, - cygrpc.OperationType.receive_close_on_server, - cygrpc.OperationType.send_status_from_server - ]), found_server_op_types) + client_event_future.result() # force completion + server_event_future.result() - del client_call - del server_call + # Messaging + for _ in range(10): + client_event_future = perform_client_operations([ + cygrpc.operation_send_message(b'', _EMPTY_FLAGS), + cygrpc.operation_receive_message(_EMPTY_FLAGS), + ], "Client message") + server_event_future = perform_server_operations([ + cygrpc.operation_send_message(b'', _EMPTY_FLAGS), + cygrpc.operation_receive_message(_EMPTY_FLAGS), + ], "Server receive") + + client_event_future.result() # force completion + server_event_future.result() + + # Epilogue + client_event_future = perform_client_operations([ + cygrpc.operation_send_close_from_client(_EMPTY_FLAGS), + cygrpc.operation_receive_status_on_client(_EMPTY_FLAGS) + ], "Client epilogue") + + server_event_future = perform_server_operations([ + cygrpc.operation_receive_close_on_server(_EMPTY_FLAGS), + cygrpc.operation_send_status_from_server( + empty_metadata, cygrpc.StatusCode.ok, b'', _EMPTY_FLAGS) + ], "Server epilogue") + + client_event_future.result() # force completion + server_event_future.result() + + +class InsecureServerInsecureClient(unittest.TestCase, ServerClientMixin): + + def setUp(self): + self.setUpMixin(None, None, None) + + def tearDown(self): + self.tearDownMixin() + + +class SecureServerSecureClient(unittest.TestCase, ServerClientMixin): + + def setUp(self): + server_credentials = cygrpc.server_credentials_ssl( + None, [cygrpc.SslPemKeyCertPair(resources.private_key(), + resources.certificate_chain())], False) + client_credentials = cygrpc.channel_credentials_ssl( + resources.test_root_certificates(), None) + self.setUpMixin(server_credentials, client_credentials, _SSL_HOST_OVERRIDE) + + def tearDown(self): + self.tearDownMixin() if __name__ == '__main__': diff --git a/src/python/grpcio/tests/unit/_cython/test_utilities.py b/src/python/grpcio/tests/unit/_cython/test_utilities.py index 6a097396431..6280ce74c47 100644 --- a/src/python/grpcio/tests/unit/_cython/test_utilities.py +++ b/src/python/grpcio/tests/unit/_cython/test_utilities.py @@ -32,15 +32,35 @@ import threading from grpc._cython import cygrpc -class CompletionQueuePollFuture: +class SimpleFuture(object): + """A simple future mechanism.""" - def __init__(self, completion_queue, deadline): - def poller_function(): - self._event_result = completion_queue.poll(deadline) - self._event_result = None - self._thread = threading.Thread(target=poller_function) + def __init__(self, function, *args, **kwargs): + def wrapped_function(): + try: + self._result = function(*args, **kwargs) + except Exception as error: + self._error = error + self._result = None + self._error = None + self._thread = threading.Thread(target=wrapped_function) self._thread.start() def result(self): + """The resulting value of this future. + + Re-raises any exceptions. + """ self._thread.join() - return self._event_result + if self._error: + # TODO(atash): re-raise exceptions in a way that preserves tracebacks + raise self._error + return self._result + + +class CompletionQueuePollFuture(SimpleFuture): + + def __init__(self, completion_queue, deadline): + super(CompletionQueuePollFuture, self).__init__( + lambda: completion_queue.poll(deadline)) + From a9d8c5564e37e2d42fa3a6f4546ac3507b1b03c8 Mon Sep 17 00:00:00 2001 From: Yuchen Zeng Date: Thu, 2 Jun 2016 16:11:28 -0700 Subject: [PATCH 03/13] Provide reasons in error messages when streams are canceled with a connection drop --- .../chttp2/transport/chttp2_transport.c | 74 ++++++++++++++++--- .../ext/transport/chttp2/transport/internal.h | 3 + 2 files changed, 65 insertions(+), 12 deletions(-) diff --git a/src/core/ext/transport/chttp2/transport/chttp2_transport.c b/src/core/ext/transport/chttp2/transport/chttp2_transport.c index b6886a2201b..52ac895271e 100644 --- a/src/core/ext/transport/chttp2/transport/chttp2_transport.c +++ b/src/core/ext/transport/chttp2/transport/chttp2_transport.c @@ -47,6 +47,7 @@ #include "src/core/ext/transport/chttp2/transport/internal.h" #include "src/core/ext/transport/chttp2/transport/status_conversion.h" #include "src/core/ext/transport/chttp2/transport/timeout_encoding.h" +#include "src/core/lib/http/parser.h" #include "src/core/lib/profiling/timers.h" #include "src/core/lib/support/string.h" #include "src/core/lib/transport/static_metadata.h" @@ -107,7 +108,8 @@ static void perform_stream_op_locked(grpc_exec_ctx *exec_ctx, static void cancel_from_api(grpc_exec_ctx *exec_ctx, grpc_chttp2_transport_global *transport_global, grpc_chttp2_stream_global *stream_global, - grpc_status_code status); + grpc_status_code status, + gpr_slice *optional_message); static void close_from_api(grpc_exec_ctx *exec_ctx, grpc_chttp2_transport_global *transport_global, @@ -161,6 +163,8 @@ static void destruct_transport(grpc_exec_ctx *exec_ctx, GPR_ASSERT(t->ep == NULL); + gpr_slice_unref(t->optional_drop_message); + gpr_slice_buffer_destroy(&t->global.qbuf); gpr_slice_buffer_destroy(&t->writing.outbuf); @@ -260,6 +264,7 @@ static void init_transport(grpc_exec_ctx *exec_ctx, grpc_chttp2_transport *t, t->parsing.deframe_state = is_client ? GRPC_DTS_FH_0 : GRPC_DTS_CLIENT_PREFIX_0; t->writing.is_client = is_client; + t->optional_drop_message = gpr_empty_slice(); grpc_connectivity_state_init( &t->channel_callback.state_tracker, GRPC_CHANNEL_READY, is_client ? "client_transport" : "server_transport"); @@ -859,7 +864,7 @@ static void maybe_start_some_streams( grpc_chttp2_list_pop_waiting_for_concurrency(transport_global, &stream_global)) { cancel_from_api(exec_ctx, transport_global, stream_global, - GRPC_STATUS_UNAVAILABLE); + GRPC_STATUS_UNAVAILABLE, NULL); } } @@ -936,7 +941,7 @@ static void perform_stream_op_locked(grpc_exec_ctx *exec_ctx, if (op->cancel_with_status != GRPC_STATUS_OK) { cancel_from_api(exec_ctx, transport_global, stream_global, - op->cancel_with_status); + op->cancel_with_status, op->optional_close_message); } if (op->close_with_status != GRPC_STATUS_OK) { @@ -960,7 +965,7 @@ static void perform_stream_op_locked(grpc_exec_ctx *exec_ctx, "(%lu vs. %lu)", metadata_size, metadata_peer_limit); cancel_from_api(exec_ctx, transport_global, stream_global, - GRPC_STATUS_RESOURCE_EXHAUSTED); + GRPC_STATUS_RESOURCE_EXHAUSTED, NULL); } else { if (contains_non_ok_status(transport_global, op->send_initial_metadata)) { stream_global->seen_error = true; @@ -1015,7 +1020,7 @@ static void perform_stream_op_locked(grpc_exec_ctx *exec_ctx, "(%lu vs. %lu)", metadata_size, metadata_peer_limit); cancel_from_api(exec_ctx, transport_global, stream_global, - GRPC_STATUS_RESOURCE_EXHAUSTED); + GRPC_STATUS_RESOURCE_EXHAUSTED, NULL); } else { if (contains_non_ok_status(transport_global, op->send_trailing_metadata)) { @@ -1201,7 +1206,7 @@ static void check_read_ops(grpc_exec_ctx *exec_ctx, } if (stream_global->exceeded_metadata_size) { cancel_from_api(exec_ctx, transport_global, stream_global, - GRPC_STATUS_RESOURCE_EXHAUSTED); + GRPC_STATUS_RESOURCE_EXHAUSTED, NULL); } } grpc_chttp2_incoming_metadata_buffer_publish( @@ -1240,7 +1245,7 @@ static void check_read_ops(grpc_exec_ctx *exec_ctx, } if (stream_global->exceeded_metadata_size) { cancel_from_api(exec_ctx, transport_global, stream_global, - GRPC_STATUS_RESOURCE_EXHAUSTED); + GRPC_STATUS_RESOURCE_EXHAUSTED, NULL); } } if (stream_global->all_incoming_byte_streams_finished) { @@ -1303,7 +1308,8 @@ static void remove_stream(grpc_exec_ctx *exec_ctx, grpc_chttp2_transport *t, static void cancel_from_api(grpc_exec_ctx *exec_ctx, grpc_chttp2_transport_global *transport_global, grpc_chttp2_stream_global *stream_global, - grpc_status_code status) { + grpc_status_code status, + gpr_slice *optional_message) { if (!stream_global->read_closed || !stream_global->write_closed) { if (stream_global->id != 0) { gpr_slice_buffer_add( @@ -1313,8 +1319,12 @@ static void cancel_from_api(grpc_exec_ctx *exec_ctx, (uint32_t)grpc_chttp2_grpc_status_to_http2_error(status), &stream_global->stats.outgoing)); } + + if (optional_message) { + gpr_slice_ref(*optional_message); + } grpc_chttp2_fake_status(exec_ctx, transport_global, stream_global, status, - NULL); + optional_message); } if (status != GRPC_STATUS_OK && !stream_global->seen_error) { stream_global->seen_error = true; @@ -1519,16 +1529,23 @@ static void close_from_api(grpc_exec_ctx *exec_ctx, 1); } +typedef struct { + grpc_exec_ctx *exec_ctx; + gpr_slice optional_drop_message; +} cancel_stream_cb_arg; + static void cancel_stream_cb(grpc_chttp2_transport_global *transport_global, void *user_data, grpc_chttp2_stream_global *stream_global) { - cancel_from_api(user_data, transport_global, stream_global, - GRPC_STATUS_UNAVAILABLE); + cancel_stream_cb_arg *arg = user_data; + cancel_from_api(arg->exec_ctx, transport_global, stream_global, + GRPC_STATUS_UNAVAILABLE, &arg->optional_drop_message); } static void end_all_the_calls(grpc_exec_ctx *exec_ctx, grpc_chttp2_transport *t) { - grpc_chttp2_for_all_streams(&t->global, exec_ctx, cancel_stream_cb); + cancel_stream_cb_arg arg = {exec_ctx, t->optional_drop_message}; + grpc_chttp2_for_all_streams(&t->global, &arg, cancel_stream_cb); } static void drop_connection(grpc_exec_ctx *exec_ctx, grpc_chttp2_transport *t) { @@ -1599,6 +1616,31 @@ static void reading_action_locked(grpc_exec_ctx *exec_ctx, } } +static bool try_http_parsing(grpc_exec_ctx *exec_ctx, + grpc_chttp2_transport *t) { + return false; + grpc_http_parser parser; + size_t i = 0; + bool success = false; + + grpc_http_parser_init(&parser); + grpc_http1_trace = 1; + + for (; i < t->read_buffer.count && + grpc_http_parser_parse(&parser, t->read_buffer.slices[i]); + i++) + ; + if (grpc_http_parser_eof(&parser) && parser.type == GRPC_HTTP_RESPONSE) { + success = true; + GRPC_CHTTP2_IF_TRACING(gpr_log( + GPR_DEBUG, "Trying to connect an http1.x server, received status:%d", + parser.http.response.status)); + } + + grpc_http_parser_destroy(&parser); + return success; +} + static void parsing_action(grpc_exec_ctx *exec_ctx, void *arg, bool success) { grpc_chttp2_transport *t = arg; GPR_TIMER_BEGIN("reading_action.parse", 0); @@ -1610,6 +1652,14 @@ static void parsing_action(grpc_exec_ctx *exec_ctx, void *arg, bool success) { ; if (i != t->read_buffer.count) { success = false; + gpr_slice_unref(t->optional_drop_message); + if (try_http_parsing(exec_ctx, t)) { + t->optional_drop_message = gpr_slice_from_copied_string( + "Connection dropped: received http1.x response"); + } else { + t->optional_drop_message = gpr_slice_from_copied_string( + "Connection dropped: received unparseable response"); + } } GPR_TIMER_END("reading_action.parse", 0); grpc_chttp2_run_with_global_lock(exec_ctx, t, NULL, post_parse_locked, diff --git a/src/core/ext/transport/chttp2/transport/internal.h b/src/core/ext/transport/chttp2/transport/internal.h index 5872fd8e0a4..7f3339a6204 100644 --- a/src/core/ext/transport/chttp2/transport/internal.h +++ b/src/core/ext/transport/chttp2/transport/internal.h @@ -383,6 +383,9 @@ struct grpc_chttp2_transport { /** Transport op to be applied post-parsing */ grpc_transport_op *post_parsing_op; + + /** Message explaining the reason of dropping connection */ + gpr_slice optional_drop_message; }; typedef struct { From 82ec451c427c0275964ee444d11238b07efeb94d Mon Sep 17 00:00:00 2001 From: Yuchen Zeng Date: Thu, 2 Jun 2016 18:34:34 -0700 Subject: [PATCH 04/13] Check if optional_drop_message is an empty slice --- .../ext/transport/chttp2/transport/chttp2_transport.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/core/ext/transport/chttp2/transport/chttp2_transport.c b/src/core/ext/transport/chttp2/transport/chttp2_transport.c index 52ac895271e..d0c94e6c463 100644 --- a/src/core/ext/transport/chttp2/transport/chttp2_transport.c +++ b/src/core/ext/transport/chttp2/transport/chttp2_transport.c @@ -1531,7 +1531,7 @@ static void close_from_api(grpc_exec_ctx *exec_ctx, typedef struct { grpc_exec_ctx *exec_ctx; - gpr_slice optional_drop_message; + gpr_slice *optional_drop_message; } cancel_stream_cb_arg; static void cancel_stream_cb(grpc_chttp2_transport_global *transport_global, @@ -1539,12 +1539,15 @@ static void cancel_stream_cb(grpc_chttp2_transport_global *transport_global, grpc_chttp2_stream_global *stream_global) { cancel_stream_cb_arg *arg = user_data; cancel_from_api(arg->exec_ctx, transport_global, stream_global, - GRPC_STATUS_UNAVAILABLE, &arg->optional_drop_message); + GRPC_STATUS_UNAVAILABLE, arg->optional_drop_message); } static void end_all_the_calls(grpc_exec_ctx *exec_ctx, grpc_chttp2_transport *t) { - cancel_stream_cb_arg arg = {exec_ctx, t->optional_drop_message}; + cancel_stream_cb_arg arg = {exec_ctx, + GPR_SLICE_IS_EMPTY(t->optional_drop_message) + ? NULL + : &t->optional_drop_message}; grpc_chttp2_for_all_streams(&t->global, &arg, cancel_stream_cb); } @@ -1618,13 +1621,11 @@ static void reading_action_locked(grpc_exec_ctx *exec_ctx, static bool try_http_parsing(grpc_exec_ctx *exec_ctx, grpc_chttp2_transport *t) { - return false; grpc_http_parser parser; size_t i = 0; bool success = false; grpc_http_parser_init(&parser); - grpc_http1_trace = 1; for (; i < t->read_buffer.count && grpc_http_parser_parse(&parser, t->read_buffer.slices[i]); From a2bc0ccb500f0fffcdd263420133b4a0d0cbad2d Mon Sep 17 00:00:00 2001 From: Yuchen Zeng Date: Mon, 6 Jun 2016 16:21:59 -0700 Subject: [PATCH 05/13] Get transport from transport global Avoid introducing cancel_stream_cb_arg --- .../chttp2/transport/chttp2_transport.c | 20 +++++++------------ 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/src/core/ext/transport/chttp2/transport/chttp2_transport.c b/src/core/ext/transport/chttp2/transport/chttp2_transport.c index d0c94e6c463..43e9ca1ff56 100644 --- a/src/core/ext/transport/chttp2/transport/chttp2_transport.c +++ b/src/core/ext/transport/chttp2/transport/chttp2_transport.c @@ -1529,26 +1529,20 @@ static void close_from_api(grpc_exec_ctx *exec_ctx, 1); } -typedef struct { - grpc_exec_ctx *exec_ctx; - gpr_slice *optional_drop_message; -} cancel_stream_cb_arg; - static void cancel_stream_cb(grpc_chttp2_transport_global *transport_global, void *user_data, grpc_chttp2_stream_global *stream_global) { - cancel_stream_cb_arg *arg = user_data; - cancel_from_api(arg->exec_ctx, transport_global, stream_global, - GRPC_STATUS_UNAVAILABLE, arg->optional_drop_message); + grpc_chttp2_transport *transport = TRANSPORT_FROM_GLOBAL(transport_global); + cancel_from_api(user_data, transport_global, stream_global, + GRPC_STATUS_UNAVAILABLE, + GPR_SLICE_IS_EMPTY(transport->optional_drop_message) + ? NULL + : &transport->optional_drop_message); } static void end_all_the_calls(grpc_exec_ctx *exec_ctx, grpc_chttp2_transport *t) { - cancel_stream_cb_arg arg = {exec_ctx, - GPR_SLICE_IS_EMPTY(t->optional_drop_message) - ? NULL - : &t->optional_drop_message}; - grpc_chttp2_for_all_streams(&t->global, &arg, cancel_stream_cb); + grpc_chttp2_for_all_streams(&t->global, exec_ctx, cancel_stream_cb); } static void drop_connection(grpc_exec_ctx *exec_ctx, grpc_chttp2_transport *t) { From d4b4009c3773a31e5fe48fb6cc0a6f9d859781a4 Mon Sep 17 00:00:00 2001 From: Masood Malekghassemi Date: Mon, 6 Jun 2016 17:03:05 -0700 Subject: [PATCH 06/13] Use Python namespace packages Before, namespace packages would break in auditwheel due to a superfluous check (fixed in the commit referenced in the Dockerfiles). Now, the auditwheel used in the manylinux1-support Docker images can handle namespace packages, and we may thus use them. This should alleviate future user pain w.r.t. installation of grpcio-tools causing loss of grpcio files and vice versa (e.g. requiring a reinstall of grpcio following installation of grpcio-tools). --- setup.py | 3 +-- tools/distrib/python/grpcio_tools/setup.py | 3 +-- .../grpc_artifact_python_manylinux_x64/Dockerfile | 7 +++++++ .../grpc_artifact_python_manylinux_x86/Dockerfile | 7 +++++++ 4 files changed, 16 insertions(+), 4 deletions(-) diff --git a/setup.py b/setup.py index f96824fa883..88f859246a8 100644 --- a/setup.py +++ b/setup.py @@ -234,8 +234,7 @@ setuptools.setup( ext_modules=CYTHON_EXTENSION_MODULES, packages=list(PACKAGES), package_dir=PACKAGE_DIRECTORIES, - # TODO(atash): Figure out why auditwheel doesn't like namespace packages. - #namespace_packages=['grpc'], + namespace_packages=['grpc'], package_data=PACKAGE_DATA, install_requires=INSTALL_REQUIRES, setup_requires=SETUP_REQUIRES, diff --git a/tools/distrib/python/grpcio_tools/setup.py b/tools/distrib/python/grpcio_tools/setup.py index 576f7ae32a5..bb08ceaadad 100644 --- a/tools/distrib/python/grpcio_tools/setup.py +++ b/tools/distrib/python/grpcio_tools/setup.py @@ -88,8 +88,7 @@ setuptools.setup( protoc_ext_module(), ]), packages=setuptools.find_packages('.'), - # TODO(atash): Figure out why auditwheel doesn't like namespace packages. - #namespace_packages=['grpc'], + namespace_packages=['grpc'], install_requires=[ 'protobuf>=3.0.0a3', ], diff --git a/tools/dockerfile/grpc_artifact_python_manylinux_x64/Dockerfile b/tools/dockerfile/grpc_artifact_python_manylinux_x64/Dockerfile index 3e31a2b623d..1d4e8e1a4a4 100644 --- a/tools/dockerfile/grpc_artifact_python_manylinux_x64/Dockerfile +++ b/tools/dockerfile/grpc_artifact_python_manylinux_x64/Dockerfile @@ -41,3 +41,10 @@ RUN /opt/python/cp27-cp27mu/bin/pip install cython RUN /opt/python/cp34-cp34m/bin/pip install cython RUN /opt/python/cp35-cp35m/bin/pip install cython +#################################################### +# Install auditwheel with fix for namespace packages +RUN git clone https://github.com/pypa/auditwheel /usr/local/src/auditwheel +RUN cd /usr/local/src/auditwheel && git checkout bf071b38c9fe78b025ea05c78b1cb61d7cb09939 +RUN /opt/python/cp35-cp35m/bin/pip install /usr/local/src/auditwheel +RUN rm /usr/local/bin/auditwheel +RUN cd /usr/local/bin && ln -s /opt/python/cp35-cp35m/bin/auditwheel diff --git a/tools/dockerfile/grpc_artifact_python_manylinux_x86/Dockerfile b/tools/dockerfile/grpc_artifact_python_manylinux_x86/Dockerfile index 5fe62c28b73..810499695ec 100644 --- a/tools/dockerfile/grpc_artifact_python_manylinux_x86/Dockerfile +++ b/tools/dockerfile/grpc_artifact_python_manylinux_x86/Dockerfile @@ -41,3 +41,10 @@ RUN /opt/python/cp27-cp27mu/bin/pip install cython RUN /opt/python/cp34-cp34m/bin/pip install cython RUN /opt/python/cp35-cp35m/bin/pip install cython +#################################################### +# Install auditwheel with fix for namespace packages +RUN git clone https://github.com/pypa/auditwheel /usr/local/src/auditwheel +RUN cd /usr/local/src/auditwheel && git checkout bf071b38c9fe78b025ea05c78b1cb61d7cb09939 +RUN /opt/python/cp35-cp35m/bin/pip install /usr/local/src/auditwheel +RUN rm /usr/local/bin/auditwheel +RUN cd /usr/local/bin && ln -s /opt/python/cp35-cp35m/bin/auditwheel From 2dbc217e5e3f44e4b2037601a7c21b650edaa066 Mon Sep 17 00:00:00 2001 From: Masood Malekghassemi Date: Tue, 7 Jun 2016 15:03:57 -0700 Subject: [PATCH 07/13] Add common proto files to Python grpcio-tools --- tools/distrib/python/check_grpcio_tools.py | 2 +- .../python/grpcio_tools/grpc/tools/protoc.py | 5 ++- .../python/grpcio_tools/protoc_lib_deps.py | 4 +++ tools/distrib/python/grpcio_tools/setup.py | 33 +++++++++++++++-- tools/distrib/python/make_grpcio_tools.py | 35 ++++++++++++++----- 5 files changed, 65 insertions(+), 14 deletions(-) diff --git a/tools/distrib/python/check_grpcio_tools.py b/tools/distrib/python/check_grpcio_tools.py index baf2ff4effb..80c63278f42 100755 --- a/tools/distrib/python/check_grpcio_tools.py +++ b/tools/distrib/python/check_grpcio_tools.py @@ -37,7 +37,7 @@ OUT_OF_DATE_MESSAGE = """file {} is out of date Have you called tools/distrib/python/make_grpcio_tools.py since upgrading protobuf?""" -check_protoc_lib_deps_content = make.get_deps(make.BAZEL_DEPS_PROTOC_LIB_QUERY) +check_protoc_lib_deps_content = make.get_deps() with open(make.GRPC_PYTHON_PROTOC_LIB_DEPS, 'r') as protoc_lib_deps_file: if protoc_lib_deps_file.read() != check_protoc_lib_deps_content: diff --git a/tools/distrib/python/grpcio_tools/grpc/tools/protoc.py b/tools/distrib/python/grpcio_tools/grpc/tools/protoc.py index b4dd0ecae2e..1c69e78920c 100644 --- a/tools/distrib/python/grpcio_tools/grpc/tools/protoc.py +++ b/tools/distrib/python/grpcio_tools/grpc/tools/protoc.py @@ -29,10 +29,13 @@ # (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE # OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. +import pkg_resources import sys from grpc.tools import protoc_compiler if __name__ == '__main__': - protoc_compiler.run_main(sys.argv) + proto_include = pkg_resources.resource_filename('grpc.tools', '_proto') + protoc_compiler.run_main( + sys.argv + ['-I{}'.format(proto_include)]) diff --git a/tools/distrib/python/grpcio_tools/protoc_lib_deps.py b/tools/distrib/python/grpcio_tools/protoc_lib_deps.py index 135ac5cbb3b..cd4effa7ae8 100644 --- a/tools/distrib/python/grpcio_tools/protoc_lib_deps.py +++ b/tools/distrib/python/grpcio_tools/protoc_lib_deps.py @@ -30,3 +30,7 @@ # AUTO-GENERATED BY make_grpcio_tools.py! CC_FILES=['google/protobuf/compiler/zip_writer.cc', 'google/protobuf/compiler/subprocess.cc', 'google/protobuf/compiler/ruby/ruby_generator.cc', 'google/protobuf/compiler/python/python_generator.cc', 'google/protobuf/compiler/plugin.pb.cc', 'google/protobuf/compiler/plugin.cc', 'google/protobuf/compiler/objectivec/objectivec_primitive_field.cc', 'google/protobuf/compiler/objectivec/objectivec_oneof.cc', 'google/protobuf/compiler/objectivec/objectivec_message_field.cc', 'google/protobuf/compiler/objectivec/objectivec_message.cc', 'google/protobuf/compiler/objectivec/objectivec_map_field.cc', 'google/protobuf/compiler/objectivec/objectivec_helpers.cc', 'google/protobuf/compiler/objectivec/objectivec_generator.cc', 'google/protobuf/compiler/objectivec/objectivec_file.cc', 'google/protobuf/compiler/objectivec/objectivec_field.cc', 'google/protobuf/compiler/objectivec/objectivec_extension.cc', 'google/protobuf/compiler/objectivec/objectivec_enum_field.cc', 'google/protobuf/compiler/objectivec/objectivec_enum.cc', 'google/protobuf/compiler/js/js_generator.cc', 'google/protobuf/compiler/javanano/javanano_primitive_field.cc', 'google/protobuf/compiler/javanano/javanano_message_field.cc', 'google/protobuf/compiler/javanano/javanano_message.cc', 'google/protobuf/compiler/javanano/javanano_map_field.cc', 'google/protobuf/compiler/javanano/javanano_helpers.cc', 'google/protobuf/compiler/javanano/javanano_generator.cc', 'google/protobuf/compiler/javanano/javanano_file.cc', 'google/protobuf/compiler/javanano/javanano_field.cc', 'google/protobuf/compiler/javanano/javanano_extension.cc', 'google/protobuf/compiler/javanano/javanano_enum_field.cc', 'google/protobuf/compiler/javanano/javanano_enum.cc', 'google/protobuf/compiler/java/java_string_field_lite.cc', 'google/protobuf/compiler/java/java_string_field.cc', 'google/protobuf/compiler/java/java_shared_code_generator.cc', 'google/protobuf/compiler/java/java_service.cc', 'google/protobuf/compiler/java/java_primitive_field_lite.cc', 'google/protobuf/compiler/java/java_primitive_field.cc', 'google/protobuf/compiler/java/java_name_resolver.cc', 'google/protobuf/compiler/java/java_message_lite.cc', 'google/protobuf/compiler/java/java_message_field_lite.cc', 'google/protobuf/compiler/java/java_message_field.cc', 'google/protobuf/compiler/java/java_message_builder_lite.cc', 'google/protobuf/compiler/java/java_message_builder.cc', 'google/protobuf/compiler/java/java_message.cc', 'google/protobuf/compiler/java/java_map_field_lite.cc', 'google/protobuf/compiler/java/java_map_field.cc', 'google/protobuf/compiler/java/java_lazy_message_field_lite.cc', 'google/protobuf/compiler/java/java_lazy_message_field.cc', 'google/protobuf/compiler/java/java_helpers.cc', 'google/protobuf/compiler/java/java_generator_factory.cc', 'google/protobuf/compiler/java/java_generator.cc', 'google/protobuf/compiler/java/java_file.cc', 'google/protobuf/compiler/java/java_field.cc', 'google/protobuf/compiler/java/java_extension_lite.cc', 'google/protobuf/compiler/java/java_extension.cc', 'google/protobuf/compiler/java/java_enum_lite.cc', 'google/protobuf/compiler/java/java_enum_field_lite.cc', 'google/protobuf/compiler/java/java_enum_field.cc', 'google/protobuf/compiler/java/java_enum.cc', 'google/protobuf/compiler/java/java_doc_comment.cc', 'google/protobuf/compiler/java/java_context.cc', 'google/protobuf/compiler/csharp/csharp_wrapper_field.cc', 'google/protobuf/compiler/csharp/csharp_source_generator_base.cc', 'google/protobuf/compiler/csharp/csharp_repeated_primitive_field.cc', 'google/protobuf/compiler/csharp/csharp_repeated_message_field.cc', 'google/protobuf/compiler/csharp/csharp_repeated_enum_field.cc', 'google/protobuf/compiler/csharp/csharp_reflection_class.cc', 'google/protobuf/compiler/csharp/csharp_primitive_field.cc', 'google/protobuf/compiler/csharp/csharp_message_field.cc', 'google/protobuf/compiler/csharp/csharp_message.cc', 'google/protobuf/compiler/csharp/csharp_map_field.cc', 'google/protobuf/compiler/csharp/csharp_helpers.cc', 'google/protobuf/compiler/csharp/csharp_generator.cc', 'google/protobuf/compiler/csharp/csharp_field_base.cc', 'google/protobuf/compiler/csharp/csharp_enum_field.cc', 'google/protobuf/compiler/csharp/csharp_enum.cc', 'google/protobuf/compiler/csharp/csharp_doc_comment.cc', 'google/protobuf/compiler/cpp/cpp_string_field.cc', 'google/protobuf/compiler/cpp/cpp_service.cc', 'google/protobuf/compiler/cpp/cpp_primitive_field.cc', 'google/protobuf/compiler/cpp/cpp_message_field.cc', 'google/protobuf/compiler/cpp/cpp_message.cc', 'google/protobuf/compiler/cpp/cpp_map_field.cc', 'google/protobuf/compiler/cpp/cpp_helpers.cc', 'google/protobuf/compiler/cpp/cpp_generator.cc', 'google/protobuf/compiler/cpp/cpp_file.cc', 'google/protobuf/compiler/cpp/cpp_field.cc', 'google/protobuf/compiler/cpp/cpp_extension.cc', 'google/protobuf/compiler/cpp/cpp_enum_field.cc', 'google/protobuf/compiler/cpp/cpp_enum.cc', 'google/protobuf/compiler/command_line_interface.cc', 'google/protobuf/compiler/code_generator.cc', 'google/protobuf/wrappers.pb.cc', 'google/protobuf/wire_format.cc', 'google/protobuf/util/type_resolver_util.cc', 'google/protobuf/util/time_util.cc', 'google/protobuf/util/message_differencer.cc', 'google/protobuf/util/json_util.cc', 'google/protobuf/util/internal/utility.cc', 'google/protobuf/util/internal/type_info_test_helper.cc', 'google/protobuf/util/internal/type_info.cc', 'google/protobuf/util/internal/protostream_objectwriter.cc', 'google/protobuf/util/internal/protostream_objectsource.cc', 'google/protobuf/util/internal/proto_writer.cc', 'google/protobuf/util/internal/object_writer.cc', 'google/protobuf/util/internal/json_stream_parser.cc', 'google/protobuf/util/internal/json_objectwriter.cc', 'google/protobuf/util/internal/json_escaping.cc', 'google/protobuf/util/internal/field_mask_utility.cc', 'google/protobuf/util/internal/error_listener.cc', 'google/protobuf/util/internal/default_value_objectwriter.cc', 'google/protobuf/util/internal/datapiece.cc', 'google/protobuf/util/field_mask_util.cc', 'google/protobuf/util/field_comparator.cc', 'google/protobuf/unknown_field_set.cc', 'google/protobuf/type.pb.cc', 'google/protobuf/timestamp.pb.cc', 'google/protobuf/text_format.cc', 'google/protobuf/stubs/substitute.cc', 'google/protobuf/stubs/mathlimits.cc', 'google/protobuf/struct.pb.cc', 'google/protobuf/source_context.pb.cc', 'google/protobuf/service.cc', 'google/protobuf/reflection_ops.cc', 'google/protobuf/message.cc', 'google/protobuf/map_field.cc', 'google/protobuf/io/zero_copy_stream_impl.cc', 'google/protobuf/io/tokenizer.cc', 'google/protobuf/io/strtod.cc', 'google/protobuf/io/printer.cc', 'google/protobuf/io/gzip_stream.cc', 'google/protobuf/generated_message_reflection.cc', 'google/protobuf/field_mask.pb.cc', 'google/protobuf/extension_set_heavy.cc', 'google/protobuf/empty.pb.cc', 'google/protobuf/dynamic_message.cc', 'google/protobuf/duration.pb.cc', 'google/protobuf/descriptor_database.cc', 'google/protobuf/descriptor.pb.cc', 'google/protobuf/descriptor.cc', 'google/protobuf/compiler/parser.cc', 'google/protobuf/compiler/importer.cc', 'google/protobuf/api.pb.cc', 'google/protobuf/any.pb.cc', 'google/protobuf/any.cc', 'google/protobuf/wire_format_lite.cc', 'google/protobuf/stubs/time.cc', 'google/protobuf/stubs/strutil.cc', 'google/protobuf/stubs/structurally_valid.cc', 'google/protobuf/stubs/stringprintf.cc', 'google/protobuf/stubs/stringpiece.cc', 'google/protobuf/stubs/statusor.cc', 'google/protobuf/stubs/status.cc', 'google/protobuf/stubs/once.cc', 'google/protobuf/stubs/int128.cc', 'google/protobuf/stubs/common.cc', 'google/protobuf/stubs/bytestream.cc', 'google/protobuf/stubs/atomicops_internals_x86_msvc.cc', 'google/protobuf/stubs/atomicops_internals_x86_gcc.cc', 'google/protobuf/repeated_field.cc', 'google/protobuf/message_lite.cc', 'google/protobuf/io/zero_copy_stream_impl_lite.cc', 'google/protobuf/io/zero_copy_stream.cc', 'google/protobuf/io/coded_stream.cc', 'google/protobuf/generated_message_util.cc', 'google/protobuf/extension_set.cc', 'google/protobuf/arenastring.cc', 'google/protobuf/arena.cc'] +PROTO_FILES=['google/protobuf/wrappers.proto', 'google/protobuf/type.proto', 'google/protobuf/timestamp.proto', 'google/protobuf/struct.proto', 'google/protobuf/source_context.proto', 'google/protobuf/field_mask.proto', 'google/protobuf/empty.proto', 'google/protobuf/duration.proto', 'google/protobuf/descriptor.proto', 'google/protobuf/compiler/plugin.proto', 'google/protobuf/api.proto', 'google/protobuf/any.proto'] + +CC_INCLUDE='third_party/protobuf/src' +PROTO_INCLUDE='third_party/protobuf/src' diff --git a/tools/distrib/python/grpcio_tools/setup.py b/tools/distrib/python/grpcio_tools/setup.py index 576f7ae32a5..6537c266b32 100644 --- a/tools/distrib/python/grpcio_tools/setup.py +++ b/tools/distrib/python/grpcio_tools/setup.py @@ -28,9 +28,11 @@ # OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. from distutils import extension +import errno import os import os.path import shlex +import shutil import sys import setuptools @@ -47,18 +49,41 @@ sys.path.insert(0, os.path.abspath('.')) # ourselves in w.r.t. the multitude of operating systems this ought to build on. # By default we assume a GCC-like compiler. EXTRA_COMPILE_ARGS = shlex.split(os.environ.get('GRPC_PYTHON_CFLAGS', - '-frtti -std=c++11')) + '-fno-wrapv -frtti -std=c++11')) EXTRA_LINK_ARGS = shlex.split(os.environ.get('GRPC_PYTHON_LDFLAGS', '-lpthread')) +GRPC_PYTHON_TOOLS_PACKAGE = 'grpc.tools' +GRPC_PYTHON_PROTO_RESOURCES_NAME = '_proto' + import protoc_lib_deps import grpc_version +def package_data(): + tools_path = GRPC_PYTHON_TOOLS_PACKAGE.replace('.', os.path.sep) + proto_resources_path = os.path.join(tools_path, + GRPC_PYTHON_PROTO_RESOURCES_NAME) + proto_files = [] + for proto_file in protoc_lib_deps.PROTO_FILES: + source = os.path.join(protoc_lib_deps.PROTO_INCLUDE, proto_file) + target = os.path.join(proto_resources_path, proto_file) + relative_target = os.path.join(GRPC_PYTHON_PROTO_RESOURCES_NAME, proto_file) + try: + os.makedirs(os.path.dirname(target)) + except OSError as error: + if error.errno == errno.EEXIST: + pass + else: + raise + shutil.copy(source, target) + proto_files.append(relative_target) + return {GRPC_PYTHON_TOOLS_PACKAGE: proto_files} + def protoc_ext_module(): plugin_sources = [ 'grpc/tools/main.cc', 'grpc_root/src/compiler/python_generator.cc'] + [ - os.path.join('third_party/protobuf/src', cc_file) + os.path.join(protoc_lib_deps.CC_INCLUDE, cc_file) for cc_file in protoc_lib_deps.CC_FILES] plugin_ext = extension.Extension( name='grpc.tools.protoc_compiler', @@ -67,7 +92,7 @@ def protoc_ext_module(): '.', 'grpc_root', 'grpc_root/include', - 'third_party/protobuf/src', + protoc_lib_deps.CC_INCLUDE, ], language='c++', define_macros=[('HAVE_PTHREAD', 1)], @@ -92,5 +117,7 @@ setuptools.setup( #namespace_packages=['grpc'], install_requires=[ 'protobuf>=3.0.0a3', + 'grpcio>=0.14.0', ], + package_data=package_data(), ) diff --git a/tools/distrib/python/make_grpcio_tools.py b/tools/distrib/python/make_grpcio_tools.py index 50fbdbb14c0..fd9b38b084b 100755 --- a/tools/distrib/python/make_grpcio_tools.py +++ b/tools/distrib/python/make_grpcio_tools.py @@ -67,11 +67,16 @@ DEPS_FILE_CONTENT=""" # OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. # AUTO-GENERATED BY make_grpcio_tools.py! -CC_FILES={} +CC_FILES={cc_files} +PROTO_FILES={proto_files} + +CC_INCLUDE={cc_include} +PROTO_INCLUDE={proto_include} """ # Bazel query result prefix for expected source files in protobuf. PROTOBUF_CC_PREFIX = '//:src/' +PROTOBUF_PROTO_PREFIX = '//:src/' GRPC_ROOT = os.path.abspath( os.path.join(os.path.dirname(os.path.abspath(__file__)), @@ -79,7 +84,8 @@ GRPC_ROOT = os.path.abspath( GRPC_PYTHON_ROOT = os.path.join(GRPC_ROOT, 'tools/distrib/python/grpcio_tools') -GRPC_PROTOBUF = os.path.join(GRPC_ROOT, 'third_party/protobuf/src') +GRPC_PYTHON_PROTOBUF_RELATIVE_ROOT = 'third_party/protobuf/src' +GRPC_PROTOBUF = os.path.join(GRPC_ROOT, GRPC_PYTHON_PROTOBUF_RELATIVE_ROOT) GRPC_PROTOC_PLUGINS = os.path.join(GRPC_ROOT, 'src/compiler') GRPC_PYTHON_PROTOBUF = os.path.join(GRPC_PYTHON_ROOT, 'third_party/protobuf/src') @@ -93,18 +99,29 @@ GRPC_PYTHON_INCLUDE = os.path.join(GRPC_PYTHON_ROOT, 'grpc_root/include') BAZEL_DEPS = os.path.join(GRPC_ROOT, 'tools/distrib/python/bazel_deps.sh') BAZEL_DEPS_PROTOC_LIB_QUERY = '//:protoc_lib' +BAZEL_DEPS_COMMON_PROTOS_QUERY = '//:well_known_protos' + +def bazel_query(query): + output = subprocess.check_output([BAZEL_DEPS, query]) + return output.splitlines() -def get_deps(query): +def get_deps(): """Write the result of the bazel query `query` against protobuf to `out_file`.""" - output = subprocess.check_output([BAZEL_DEPS, query]) - output = output.splitlines() + cc_files_output = bazel_query(BAZEL_DEPS_PROTOC_LIB_QUERY) cc_files = [ - name for name in output + name[len(PROTOBUF_CC_PREFIX):] for name in cc_files_output if name.endswith('.cc') and name.startswith(PROTOBUF_CC_PREFIX)] - cc_files = [cc_file[len(PROTOBUF_CC_PREFIX):] for cc_file in cc_files] - deps_file_content = DEPS_FILE_CONTENT.format(cc_files) + proto_files_output = bazel_query(BAZEL_DEPS_COMMON_PROTOS_QUERY) + proto_files = [ + name[len(PROTOBUF_PROTO_PREFIX):] for name in proto_files_output + if name.endswith('.proto') and name.startswith(PROTOBUF_PROTO_PREFIX)] + deps_file_content = DEPS_FILE_CONTENT.format( + cc_files=cc_files, + proto_files=proto_files, + cc_include=repr(GRPC_PYTHON_PROTOBUF_RELATIVE_ROOT), + proto_include=repr(GRPC_PYTHON_PROTOBUF_RELATIVE_ROOT)) return deps_file_content @@ -123,7 +140,7 @@ def main(): shutil.copytree(GRPC_INCLUDE, GRPC_PYTHON_INCLUDE) try: - protoc_lib_deps_content = get_deps(BAZEL_DEPS_PROTOC_LIB_QUERY) + protoc_lib_deps_content = get_deps() except Exception as error: # We allow this script to succeed even if we couldn't get the dependencies, # as then we can assume that even without a successful bazel run the From 95e547ef9ec96d7693c86d007555b5ce68be3510 Mon Sep 17 00:00:00 2001 From: Jan Tattermusch Date: Tue, 7 Jun 2016 17:14:42 -0700 Subject: [PATCH 08/13] add unstarted server test --- src/csharp/Grpc.Core.Tests/ServerTest.cs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/csharp/Grpc.Core.Tests/ServerTest.cs b/src/csharp/Grpc.Core.Tests/ServerTest.cs index fa693162ada..5f7144611e5 100644 --- a/src/csharp/Grpc.Core.Tests/ServerTest.cs +++ b/src/csharp/Grpc.Core.Tests/ServerTest.cs @@ -93,5 +93,12 @@ namespace Grpc.Core.Tests server.ShutdownAsync().Wait(); } + + [Test] + public void UnstartedServerDoesNotPreventShutdown() + { + // just create a server, don't start it, and make sure it doesn't prevent shutdown. + var server = new Server(); + } } } From f6d09f2566561c9407c564a46901ff3a237ce115 Mon Sep 17 00:00:00 2001 From: Masood Malekghassemi Date: Tue, 7 Jun 2016 22:19:46 -0700 Subject: [PATCH 09/13] Fix command ordering for building Python artifacts Source packages were not getting the necessary C files due to `make_grpcio_tools.py` not being invoked before `sdist` through `setup.py`. --- tools/run_tests/build_artifact_python.sh | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tools/run_tests/build_artifact_python.sh b/tools/run_tests/build_artifact_python.sh index 4320f978e30..55f8eb634ba 100755 --- a/tools/run_tests/build_artifact_python.sh +++ b/tools/run_tests/build_artifact_python.sh @@ -59,12 +59,14 @@ ${SETARCH_CMD} ${PYTHON} setup.py \ ${SETARCH_CMD} ${PYTHON} setup.py \ bdist_wheel +# Build gRPC tools package distribution +${PYTHON} tools/distrib/python/make_grpcio_tools.py + # Build gRPC tools package source distribution ${SETARCH_CMD} ${PYTHON} tools/distrib/python/grpcio_tools/setup.py \ sdist # Build gRPC tools package binary distribution -${PYTHON} tools/distrib/python/make_grpcio_tools.py CFLAGS="$CFLAGS -fno-wrapv" ${SETARCH_CMD} \ ${PYTHON} tools/distrib/python/grpcio_tools/setup.py bdist_wheel From cb79b29ac589ba0f17da1588430db4407f73b5f6 Mon Sep 17 00:00:00 2001 From: Makarand Dharmapurikar Date: Wed, 8 Jun 2016 15:07:54 -0700 Subject: [PATCH 10/13] fix for issue 6344. added printing of .proto file comments in generated code. --- src/compiler/objective_c_generator.cc | 33 ++++++++++++++++++++++++--- 1 file changed, 30 insertions(+), 3 deletions(-) diff --git a/src/compiler/objective_c_generator.cc b/src/compiler/objective_c_generator.cc index 71a674174d7..db0b61ee6f0 100644 --- a/src/compiler/objective_c_generator.cc +++ b/src/compiler/objective_c_generator.cc @@ -60,9 +60,34 @@ void PrintProtoRpcDeclarationAsPragma(Printer *printer, " returns ($server_stream$$response_type$)\n\n"); } +template +static void PrintAllComments(const DescriptorType* desc, Printer* printer) { + std::vector comments; + grpc_generator::GetComment(desc, grpc_generator::COMMENTTYPE_LEADING_DETACHED, + &comments); + grpc_generator::GetComment(desc, grpc_generator::COMMENTTYPE_LEADING, + &comments); + grpc_generator::GetComment(desc, grpc_generator::COMMENTTYPE_TRAILING, + &comments); + if (comments.empty()) { + return; + } + printer->Print("/**\n"); + for (auto it = comments.begin(); it != comments.end(); ++it) { + printer->Print(" * "); + size_t start_pos = it->find_first_not_of(' '); + if (start_pos != grpc::string::npos) { + printer->Print(it->c_str() + start_pos); + } + printer->Print("\n"); + } + printer->Print(" */\n"); +} + void PrintMethodSignature(Printer *printer, const MethodDescriptor *method, const map< ::grpc::string, ::grpc::string> &vars) { - // TODO(jcanizales): Print method comments. + // Print comment + PrintAllComments(method, printer); printer->Print(vars, "- ($return_type$)$method_name$With"); if (method->client_streaming()) { @@ -195,8 +220,10 @@ void PrintMethodImplementations(Printer *printer, printer.Print("@end\n\n"); printer.Print( - "// Basic service implementation, over gRPC, that only does" - " marshalling and parsing.\n"); + "/**\n" + " * Basic service implementation, over gRPC, that only does\n" + " * marshalling and parsing.\n" + " */\n"); printer.Print(vars, "@interface $service_class$ :" " GRPCProtoService<$service_class$>\n"); From 82c3dd1f0a96944122d3a0e40242d592a2c7db64 Mon Sep 17 00:00:00 2001 From: Masood Malekghassemi Date: Wed, 8 Jun 2016 16:56:00 -0700 Subject: [PATCH 11/13] Add usage documentation to Python tools distribution --- tools/distrib/python/grpcio_tools/README.rst | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/tools/distrib/python/grpcio_tools/README.rst b/tools/distrib/python/grpcio_tools/README.rst index 10d2fe8c309..e9f137493ba 100644 --- a/tools/distrib/python/grpcio_tools/README.rst +++ b/tools/distrib/python/grpcio_tools/README.rst @@ -126,3 +126,13 @@ Help, I ... GCC 6.0), this is probably a bug where GCC chokes on constant expressions when the :code:`-fwrapv` flag is specified. You should consider setting your environment with :code:`CFLAGS=-fno-wrapv` or using clang (:code:`CC=clang`). + +Usage +----- + +Given protobuf include directories :code:`$INCLUDE`, an output directory +:code:`$OUTPUT`, and proto files :code:`$PROTO_FILES`, invoke as: + +:: + + $ python -m grpc.tools.protoc -I$INCLUDE --python_out=$OUTPUT --grpc_python_out=$OUTPUT $PROTO_FILES From 8e93533f65938a83760f522c9825cd903a307f03 Mon Sep 17 00:00:00 2001 From: Jan Tattermusch Date: Wed, 8 Jun 2016 19:45:54 -0700 Subject: [PATCH 12/13] allow shutdown of unstarted server --- src/csharp/Grpc.Core.Tests/ServerTest.cs | 8 ++++++++ src/csharp/Grpc.Core/Server.cs | 3 +-- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/src/csharp/Grpc.Core.Tests/ServerTest.cs b/src/csharp/Grpc.Core.Tests/ServerTest.cs index 5f7144611e5..3b51aa63300 100644 --- a/src/csharp/Grpc.Core.Tests/ServerTest.cs +++ b/src/csharp/Grpc.Core.Tests/ServerTest.cs @@ -94,6 +94,14 @@ namespace Grpc.Core.Tests server.ShutdownAsync().Wait(); } + [Test] + public void UnstartedServerCanBeShutdown() + { + var server = new Server(); + server.ShutdownAsync().Wait(); + Assert.Throws(typeof(InvalidOperationException), () => server.Start()); + } + [Test] public void UnstartedServerDoesNotPreventShutdown() { diff --git a/src/csharp/Grpc.Core/Server.cs b/src/csharp/Grpc.Core/Server.cs index ae7a8c9a9a1..3b554e5e87e 100644 --- a/src/csharp/Grpc.Core/Server.cs +++ b/src/csharp/Grpc.Core/Server.cs @@ -140,6 +140,7 @@ namespace Grpc.Core lock (myLock) { GrpcPreconditions.CheckState(!startRequested); + GrpcPreconditions.CheckState(!shutdownRequested); startRequested = true; handle.Start(); @@ -203,7 +204,6 @@ namespace Grpc.Core { lock (myLock) { - GrpcPreconditions.CheckState(startRequested); GrpcPreconditions.CheckState(!shutdownRequested); shutdownRequested = true; } @@ -215,7 +215,6 @@ namespace Grpc.Core { handle.CancelAllCalls(); } - await ShutdownCompleteOrEnvironmentDeadAsync().ConfigureAwait(false); DisposeHandle(); From 857495ae449a3c528978047e716341e9e316d64b Mon Sep 17 00:00:00 2001 From: Makarand Dharmapurikar Date: Thu, 9 Jun 2016 14:25:31 -0700 Subject: [PATCH 13/13] fix for #6834 Removed constants kPackageName and kServiceName so multiple services can be defined in one proto. --- src/compiler/objective_c_generator.cc | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/src/compiler/objective_c_generator.cc b/src/compiler/objective_c_generator.cc index 71a674174d7..6e14d2446f9 100644 --- a/src/compiler/objective_c_generator.cc +++ b/src/compiler/objective_c_generator.cc @@ -220,18 +220,13 @@ void PrintMethodImplementations(Printer *printer, {"service_class", ServiceClassName(service)}, {"package", service->file()->package()}}; - printer.Print(vars, - "static NSString *const kPackageName = @\"$package$\";\n"); - printer.Print( - vars, "static NSString *const kServiceName = @\"$service_name$\";\n\n"); - printer.Print(vars, "@implementation $service_class$\n\n"); printer.Print("// Designated initializer\n"); printer.Print("- (instancetype)initWithHost:(NSString *)host {\n"); - printer.Print( + printer.Print(vars, " return (self = [super initWithHost:host" - " packageName:kPackageName serviceName:kServiceName]);\n"); + " packageName:@\"$package$\" serviceName:@\"$service_name$\"]);\n"); printer.Print("}\n\n"); printer.Print( "// Override superclass initializer to disallow different"