From 3cdbee6cbc580629b64e0d92e745835ebb9bd95d Mon Sep 17 00:00:00 2001 From: haorenfsa Date: Wed, 26 Apr 2017 22:30:10 +0800 Subject: read_fd should always have a certain value when create error occurs [fix: while using eventfd, when error occurs during creating eventfd, a random fd will be closed] --- src/core/lib/iomgr/wakeup_fd_eventfd.cc | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) (limited to 'src') diff --git a/src/core/lib/iomgr/wakeup_fd_eventfd.cc b/src/core/lib/iomgr/wakeup_fd_eventfd.cc index dcf7dab71f..d68c9ada1f 100644 --- a/src/core/lib/iomgr/wakeup_fd_eventfd.cc +++ b/src/core/lib/iomgr/wakeup_fd_eventfd.cc @@ -32,12 +32,11 @@ #include "src/core/lib/profiling/timers.h" static grpc_error* eventfd_create(grpc_wakeup_fd* fd_info) { - int efd = eventfd(0, EFD_NONBLOCK | EFD_CLOEXEC); - if (efd < 0) { + fd_info->read_fd = eventfd(0, EFD_NONBLOCK | EFD_CLOEXEC); + fd_info->write_fd = -1; + if (fd_info->read_fd < 0) { return GRPC_OS_ERROR(errno, "eventfd"); } - fd_info->read_fd = efd; - fd_info->write_fd = -1; return GRPC_ERROR_NONE; } -- cgit v1.2.3 From 5faf1b72edba62a06dd6b4273c2a36f570b7c949 Mon Sep 17 00:00:00 2001 From: Soheil Hassas Yeganeh Date: Sun, 7 Oct 2018 18:41:31 -0400 Subject: Avoid unnecessary ref/unref calls to get mdelem from slices. grpc_mdelem_from_slices() unref's the key and value. As a result, in quite a few cases on the hot path, we first ref slice, so that grpc_mdelem_from_slices() can unref them. Add grpc_mdelem_from_slices_no_unref() which does not unref() the input slices. This cuts 0.5% - 1.0% across app benchmarks. --- include/grpc/grpc.h | 2 +- .../ext/filters/client_channel/client_channel.cc | 2 +- .../ext/filters/http/client_authority_filter.cc | 5 ++-- .../transport/chttp2/transport/chttp2_transport.cc | 5 ++-- .../credentials/plugin/plugin_credentials.cc | 3 +-- src/core/lib/surface/channel.cc | 30 ++++++++++------------ src/core/lib/surface/channel.h | 4 +-- src/core/lib/transport/metadata.cc | 10 ++++++-- src/core/lib/transport/metadata.h | 8 ++++-- 9 files changed, 37 insertions(+), 32 deletions(-) (limited to 'src') diff --git a/include/grpc/grpc.h b/include/grpc/grpc.h index a9beee1c9e..416958ed96 100644 --- a/include/grpc/grpc.h +++ b/include/grpc/grpc.h @@ -211,7 +211,7 @@ GRPCAPI int grpc_channel_support_connectivity_watcher(grpc_channel* channel); possible values). */ GRPCAPI grpc_call* grpc_channel_create_call( grpc_channel* channel, grpc_call* parent_call, uint32_t propagation_mask, - grpc_completion_queue* completion_queue, grpc_slice method, + grpc_completion_queue* completion_queue, const grpc_slice& method, const grpc_slice* host, gpr_timespec deadline, void* reserved); /** Ping the channels peer (load balanced channels will select one sub-channel diff --git a/src/core/ext/filters/client_channel/client_channel.cc b/src/core/ext/filters/client_channel/client_channel.cc index 388736b60a..3df5f6f2a0 100644 --- a/src/core/ext/filters/client_channel/client_channel.cc +++ b/src/core/ext/filters/client_channel/client_channel.cc @@ -2211,7 +2211,7 @@ static void add_retriable_send_initial_metadata_op( .grpc_previous_rpc_attempts); } if (GPR_UNLIKELY(calld->num_attempts_completed > 0)) { - grpc_mdelem retry_md = grpc_mdelem_from_slices( + grpc_mdelem retry_md = grpc_mdelem_from_slices_no_unref( GRPC_MDSTR_GRPC_PREVIOUS_RPC_ATTEMPTS, *retry_count_strings[calld->num_attempts_completed - 1]); grpc_error* error = grpc_metadata_batch_add_tail( diff --git a/src/core/ext/filters/http/client_authority_filter.cc b/src/core/ext/filters/http/client_authority_filter.cc index 1ca20ebb26..40b3ea22fd 100644 --- a/src/core/ext/filters/http/client_authority_filter.cc +++ b/src/core/ext/filters/http/client_authority_filter.cc @@ -59,9 +59,8 @@ void authority_start_transport_stream_op_batch( initial_metadata->idx.named.authority == nullptr) { grpc_error* error = grpc_metadata_batch_add_head( initial_metadata, &calld->authority_storage, - grpc_mdelem_from_slices( - GRPC_MDSTR_AUTHORITY, - grpc_slice_ref_internal(chand->default_authority))); + grpc_mdelem_from_slices_no_unref(GRPC_MDSTR_AUTHORITY, + chand->default_authority)); if (error != GRPC_ERROR_NONE) { grpc_transport_stream_op_batch_finish_with_failure(batch, error, calld->call_combiner); diff --git a/src/core/ext/transport/chttp2/transport/chttp2_transport.cc b/src/core/ext/transport/chttp2/transport/chttp2_transport.cc index 2d4b4da4c6..77a0a69393 100644 --- a/src/core/ext/transport/chttp2/transport/chttp2_transport.cc +++ b/src/core/ext/transport/chttp2/transport/chttp2_transport.cc @@ -2127,9 +2127,8 @@ void grpc_chttp2_fake_status(grpc_chttp2_transport* t, grpc_chttp2_stream* s, GRPC_LOG_IF_ERROR( "add_status_message", grpc_chttp2_incoming_metadata_buffer_replace_or_add( - &s->metadata_buffer[1], - grpc_mdelem_from_slices(GRPC_MDSTR_GRPC_MESSAGE, - grpc_slice_ref_internal(slice)))); + &s->metadata_buffer[1], grpc_mdelem_from_slices_no_unref( + GRPC_MDSTR_GRPC_MESSAGE, slice))); } s->published_metadata[1] = GRPC_METADATA_SYNTHESIZED_FROM_FAKE; grpc_chttp2_maybe_complete_recv_trailing_metadata(t, s); diff --git a/src/core/lib/security/credentials/plugin/plugin_credentials.cc b/src/core/lib/security/credentials/plugin/plugin_credentials.cc index 73946ce039..d97963d891 100644 --- a/src/core/lib/security/credentials/plugin/plugin_credentials.cc +++ b/src/core/lib/security/credentials/plugin/plugin_credentials.cc @@ -102,8 +102,7 @@ static grpc_error* process_plugin_result( } else { for (size_t i = 0; i < num_md; ++i) { grpc_mdelem mdelem = - grpc_mdelem_from_slices(grpc_slice_ref_internal(md[i].key), - grpc_slice_ref_internal(md[i].value)); + grpc_mdelem_from_slices_no_unref(md[i].key, md[i].value); grpc_credentials_mdelem_array_add(r->md_array, mdelem); GRPC_MDELEM_UNREF(mdelem); } diff --git a/src/core/lib/surface/channel.cc b/src/core/lib/surface/channel.cc index 4294434504..cd0b3cb458 100644 --- a/src/core/lib/surface/channel.cc +++ b/src/core/lib/surface/channel.cc @@ -326,20 +326,18 @@ static grpc_call* grpc_channel_create_call_internal( return call; } -grpc_call* grpc_channel_create_call(grpc_channel* channel, - grpc_call* parent_call, - uint32_t propagation_mask, - grpc_completion_queue* cq, - grpc_slice method, const grpc_slice* host, - gpr_timespec deadline, void* reserved) { +grpc_call* grpc_channel_create_call( + grpc_channel* channel, grpc_call* parent_call, uint32_t propagation_mask, + grpc_completion_queue* cq, const grpc_slice& method, const grpc_slice* host, + gpr_timespec deadline, void* reserved) { GPR_ASSERT(!reserved); grpc_core::ExecCtx exec_ctx; grpc_call* call = grpc_channel_create_call_internal( channel, parent_call, propagation_mask, cq, nullptr, - grpc_mdelem_from_slices(GRPC_MDSTR_PATH, grpc_slice_ref_internal(method)), - host != nullptr ? grpc_mdelem_from_slices(GRPC_MDSTR_AUTHORITY, - grpc_slice_ref_internal(*host)) - : GRPC_MDNULL, + grpc_mdelem_from_slices_no_unref(GRPC_MDSTR_PATH, method), + host != nullptr + ? grpc_mdelem_from_slices_no_unref(GRPC_MDSTR_AUTHORITY, *host) + : GRPC_MDNULL, grpc_timespec_to_millis_round_up(deadline)); return call; @@ -347,15 +345,15 @@ grpc_call* grpc_channel_create_call(grpc_channel* channel, grpc_call* grpc_channel_create_pollset_set_call( grpc_channel* channel, grpc_call* parent_call, uint32_t propagation_mask, - grpc_pollset_set* pollset_set, grpc_slice method, const grpc_slice* host, - grpc_millis deadline, void* reserved) { + grpc_pollset_set* pollset_set, const grpc_slice& method, + const grpc_slice* host, grpc_millis deadline, void* reserved) { GPR_ASSERT(!reserved); return grpc_channel_create_call_internal( channel, parent_call, propagation_mask, nullptr, pollset_set, - grpc_mdelem_from_slices(GRPC_MDSTR_PATH, grpc_slice_ref_internal(method)), - host != nullptr ? grpc_mdelem_from_slices(GRPC_MDSTR_AUTHORITY, - grpc_slice_ref_internal(*host)) - : GRPC_MDNULL, + grpc_mdelem_from_slices_no_unref(GRPC_MDSTR_PATH, method), + host != nullptr + ? grpc_mdelem_from_slices_no_unref(GRPC_MDSTR_AUTHORITY, *host) + : GRPC_MDNULL, deadline); } diff --git a/src/core/lib/surface/channel.h b/src/core/lib/surface/channel.h index e5ff2c3596..4ac76b8a29 100644 --- a/src/core/lib/surface/channel.h +++ b/src/core/lib/surface/channel.h @@ -45,8 +45,8 @@ grpc_channel* grpc_channel_create_with_builder( value of \a propagation_mask (see propagation_bits.h for possible values) */ grpc_call* grpc_channel_create_pollset_set_call( grpc_channel* channel, grpc_call* parent_call, uint32_t propagation_mask, - grpc_pollset_set* pollset_set, grpc_slice method, const grpc_slice* host, - grpc_millis deadline, void* reserved); + grpc_pollset_set* pollset_set, const grpc_slice& method, + const grpc_slice* host, grpc_millis deadline, void* reserved); /** Get a (borrowed) pointer to this channels underlying channel stack */ grpc_channel_stack* grpc_channel_get_channel_stack(grpc_channel* channel); diff --git a/src/core/lib/transport/metadata.cc b/src/core/lib/transport/metadata.cc index d164502280..8f84e06fae 100644 --- a/src/core/lib/transport/metadata.cc +++ b/src/core/lib/transport/metadata.cc @@ -237,7 +237,7 @@ static void rehash_mdtab(mdtab_shard* shard) { } grpc_mdelem grpc_mdelem_create( - grpc_slice key, grpc_slice value, + const grpc_slice& key, const grpc_slice& value, grpc_mdelem_data* compatible_external_backing_store) { if (!grpc_slice_is_interned(key) || !grpc_slice_is_interned(value)) { if (compatible_external_backing_store != nullptr) { @@ -324,13 +324,19 @@ grpc_mdelem grpc_mdelem_create( return GRPC_MAKE_MDELEM(md, GRPC_MDELEM_STORAGE_INTERNED); } -grpc_mdelem grpc_mdelem_from_slices(grpc_slice key, grpc_slice value) { +grpc_mdelem grpc_mdelem_from_slices(const grpc_slice& key, + const grpc_slice& value) { grpc_mdelem out = grpc_mdelem_create(key, value, nullptr); grpc_slice_unref_internal(key); grpc_slice_unref_internal(value); return out; } +grpc_mdelem grpc_mdelem_from_slices_no_unref(const grpc_slice& key, + const grpc_slice& value) { + return grpc_mdelem_create(key, value, nullptr); +} + grpc_mdelem grpc_mdelem_from_grpc_metadata(grpc_metadata* metadata) { bool changed = false; grpc_slice key_slice = diff --git a/src/core/lib/transport/metadata.h b/src/core/lib/transport/metadata.h index 338082276c..22481d6824 100644 --- a/src/core/lib/transport/metadata.h +++ b/src/core/lib/transport/metadata.h @@ -109,7 +109,11 @@ struct grpc_mdelem { (uintptr_t)GRPC_MDELEM_STORAGE_INTERNED_BIT)) /* Unrefs the slices. */ -grpc_mdelem grpc_mdelem_from_slices(grpc_slice key, grpc_slice value); +grpc_mdelem grpc_mdelem_from_slices(const grpc_slice& key, + const grpc_slice& value); +/* Does not unref the slices. */ +grpc_mdelem grpc_mdelem_from_slices_no_unref(const grpc_slice& key, + const grpc_slice& value); /* Cheaply convert a grpc_metadata to a grpc_mdelem; may use the grpc_metadata object as backing storage (so lifetimes should align) */ @@ -120,7 +124,7 @@ grpc_mdelem grpc_mdelem_from_grpc_metadata(grpc_metadata* metadata); compatible_external_backing_store if it is non-NULL (in which case it's the users responsibility to ensure that it outlives usage) */ grpc_mdelem grpc_mdelem_create( - grpc_slice key, grpc_slice value, + const grpc_slice& key, const grpc_slice& value, grpc_mdelem_data* compatible_external_backing_store); bool grpc_mdelem_eq(grpc_mdelem a, grpc_mdelem b); -- cgit v1.2.3 From d6b140df030f4032a2e039b0af174374141300b0 Mon Sep 17 00:00:00 2001 From: Soheil Hassas Yeganeh Date: Mon, 8 Oct 2018 16:19:50 -0400 Subject: Revert the change in grpc.h because it's part of C API. I mistakenly added "const ref" which breaks Android client. --- include/grpc/grpc.h | 2 +- src/core/lib/surface/channel.cc | 10 ++++++---- 2 files changed, 7 insertions(+), 5 deletions(-) (limited to 'src') diff --git a/include/grpc/grpc.h b/include/grpc/grpc.h index 416958ed96..a9beee1c9e 100644 --- a/include/grpc/grpc.h +++ b/include/grpc/grpc.h @@ -211,7 +211,7 @@ GRPCAPI int grpc_channel_support_connectivity_watcher(grpc_channel* channel); possible values). */ GRPCAPI grpc_call* grpc_channel_create_call( grpc_channel* channel, grpc_call* parent_call, uint32_t propagation_mask, - grpc_completion_queue* completion_queue, const grpc_slice& method, + grpc_completion_queue* completion_queue, grpc_slice method, const grpc_slice* host, gpr_timespec deadline, void* reserved); /** Ping the channels peer (load balanced channels will select one sub-channel diff --git a/src/core/lib/surface/channel.cc b/src/core/lib/surface/channel.cc index cd0b3cb458..ebc532c4fd 100644 --- a/src/core/lib/surface/channel.cc +++ b/src/core/lib/surface/channel.cc @@ -326,10 +326,12 @@ static grpc_call* grpc_channel_create_call_internal( return call; } -grpc_call* grpc_channel_create_call( - grpc_channel* channel, grpc_call* parent_call, uint32_t propagation_mask, - grpc_completion_queue* cq, const grpc_slice& method, const grpc_slice* host, - gpr_timespec deadline, void* reserved) { +grpc_call* grpc_channel_create_call(grpc_channel* channel, + grpc_call* parent_call, + uint32_t propagation_mask, + grpc_completion_queue* cq, + grpc_slice method, const grpc_slice* host, + gpr_timespec deadline, void* reserved) { GPR_ASSERT(!reserved); grpc_core::ExecCtx exec_ctx; grpc_call* call = grpc_channel_create_call_internal( -- cgit v1.2.3 From 10f995d283607a161e2ddcae5dd94636de950ec5 Mon Sep 17 00:00:00 2001 From: ncteisen Date: Tue, 9 Oct 2018 14:33:41 -0700 Subject: Enable channelz by default --- src/core/lib/channel/channelz.h | 4 ++-- test/core/channel/channelz_test.cc | 9 ++++++++- test/core/end2end/tests/channelz.cc | 16 +++++++++++++--- 3 files changed, 23 insertions(+), 6 deletions(-) (limited to 'src') diff --git a/src/core/lib/channel/channelz.h b/src/core/lib/channel/channelz.h index fddef793fb..88551befc8 100644 --- a/src/core/lib/channel/channelz.h +++ b/src/core/lib/channel/channelz.h @@ -42,13 +42,13 @@ /** This is the default value for whether or not to enable channelz. If * GRPC_ARG_ENABLE_CHANNELZ is set, it will override this default value. */ -#define GRPC_ENABLE_CHANNELZ_DEFAULT false +#define GRPC_ENABLE_CHANNELZ_DEFAULT true /** This is the default value for the maximum amount of memory used by trace * events per channel trace node. If * GRPC_ARG_MAX_CHANNEL_TRACE_EVENT_MEMORY_PER_NODE is set, it will override * this default value. */ -#define GRPC_MAX_CHANNEL_TRACE_EVENT_MEMORY_PER_NODE_DEFAULT 0 +#define GRPC_MAX_CHANNEL_TRACE_EVENT_MEMORY_PER_NODE_DEFAULT 1024 * 4 namespace grpc_core { diff --git a/test/core/channel/channelz_test.cc b/test/core/channel/channelz_test.cc index aed1fba47c..b7cdde581a 100644 --- a/test/core/channel/channelz_test.cc +++ b/test/core/channel/channelz_test.cc @@ -238,8 +238,15 @@ TEST_P(ChannelzChannelTest, BasicChannel) { TEST(ChannelzChannelTest, ChannelzDisabled) { grpc_core::ExecCtx exec_ctx; + // explicitly disable channelz + grpc_arg arg[2]; + arg[0] = grpc_channel_arg_integer_create( + const_cast(GRPC_ARG_MAX_CHANNEL_TRACE_EVENT_MEMORY_PER_NODE), 0); + arg[1] = grpc_channel_arg_integer_create( + const_cast(GRPC_ARG_ENABLE_CHANNELZ), false); + grpc_channel_args args = {GPR_ARRAY_SIZE(arg), arg}; grpc_channel* channel = - grpc_insecure_channel_create("fake_target", nullptr, nullptr); + grpc_insecure_channel_create("fake_target", &args, nullptr); ChannelNode* channelz_channel = grpc_channel_get_channelz_node(channel); ASSERT_EQ(channelz_channel, nullptr); grpc_channel_destroy(channel); diff --git a/test/core/end2end/tests/channelz.cc b/test/core/end2end/tests/channelz.cc index 41549190e3..da8a8eca42 100644 --- a/test/core/end2end/tests/channelz.cc +++ b/test/core/end2end/tests/channelz.cc @@ -199,9 +199,12 @@ static void run_one_request(grpc_end2end_test_config config, static void test_channelz(grpc_end2end_test_config config) { grpc_end2end_test_fixture f; - grpc_arg arg = grpc_channel_arg_integer_create( + grpc_arg arg[2]; + arg[0] = grpc_channel_arg_integer_create( + const_cast(GRPC_ARG_MAX_CHANNEL_TRACE_EVENT_MEMORY_PER_NODE), 0); + arg[1] = grpc_channel_arg_integer_create( const_cast(GRPC_ARG_ENABLE_CHANNELZ), true); - grpc_channel_args args = {1, &arg}; + grpc_channel_args args = {GPR_ARRAY_SIZE(arg), arg}; f = begin_test(config, "test_channelz", &args, &args); grpc_core::channelz::ChannelNode* channelz_channel = @@ -307,7 +310,14 @@ static void test_channelz_with_channel_trace(grpc_end2end_test_config config) { static void test_channelz_disabled(grpc_end2end_test_config config) { grpc_end2end_test_fixture f; - f = begin_test(config, "test_channelz_disabled", nullptr, nullptr); + grpc_arg arg[2]; + arg[0] = grpc_channel_arg_integer_create( + const_cast(GRPC_ARG_MAX_CHANNEL_TRACE_EVENT_MEMORY_PER_NODE), 0); + arg[1] = grpc_channel_arg_integer_create( + const_cast(GRPC_ARG_ENABLE_CHANNELZ), false); + grpc_channel_args args = {GPR_ARRAY_SIZE(arg), arg}; + + f = begin_test(config, "test_channelz_disabled", &args, &args); grpc_core::channelz::ChannelNode* channelz_channel = grpc_channel_get_channelz_node(f.client); GPR_ASSERT(channelz_channel == nullptr); -- cgit v1.2.3 From a280d89937db5a2f14aa05642dccfd00131498cc Mon Sep 17 00:00:00 2001 From: Soheil Hassas Yeganeh Date: Sun, 14 Oct 2018 22:47:15 -0400 Subject: Use grpc_mdelem_create() directly. Remove grpc_mdelem_from_slices_no_unref() since it's a wrapper around grpc_mdelem_create(). --- src/core/ext/filters/client_channel/client_channel.cc | 4 ++-- src/core/ext/filters/http/client_authority_filter.cc | 4 ++-- src/core/ext/transport/chttp2/transport/chttp2_transport.cc | 4 ++-- src/core/lib/security/credentials/plugin/plugin_credentials.cc | 2 +- src/core/lib/surface/channel.cc | 8 ++++---- src/core/lib/transport/metadata.cc | 5 ----- src/core/lib/transport/metadata.h | 3 --- 7 files changed, 11 insertions(+), 19 deletions(-) (limited to 'src') diff --git a/src/core/ext/filters/client_channel/client_channel.cc b/src/core/ext/filters/client_channel/client_channel.cc index 3df5f6f2a0..d2c88742bf 100644 --- a/src/core/ext/filters/client_channel/client_channel.cc +++ b/src/core/ext/filters/client_channel/client_channel.cc @@ -2211,9 +2211,9 @@ static void add_retriable_send_initial_metadata_op( .grpc_previous_rpc_attempts); } if (GPR_UNLIKELY(calld->num_attempts_completed > 0)) { - grpc_mdelem retry_md = grpc_mdelem_from_slices_no_unref( + grpc_mdelem retry_md = grpc_mdelem_create( GRPC_MDSTR_GRPC_PREVIOUS_RPC_ATTEMPTS, - *retry_count_strings[calld->num_attempts_completed - 1]); + *retry_count_strings[calld->num_attempts_completed - 1], nullptr); grpc_error* error = grpc_metadata_batch_add_tail( &retry_state->send_initial_metadata, &retry_state->send_initial_metadata_storage[calld->send_initial_metadata diff --git a/src/core/ext/filters/http/client_authority_filter.cc b/src/core/ext/filters/http/client_authority_filter.cc index 40b3ea22fd..6383f12594 100644 --- a/src/core/ext/filters/http/client_authority_filter.cc +++ b/src/core/ext/filters/http/client_authority_filter.cc @@ -59,8 +59,8 @@ void authority_start_transport_stream_op_batch( initial_metadata->idx.named.authority == nullptr) { grpc_error* error = grpc_metadata_batch_add_head( initial_metadata, &calld->authority_storage, - grpc_mdelem_from_slices_no_unref(GRPC_MDSTR_AUTHORITY, - chand->default_authority)); + grpc_mdelem_create(GRPC_MDSTR_AUTHORITY, chand->default_authority, + nullptr)); if (error != GRPC_ERROR_NONE) { grpc_transport_stream_op_batch_finish_with_failure(batch, error, calld->call_combiner); diff --git a/src/core/ext/transport/chttp2/transport/chttp2_transport.cc b/src/core/ext/transport/chttp2/transport/chttp2_transport.cc index 77a0a69393..8a481bb7d5 100644 --- a/src/core/ext/transport/chttp2/transport/chttp2_transport.cc +++ b/src/core/ext/transport/chttp2/transport/chttp2_transport.cc @@ -2127,8 +2127,8 @@ void grpc_chttp2_fake_status(grpc_chttp2_transport* t, grpc_chttp2_stream* s, GRPC_LOG_IF_ERROR( "add_status_message", grpc_chttp2_incoming_metadata_buffer_replace_or_add( - &s->metadata_buffer[1], grpc_mdelem_from_slices_no_unref( - GRPC_MDSTR_GRPC_MESSAGE, slice))); + &s->metadata_buffer[1], + grpc_mdelem_create(GRPC_MDSTR_GRPC_MESSAGE, slice, nullptr))); } s->published_metadata[1] = GRPC_METADATA_SYNTHESIZED_FROM_FAKE; grpc_chttp2_maybe_complete_recv_trailing_metadata(t, s); diff --git a/src/core/lib/security/credentials/plugin/plugin_credentials.cc b/src/core/lib/security/credentials/plugin/plugin_credentials.cc index d97963d891..4015124298 100644 --- a/src/core/lib/security/credentials/plugin/plugin_credentials.cc +++ b/src/core/lib/security/credentials/plugin/plugin_credentials.cc @@ -102,7 +102,7 @@ static grpc_error* process_plugin_result( } else { for (size_t i = 0; i < num_md; ++i) { grpc_mdelem mdelem = - grpc_mdelem_from_slices_no_unref(md[i].key, md[i].value); + grpc_mdelem_create(md[i].key, md[i].value, nullptr); grpc_credentials_mdelem_array_add(r->md_array, mdelem); GRPC_MDELEM_UNREF(mdelem); } diff --git a/src/core/lib/surface/channel.cc b/src/core/lib/surface/channel.cc index ebc532c4fd..044241470b 100644 --- a/src/core/lib/surface/channel.cc +++ b/src/core/lib/surface/channel.cc @@ -336,9 +336,9 @@ grpc_call* grpc_channel_create_call(grpc_channel* channel, grpc_core::ExecCtx exec_ctx; grpc_call* call = grpc_channel_create_call_internal( channel, parent_call, propagation_mask, cq, nullptr, - grpc_mdelem_from_slices_no_unref(GRPC_MDSTR_PATH, method), + grpc_mdelem_create(GRPC_MDSTR_PATH, method, nullptr), host != nullptr - ? grpc_mdelem_from_slices_no_unref(GRPC_MDSTR_AUTHORITY, *host) + ? grpc_mdelem_create(GRPC_MDSTR_AUTHORITY, *host, nullptr) : GRPC_MDNULL, grpc_timespec_to_millis_round_up(deadline)); @@ -352,9 +352,9 @@ grpc_call* grpc_channel_create_pollset_set_call( GPR_ASSERT(!reserved); return grpc_channel_create_call_internal( channel, parent_call, propagation_mask, nullptr, pollset_set, - grpc_mdelem_from_slices_no_unref(GRPC_MDSTR_PATH, method), + grpc_mdelem_create(GRPC_MDSTR_PATH, method, nullptr), host != nullptr - ? grpc_mdelem_from_slices_no_unref(GRPC_MDSTR_AUTHORITY, *host) + ? grpc_mdelem_create(GRPC_MDSTR_AUTHORITY, *host, nullptr) : GRPC_MDNULL, deadline); } diff --git a/src/core/lib/transport/metadata.cc b/src/core/lib/transport/metadata.cc index 8f84e06fae..60af22393e 100644 --- a/src/core/lib/transport/metadata.cc +++ b/src/core/lib/transport/metadata.cc @@ -332,11 +332,6 @@ grpc_mdelem grpc_mdelem_from_slices(const grpc_slice& key, return out; } -grpc_mdelem grpc_mdelem_from_slices_no_unref(const grpc_slice& key, - const grpc_slice& value) { - return grpc_mdelem_create(key, value, nullptr); -} - grpc_mdelem grpc_mdelem_from_grpc_metadata(grpc_metadata* metadata) { bool changed = false; grpc_slice key_slice = diff --git a/src/core/lib/transport/metadata.h b/src/core/lib/transport/metadata.h index 22481d6824..989c7544c1 100644 --- a/src/core/lib/transport/metadata.h +++ b/src/core/lib/transport/metadata.h @@ -111,9 +111,6 @@ struct grpc_mdelem { /* Unrefs the slices. */ grpc_mdelem grpc_mdelem_from_slices(const grpc_slice& key, const grpc_slice& value); -/* Does not unref the slices. */ -grpc_mdelem grpc_mdelem_from_slices_no_unref(const grpc_slice& key, - const grpc_slice& value); /* Cheaply convert a grpc_metadata to a grpc_mdelem; may use the grpc_metadata object as backing storage (so lifetimes should align) */ -- cgit v1.2.3 From 9929a23ecdf1b221f7b93481206f3dc87ffdd308 Mon Sep 17 00:00:00 2001 From: Jan Tattermusch Date: Tue, 9 Oct 2018 11:24:31 +0200 Subject: add dlerror stubs --- src/csharp/Grpc.Core/Internal/UnmanagedLibrary.cs | 12 ++++++++++++ 1 file changed, 12 insertions(+) (limited to 'src') diff --git a/src/csharp/Grpc.Core/Internal/UnmanagedLibrary.cs b/src/csharp/Grpc.Core/Internal/UnmanagedLibrary.cs index 7185d68efe..63c6c1f2f2 100644 --- a/src/csharp/Grpc.Core/Internal/UnmanagedLibrary.cs +++ b/src/csharp/Grpc.Core/Internal/UnmanagedLibrary.cs @@ -183,6 +183,9 @@ namespace Grpc.Core.Internal [DllImport("libdl.so")] internal static extern IntPtr dlopen(string filename, int flags); + [DllImport("libdl.so")] + internal static extern IntPtr dlerror(); + [DllImport("libdl.so")] internal static extern IntPtr dlsym(IntPtr handle, string symbol); } @@ -192,6 +195,9 @@ namespace Grpc.Core.Internal [DllImport("libSystem.dylib")] internal static extern IntPtr dlopen(string filename, int flags); + [DllImport("libSystem.dylib")] + internal static extern IntPtr dlerror(); + [DllImport("libSystem.dylib")] internal static extern IntPtr dlsym(IntPtr handle, string symbol); } @@ -208,6 +214,9 @@ namespace Grpc.Core.Internal [DllImport("__Internal")] internal static extern IntPtr dlopen(string filename, int flags); + [DllImport("__Internal")] + internal static extern IntPtr dlerror(); + [DllImport("__Internal")] internal static extern IntPtr dlsym(IntPtr handle, string symbol); } @@ -222,6 +231,9 @@ namespace Grpc.Core.Internal [DllImport("libcoreclr.so")] internal static extern IntPtr dlopen(string filename, int flags); + [DllImport("libcoreclr.so")] + internal static extern IntPtr dlerror(); + [DllImport("libcoreclr.so")] internal static extern IntPtr dlsym(IntPtr handle, string symbol); } -- cgit v1.2.3 From a959b6d7d2b6df11afe5381390fbfae6000675d3 Mon Sep 17 00:00:00 2001 From: Jan Tattermusch Date: Tue, 9 Oct 2018 11:56:57 +0200 Subject: Show dlerror if grpc_csharp_ext load fails --- src/csharp/Grpc.Core/Internal/UnmanagedLibrary.cs | 28 +++++++++++++++++------ 1 file changed, 21 insertions(+), 7 deletions(-) (limited to 'src') diff --git a/src/csharp/Grpc.Core/Internal/UnmanagedLibrary.cs b/src/csharp/Grpc.Core/Internal/UnmanagedLibrary.cs index 63c6c1f2f2..1786fc2e3f 100644 --- a/src/csharp/Grpc.Core/Internal/UnmanagedLibrary.cs +++ b/src/csharp/Grpc.Core/Internal/UnmanagedLibrary.cs @@ -51,11 +51,12 @@ namespace Grpc.Core.Internal Logger.Debug("Attempting to load native library \"{0}\"", this.libraryPath); - this.handle = PlatformSpecificLoadLibrary(this.libraryPath); + this.handle = PlatformSpecificLoadLibrary(this.libraryPath, out string loadLibraryErrorDetail); if (this.handle == IntPtr.Zero) { - throw new IOException(string.Format("Error loading native library \"{0}\"", this.libraryPath)); + throw new IOException(string.Format("Error loading native library \"{0}\". {1}", + this.libraryPath, loadLibraryErrorDetail)); } } @@ -129,31 +130,44 @@ namespace Grpc.Core.Internal /// /// Loads library in a platform specific way. /// - private static IntPtr PlatformSpecificLoadLibrary(string libraryPath) + private static IntPtr PlatformSpecificLoadLibrary(string libraryPath, out string errorMsg) { if (PlatformApis.IsWindows) { + // TODO(jtattermusch): populate the error on Windows + errorMsg = null; return Windows.LoadLibrary(libraryPath); } if (PlatformApis.IsLinux) { if (PlatformApis.IsMono) { - return Mono.dlopen(libraryPath, RTLD_GLOBAL + RTLD_LAZY); + return LoadLibraryPosix(Mono.dlopen, Mono.dlerror, libraryPath, out errorMsg); } if (PlatformApis.IsNetCore) { - return CoreCLR.dlopen(libraryPath, RTLD_GLOBAL + RTLD_LAZY); + return LoadLibraryPosix(CoreCLR.dlopen, CoreCLR.dlerror, libraryPath, out errorMsg); } - return Linux.dlopen(libraryPath, RTLD_GLOBAL + RTLD_LAZY); + return LoadLibraryPosix(Linux.dlopen, Linux.dlerror, libraryPath, out errorMsg); } if (PlatformApis.IsMacOSX) { - return MacOSX.dlopen(libraryPath, RTLD_GLOBAL + RTLD_LAZY); + return LoadLibraryPosix(MacOSX.dlopen, MacOSX.dlerror, libraryPath, out errorMsg); } throw new InvalidOperationException("Unsupported platform."); } + private static IntPtr LoadLibraryPosix(Func dlopenFunc, Func dlerrorFunc, string libraryPath, out string errorMsg) + { + errorMsg = null; + IntPtr ret = dlopenFunc(libraryPath, RTLD_GLOBAL + RTLD_LAZY); + if (ret == IntPtr.Zero) + { + errorMsg = Marshal.PtrToStringAnsi(dlerrorFunc()); + } + return ret; + } + private static string FirstValidLibraryPath(string[] libraryPathAlternatives) { GrpcPreconditions.CheckArgument(libraryPathAlternatives.Length > 0, "libraryPathAlternatives cannot be empty."); -- cgit v1.2.3 From 08ae060a445ae544eb58455e751eb2b77e70b65c Mon Sep 17 00:00:00 2001 From: Soheil Hassas Yeganeh Date: Mon, 15 Oct 2018 11:33:30 -0400 Subject: Fix formatting errors introduced in a280d899. --- src/core/lib/surface/channel.cc | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) (limited to 'src') diff --git a/src/core/lib/surface/channel.cc b/src/core/lib/surface/channel.cc index 044241470b..d7095c24d4 100644 --- a/src/core/lib/surface/channel.cc +++ b/src/core/lib/surface/channel.cc @@ -337,9 +337,8 @@ grpc_call* grpc_channel_create_call(grpc_channel* channel, grpc_call* call = grpc_channel_create_call_internal( channel, parent_call, propagation_mask, cq, nullptr, grpc_mdelem_create(GRPC_MDSTR_PATH, method, nullptr), - host != nullptr - ? grpc_mdelem_create(GRPC_MDSTR_AUTHORITY, *host, nullptr) - : GRPC_MDNULL, + host != nullptr ? grpc_mdelem_create(GRPC_MDSTR_AUTHORITY, *host, nullptr) + : GRPC_MDNULL, grpc_timespec_to_millis_round_up(deadline)); return call; @@ -353,9 +352,8 @@ grpc_call* grpc_channel_create_pollset_set_call( return grpc_channel_create_call_internal( channel, parent_call, propagation_mask, nullptr, pollset_set, grpc_mdelem_create(GRPC_MDSTR_PATH, method, nullptr), - host != nullptr - ? grpc_mdelem_create(GRPC_MDSTR_AUTHORITY, *host, nullptr) - : GRPC_MDNULL, + host != nullptr ? grpc_mdelem_create(GRPC_MDSTR_AUTHORITY, *host, nullptr) + : GRPC_MDNULL, deadline); } -- cgit v1.2.3 From 39b8de9eff9e2bf3f93305cda08086912e45ba4d Mon Sep 17 00:00:00 2001 From: ncteisen Date: Mon, 15 Oct 2018 13:38:18 -0400 Subject: Add channel conn state tracing --- .../ext/filters/client_channel/client_channel.cc | 32 ++++++++++++++++++++++ .../ext/filters/client_channel/client_channel.h | 3 ++ .../client_channel/client_channel_channelz.cc | 1 + 3 files changed, 36 insertions(+) (limited to 'src') diff --git a/src/core/ext/filters/client_channel/client_channel.cc b/src/core/ext/filters/client_channel/client_channel.cc index bb3ea400d1..8b9033ede2 100644 --- a/src/core/ext/filters/client_channel/client_channel.cc +++ b/src/core/ext/filters/client_channel/client_channel.cc @@ -129,6 +129,8 @@ typedef struct client_channel_channel_data { grpc_core::UniquePtr info_lb_policy_name; /** service config in JSON form */ grpc_core::UniquePtr info_service_config_json; + /* backpointer to grpc_channel's channelz node */ + grpc_core::channelz::ClientChannelNode* channelz_channel; } channel_data; typedef struct { @@ -153,6 +155,23 @@ static void watch_lb_policy_locked(channel_data* chand, grpc_core::LoadBalancingPolicy* lb_policy, grpc_connectivity_state current_state); +static const char* channel_connectivity_state_change_string( + grpc_connectivity_state state) { + switch (state) { + case GRPC_CHANNEL_IDLE: + return "Channel state change to IDLE"; + case GRPC_CHANNEL_CONNECTING: + return "Channel state change to CONNECTING"; + case GRPC_CHANNEL_READY: + return "Channel state change to READY"; + case GRPC_CHANNEL_TRANSIENT_FAILURE: + return "Channel state change to TRANSIENT_FAILURE"; + case GRPC_CHANNEL_SHUTDOWN: + return "Channel state change to SHUTDOWN"; + } + GPR_UNREACHABLE_CODE(return "UNKNOWN"); +} + static void set_channel_connectivity_state_locked(channel_data* chand, grpc_connectivity_state state, grpc_error* error, @@ -177,6 +196,12 @@ static void set_channel_connectivity_state_locked(channel_data* chand, gpr_log(GPR_INFO, "chand=%p: setting connectivity state to %s", chand, grpc_connectivity_state_name(state)); } + if (chand->channelz_channel != nullptr) { + chand->channelz_channel->AddTraceEvent( + grpc_core::channelz::ChannelTrace::Severity::Info, + grpc_slice_from_static_string( + channel_connectivity_state_change_string(state))); + } grpc_connectivity_state_set(&chand->state_tracker, state, error, reason); } @@ -699,6 +724,7 @@ static grpc_error* cc_init_channel_elem(grpc_channel_element* elem, // Record enable_retries. arg = grpc_channel_args_find(args->channel_args, GRPC_ARG_ENABLE_RETRIES); chand->enable_retries = grpc_channel_arg_get_bool(arg, true); + chand->channelz_channel = nullptr; // Record client channel factory. arg = grpc_channel_args_find(args->channel_args, GRPC_ARG_CLIENT_CHANNEL_FACTORY); @@ -3208,6 +3234,12 @@ static void try_to_connect_locked(void* arg, grpc_error* error_ignored) { GRPC_CHANNEL_STACK_UNREF(chand->owning_stack, "try_to_connect"); } +void grpc_client_channel_set_channelz_node( + grpc_channel_element* elem, grpc_core::channelz::ClientChannelNode* node) { + channel_data* chand = static_cast(elem->channel_data); + chand->channelz_channel = node; +} + void grpc_client_channel_populate_child_refs( grpc_channel_element* elem, grpc_core::channelz::ChildRefsList* child_subchannels, diff --git a/src/core/ext/filters/client_channel/client_channel.h b/src/core/ext/filters/client_channel/client_channel.h index d64faaabd2..4935fd24d8 100644 --- a/src/core/ext/filters/client_channel/client_channel.h +++ b/src/core/ext/filters/client_channel/client_channel.h @@ -40,6 +40,9 @@ extern grpc_core::TraceFlag grpc_client_channel_trace; extern const grpc_channel_filter grpc_client_channel_filter; +void grpc_client_channel_set_channelz_node( + grpc_channel_element* elem, grpc_core::channelz::ClientChannelNode* node); + void grpc_client_channel_populate_child_refs( grpc_channel_element* elem, grpc_core::channelz::ChildRefsList* child_subchannels, diff --git a/src/core/ext/filters/client_channel/client_channel_channelz.cc b/src/core/ext/filters/client_channel/client_channel_channelz.cc index b66c920b90..bad1ef668c 100644 --- a/src/core/ext/filters/client_channel/client_channel_channelz.cc +++ b/src/core/ext/filters/client_channel/client_channel_channelz.cc @@ -49,6 +49,7 @@ ClientChannelNode::ClientChannelNode(grpc_channel* channel, : ChannelNode(channel, channel_tracer_max_nodes, is_top_level_channel) { client_channel_ = grpc_channel_stack_last_element(grpc_channel_get_channel_stack(channel)); + grpc_client_channel_set_channelz_node(client_channel_, this); GPR_ASSERT(client_channel_->filter == &grpc_client_channel_filter); } -- cgit v1.2.3 From c13260802edf099f33fab26aa6b6b43aa4f4e5c0 Mon Sep 17 00:00:00 2001 From: ncteisen Date: Mon, 15 Oct 2018 13:48:07 -0400 Subject: Add subchannnel conn state tracing --- src/core/ext/filters/client_channel/subchannel.cc | 51 ++++++++++++++++++----- 1 file changed, 40 insertions(+), 11 deletions(-) (limited to 'src') diff --git a/src/core/ext/filters/client_channel/subchannel.cc b/src/core/ext/filters/client_channel/subchannel.cc index 3a1c14c6f1..949670c4a0 100644 --- a/src/core/ext/filters/client_channel/subchannel.cc +++ b/src/core/ext/filters/client_channel/subchannel.cc @@ -428,6 +428,35 @@ intptr_t grpc_subchannel_get_child_socket_uuid(grpc_subchannel* subchannel) { } } +static const char* subchannel_connectivity_state_change_string( + grpc_connectivity_state state) { + switch (state) { + case GRPC_CHANNEL_IDLE: + return "Subchannel state change to IDLE"; + case GRPC_CHANNEL_CONNECTING: + return "Subchannel state change to CONNECTING"; + case GRPC_CHANNEL_READY: + return "Subchannel state change to READY"; + case GRPC_CHANNEL_TRANSIENT_FAILURE: + return "Subchannel state change to TRANSIENT_FAILURE"; + case GRPC_CHANNEL_SHUTDOWN: + return "Subchannel state change to SHUTDOWN"; + } + GPR_UNREACHABLE_CODE(return "UNKNOWN"); +} + +static void set_subchannel_connectivity_state_locked( + grpc_subchannel* c, grpc_connectivity_state state, grpc_error* error, + const char* reason) { + if (c->channelz_subchannel != nullptr) { + c->channelz_subchannel->AddTraceEvent( + grpc_core::channelz::ChannelTrace::Severity::Info, + grpc_slice_from_static_string( + subchannel_connectivity_state_change_string(state))); + } + grpc_connectivity_state_set(&c->state_tracker, state, error, reason); +} + static void continue_connect_locked(grpc_subchannel* c) { grpc_connect_in_args args; args.interested_parties = c->pollset_set; @@ -436,8 +465,8 @@ static void continue_connect_locked(grpc_subchannel* c) { c->next_attempt_deadline = c->backoff->NextAttemptTime(); args.deadline = std::max(c->next_attempt_deadline, min_deadline); args.channel_args = c->args; - grpc_connectivity_state_set(&c->state_tracker, GRPC_CHANNEL_CONNECTING, - GRPC_ERROR_NONE, "connecting"); + set_subchannel_connectivity_state_locked(c, GRPC_CHANNEL_CONNECTING, + GRPC_ERROR_NONE, "connecting"); grpc_connector_connect(c->connector, &args, &c->connecting_result, &c->on_connected); } @@ -587,9 +616,9 @@ static void on_connected_subchannel_connectivity_changed(void* p, connected_subchannel_watcher->connectivity_state)); } c->connected_subchannel.reset(); - grpc_connectivity_state_set(&c->state_tracker, - GRPC_CHANNEL_TRANSIENT_FAILURE, - GRPC_ERROR_REF(error), "reflect_child"); + set_subchannel_connectivity_state_locked( + c, GRPC_CHANNEL_TRANSIENT_FAILURE, GRPC_ERROR_REF(error), + "reflect_child"); c->backoff_begun = false; c->backoff->Reset(); maybe_start_connecting_locked(c); @@ -600,8 +629,8 @@ static void on_connected_subchannel_connectivity_changed(void* p, break; } default: { - grpc_connectivity_state_set( - &c->state_tracker, connected_subchannel_watcher->connectivity_state, + set_subchannel_connectivity_state_locked( + c, connected_subchannel_watcher->connectivity_state, GRPC_ERROR_REF(error), "reflect_child"); GRPC_SUBCHANNEL_WEAK_REF(c, "state_watcher"); c->connected_subchannel->NotifyOnStateChange( @@ -672,8 +701,8 @@ static bool publish_transport_locked(grpc_subchannel* c) { &connected_subchannel_watcher->closure); /* signal completion */ - grpc_connectivity_state_set(&c->state_tracker, GRPC_CHANNEL_READY, - GRPC_ERROR_NONE, "connected"); + set_subchannel_connectivity_state_locked(c, GRPC_CHANNEL_READY, + GRPC_ERROR_NONE, "connected"); return true; } @@ -690,8 +719,8 @@ static void on_subchannel_connected(void* arg, grpc_error* error) { } else if (c->disconnected) { GRPC_SUBCHANNEL_WEAK_UNREF(c, "connecting"); } else { - grpc_connectivity_state_set( - &c->state_tracker, GRPC_CHANNEL_TRANSIENT_FAILURE, + set_subchannel_connectivity_state_locked( + c, GRPC_CHANNEL_TRANSIENT_FAILURE, grpc_error_set_int(GRPC_ERROR_CREATE_REFERENCING_FROM_STATIC_STRING( "Connect Failed", &error, 1), GRPC_ERROR_INT_GRPC_STATUS, GRPC_STATUS_UNAVAILABLE), -- cgit v1.2.3 From c27d2fcbbe4bec9e57b6aaaf92d0a93457e60da8 Mon Sep 17 00:00:00 2001 From: ncteisen Date: Mon, 15 Oct 2018 15:59:17 -0400 Subject: Ban gevent test --- src/python/grpcio_tests/commands.py | 2 ++ 1 file changed, 2 insertions(+) (limited to 'src') diff --git a/src/python/grpcio_tests/commands.py b/src/python/grpcio_tests/commands.py index 0dfbf3180b..6931d93ef0 100644 --- a/src/python/grpcio_tests/commands.py +++ b/src/python/grpcio_tests/commands.py @@ -116,6 +116,8 @@ class TestGevent(setuptools.Command): # eventually succeed, but need to dig into performance issues. 'unit._cython._no_messages_server_completion_queue_per_call_test.Test.test_rpcs', 'unit._cython._no_messages_single_server_completion_queue_test.Test.test_rpcs', + # TODO(https://github.com/grpc/grpc/issues/16890) enable this test + 'unit._cython._channel_test.ChannelTest.test_multiple_channels_lonely_connectivity', # I have no idea why this doesn't work in gevent, but it shouldn't even be # using the c-core 'testing._client_test.ClientTest.test_infinite_request_stream_real_time', -- cgit v1.2.3 From f1d3d32f9cb0a0337b3de9b61af05b651a8cc4e3 Mon Sep 17 00:00:00 2001 From: ncteisen Date: Mon, 15 Oct 2018 19:45:45 -0400 Subject: Assume UNKNOWN if no status --- src/core/ext/filters/client_channel/subchannel.cc | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) (limited to 'src') diff --git a/src/core/ext/filters/client_channel/subchannel.cc b/src/core/ext/filters/client_channel/subchannel.cc index 3a1c14c6f1..78ff426b11 100644 --- a/src/core/ext/filters/client_channel/subchannel.cc +++ b/src/core/ext/filters/client_channel/subchannel.cc @@ -760,9 +760,12 @@ static void get_call_status(grpc_subchannel_call* call, grpc_error_get_status(error, call->deadline, status, nullptr, nullptr, nullptr); } else { - GPR_ASSERT(md_batch->idx.named.grpc_status != nullptr); - *status = - grpc_get_status_code_from_metadata(md_batch->idx.named.grpc_status->md); + if (md_batch->idx.named.grpc_status != nullptr) { + *status = grpc_get_status_code_from_metadata( + md_batch->idx.named.grpc_status->md); + } else { + *status = GRPC_STATUS_UNKNOWN; + } } GRPC_ERROR_UNREF(error); } -- cgit v1.2.3