diff --git a/BUILD b/BUILD index c3056cdba96..530904c7a5f 100644 --- a/BUILD +++ b/BUILD @@ -1722,6 +1722,7 @@ grpc_cc_library( "ref_counted_ptr", "slice", "slice_refcount", + "table", "useful", ], ) diff --git a/CMakeLists.txt b/CMakeLists.txt index 4baf73d4578..14f6e9903f2 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -917,6 +917,7 @@ if(gRPC_BUILD_TESTS) add_dependencies(buildtests_cxx match_test) add_dependencies(buildtests_cxx matchers_test) add_dependencies(buildtests_cxx message_allocator_end2end_test) + add_dependencies(buildtests_cxx metadata_map_test) add_dependencies(buildtests_cxx miscompile_with_no_unique_address_test) add_dependencies(buildtests_cxx mock_stream_test) add_dependencies(buildtests_cxx mock_test) @@ -2157,6 +2158,7 @@ target_link_libraries(grpc absl::bind_front absl::statusor absl::variant + absl::utility gpr ${_gRPC_SSL_LIBRARIES} address_sorting @@ -2715,6 +2717,7 @@ target_link_libraries(grpc_unsecure absl::bind_front absl::statusor absl::variant + absl::utility gpr address_sorting ) @@ -13325,6 +13328,41 @@ target_link_libraries(message_allocator_end2end_test ) +endif() +if(gRPC_BUILD_TESTS) + +add_executable(metadata_map_test + test/core/transport/metadata_map_test.cc + third_party/googletest/googletest/src/gtest-all.cc + third_party/googletest/googlemock/src/gmock-all.cc +) + +target_include_directories(metadata_map_test + PRIVATE + ${CMAKE_CURRENT_SOURCE_DIR} + ${CMAKE_CURRENT_SOURCE_DIR}/include + ${_gRPC_ADDRESS_SORTING_INCLUDE_DIR} + ${_gRPC_RE2_INCLUDE_DIR} + ${_gRPC_SSL_INCLUDE_DIR} + ${_gRPC_UPB_GENERATED_DIR} + ${_gRPC_UPB_GRPC_GENERATED_DIR} + ${_gRPC_UPB_INCLUDE_DIR} + ${_gRPC_XXHASH_INCLUDE_DIR} + ${_gRPC_ZLIB_INCLUDE_DIR} + third_party/googletest/googletest/include + third_party/googletest/googletest + third_party/googletest/googlemock/include + third_party/googletest/googlemock + ${_gRPC_PROTO_GENS_DIR} +) + +target_link_libraries(metadata_map_test + ${_gRPC_PROTOBUF_LIBRARIES} + ${_gRPC_ALLTARGETS_LIBRARIES} + grpc_test_util +) + + endif() if(gRPC_BUILD_TESTS) @@ -17254,7 +17292,7 @@ generate_pkgconfig( "gRPC" "high performance general RPC framework" "${gRPC_CORE_VERSION}" - "gpr openssl absl_base absl_bind_front absl_cord absl_core_headers absl_flat_hash_map absl_inlined_vector absl_memory absl_optional absl_status absl_statusor absl_str_format absl_strings absl_synchronization absl_time absl_variant" + "gpr openssl absl_base absl_bind_front absl_cord absl_core_headers absl_flat_hash_map absl_inlined_vector absl_memory absl_optional absl_status absl_statusor absl_str_format absl_strings absl_synchronization absl_time absl_utility absl_variant" "-lgrpc -laddress_sorting -lre2 -lupb -lcares -lz" "" "grpc.pc") @@ -17264,7 +17302,7 @@ generate_pkgconfig( "gRPC unsecure" "high performance general RPC framework without SSL" "${gRPC_CORE_VERSION}" - "gpr absl_base absl_bind_front absl_cord absl_core_headers absl_flat_hash_map absl_inlined_vector absl_memory absl_optional absl_status absl_statusor absl_str_format absl_strings absl_synchronization absl_time absl_variant" + "gpr absl_base absl_bind_front absl_cord absl_core_headers absl_flat_hash_map absl_inlined_vector absl_memory absl_optional absl_status absl_statusor absl_str_format absl_strings absl_synchronization absl_time absl_utility absl_variant" "-lgrpc_unsecure" "" "grpc_unsecure.pc") @@ -17274,7 +17312,7 @@ generate_pkgconfig( "gRPC++" "C++ wrapper for gRPC" "${gRPC_CPP_VERSION}" - "grpc absl_base absl_bind_front absl_cord absl_core_headers absl_flat_hash_map absl_inlined_vector absl_memory absl_optional absl_status absl_statusor absl_str_format absl_strings absl_synchronization absl_time absl_variant" + "grpc absl_base absl_bind_front absl_cord absl_core_headers absl_flat_hash_map absl_inlined_vector absl_memory absl_optional absl_status absl_statusor absl_str_format absl_strings absl_synchronization absl_time absl_utility absl_variant" "-lgrpc++" "" "grpc++.pc") @@ -17284,7 +17322,7 @@ generate_pkgconfig( "gRPC++ unsecure" "C++ wrapper for gRPC without SSL" "${gRPC_CPP_VERSION}" - "grpc_unsecure absl_base absl_bind_front absl_cord absl_core_headers absl_flat_hash_map absl_inlined_vector absl_memory absl_optional absl_status absl_statusor absl_str_format absl_strings absl_synchronization absl_time absl_variant" + "grpc_unsecure absl_base absl_bind_front absl_cord absl_core_headers absl_flat_hash_map absl_inlined_vector absl_memory absl_optional absl_status absl_statusor absl_str_format absl_strings absl_synchronization absl_time absl_utility absl_variant" "-lgrpc++_unsecure" "" "grpc++_unsecure.pc") diff --git a/build_autogenerated.yaml b/build_autogenerated.yaml index 4d22cb2be94..03c2643d5cf 100644 --- a/build_autogenerated.yaml +++ b/build_autogenerated.yaml @@ -761,6 +761,7 @@ libs: - src/core/lib/gprpp/overload.h - src/core/lib/gprpp/ref_counted.h - src/core/lib/gprpp/ref_counted_ptr.h + - src/core/lib/gprpp/table.h - src/core/lib/http/format_request.h - src/core/lib/http/httpcli.h - src/core/lib/http/parser.h @@ -1535,6 +1536,7 @@ libs: - absl/functional:bind_front - absl/status:statusor - absl/types:variant + - absl/utility:utility - gpr - libssl - address_sorting @@ -1815,6 +1817,7 @@ libs: - src/core/lib/gprpp/overload.h - src/core/lib/gprpp/ref_counted.h - src/core/lib/gprpp/ref_counted_ptr.h + - src/core/lib/gprpp/table.h - src/core/lib/http/format_request.h - src/core/lib/http/httpcli.h - src/core/lib/http/parser.h @@ -2232,6 +2235,7 @@ libs: - absl/functional:bind_front - absl/status:statusor - absl/types:variant + - absl/utility:utility - gpr - address_sorting baselib: true @@ -6622,6 +6626,15 @@ targets: - test/cpp/end2end/test_service_impl.cc deps: - grpc++_test_util +- name: metadata_map_test + gtest: true + build: test + language: c++ + headers: [] + src: + - test/core/transport/metadata_map_test.cc + deps: + - grpc_test_util - name: miscompile_with_no_unique_address_test gtest: true build: test diff --git a/gRPC-C++.podspec b/gRPC-C++.podspec index 09aebbd2fed..c7d182e62e7 100644 --- a/gRPC-C++.podspec +++ b/gRPC-C++.podspec @@ -203,6 +203,7 @@ Pod::Spec.new do |s| ss.dependency 'abseil/time/time', abseil_version ss.dependency 'abseil/types/optional', abseil_version ss.dependency 'abseil/types/variant', abseil_version + ss.dependency 'abseil/utility/utility', abseil_version ss.source_files = 'src/core/ext/filters/client_channel/backend_metric.h', 'src/core/ext/filters/client_channel/backup_poller.h', @@ -570,6 +571,7 @@ Pod::Spec.new do |s| 'src/core/lib/gprpp/stat.h', 'src/core/lib/gprpp/status_helper.h', 'src/core/lib/gprpp/sync.h', + 'src/core/lib/gprpp/table.h', 'src/core/lib/gprpp/thd.h', 'src/core/lib/gprpp/time_util.h', 'src/core/lib/http/format_request.h', @@ -1242,6 +1244,7 @@ Pod::Spec.new do |s| 'src/core/lib/gprpp/stat.h', 'src/core/lib/gprpp/status_helper.h', 'src/core/lib/gprpp/sync.h', + 'src/core/lib/gprpp/table.h', 'src/core/lib/gprpp/thd.h', 'src/core/lib/gprpp/time_util.h', 'src/core/lib/http/format_request.h', diff --git a/gRPC-Core.podspec b/gRPC-Core.podspec index 30d4538ddac..b3ce512ca0b 100644 --- a/gRPC-Core.podspec +++ b/gRPC-Core.podspec @@ -193,6 +193,7 @@ Pod::Spec.new do |s| ss.dependency 'abseil/time/time', abseil_version ss.dependency 'abseil/types/optional', abseil_version ss.dependency 'abseil/types/variant', abseil_version + ss.dependency 'abseil/utility/utility', abseil_version ss.compiler_flags = '-DBORINGSSL_PREFIX=GRPC -Wno-unreachable-code -Wno-shorten-64-to-32' ss.source_files = 'src/core/ext/filters/census/grpc_context.cc', @@ -950,6 +951,7 @@ Pod::Spec.new do |s| 'src/core/lib/gprpp/status_helper.cc', 'src/core/lib/gprpp/status_helper.h', 'src/core/lib/gprpp/sync.h', + 'src/core/lib/gprpp/table.h', 'src/core/lib/gprpp/thd.h', 'src/core/lib/gprpp/thd_posix.cc', 'src/core/lib/gprpp/thd_windows.cc', @@ -1830,6 +1832,7 @@ Pod::Spec.new do |s| 'src/core/lib/gprpp/stat.h', 'src/core/lib/gprpp/status_helper.h', 'src/core/lib/gprpp/sync.h', + 'src/core/lib/gprpp/table.h', 'src/core/lib/gprpp/thd.h', 'src/core/lib/gprpp/time_util.h', 'src/core/lib/http/format_request.h', diff --git a/grpc.gemspec b/grpc.gemspec index 69f111b5d05..85069c0007e 100644 --- a/grpc.gemspec +++ b/grpc.gemspec @@ -863,6 +863,7 @@ Gem::Specification.new do |s| s.files += %w( src/core/lib/gprpp/status_helper.cc ) s.files += %w( src/core/lib/gprpp/status_helper.h ) s.files += %w( src/core/lib/gprpp/sync.h ) + s.files += %w( src/core/lib/gprpp/table.h ) s.files += %w( src/core/lib/gprpp/thd.h ) s.files += %w( src/core/lib/gprpp/thd_posix.cc ) s.files += %w( src/core/lib/gprpp/thd_windows.cc ) diff --git a/grpc.gyp b/grpc.gyp index a596241a517..8df96d8852b 100644 --- a/grpc.gyp +++ b/grpc.gyp @@ -482,6 +482,7 @@ 'absl/functional:bind_front', 'absl/status:statusor', 'absl/types:variant', + 'absl/utility:utility', 'gpr', 'address_sorting', ], @@ -1145,6 +1146,7 @@ 'absl/functional:bind_front', 'absl/status:statusor', 'absl/types:variant', + 'absl/utility:utility', 'gpr', 'address_sorting', ], diff --git a/package.xml b/package.xml index 32ffe369e5e..302cf7f2e4e 100644 --- a/package.xml +++ b/package.xml @@ -843,6 +843,7 @@ + diff --git a/src/core/ext/filters/deadline/deadline_filter.cc b/src/core/ext/filters/deadline/deadline_filter.cc index 1009cb94f40..9bdd51fc240 100644 --- a/src/core/ext/filters/deadline/deadline_filter.cc +++ b/src/core/ext/filters/deadline/deadline_filter.cc @@ -114,9 +114,7 @@ class TimerState { // synchronized. static void start_timer_if_needed(grpc_call_element* elem, grpc_millis deadline) { - if (deadline == GRPC_MILLIS_INF_FUTURE) { - return; - } + if (deadline == GRPC_MILLIS_INF_FUTURE) return; grpc_deadline_state* deadline_state = static_cast(elem->call_data); GPR_ASSERT(deadline_state->timer_state == nullptr); @@ -295,7 +293,9 @@ static void deadline_client_start_transport_stream_op_batch( static void recv_initial_metadata_ready(void* arg, grpc_error_handle error) { grpc_call_element* elem = static_cast(arg); server_call_data* calld = static_cast(elem->call_data); - start_timer_if_needed(elem, calld->recv_initial_metadata->deadline()); + start_timer_if_needed( + elem, calld->recv_initial_metadata->get(grpc_core::GrpcTimeoutMetadata()) + .value_or(GRPC_MILLIS_INF_FUTURE)); // Invoke the next callback. grpc_core::Closure::Run(DEBUG_LOCATION, calld->next_recv_initial_metadata_ready, diff --git a/src/core/ext/transport/chttp2/transport/chttp2_transport.cc b/src/core/ext/transport/chttp2/transport/chttp2_transport.cc index 6d0356e7e6b..8ec6ed1000d 100644 --- a/src/core/ext/transport/chttp2/transport/chttp2_transport.cc +++ b/src/core/ext/transport/chttp2/transport/chttp2_transport.cc @@ -1431,7 +1431,10 @@ static void perform_stream_op_locked(void* stream_op, s->send_initial_metadata = op_payload->send_initial_metadata.send_initial_metadata; if (t->is_client) { - s->deadline = std::min(s->deadline, s->send_initial_metadata->deadline()); + s->deadline = std::min( + s->deadline, + s->send_initial_metadata->get(grpc_core::GrpcTimeoutMetadata()) + .value_or(GRPC_MILLIS_INF_FUTURE)); } if (contains_non_ok_status(s->send_initial_metadata)) { s->seen_error = true; @@ -1623,14 +1626,14 @@ static void perform_stream_op(grpc_transport* gt, grpc_stream* gs, if (!t->is_client) { if (op->send_initial_metadata) { - grpc_millis deadline = - op->payload->send_initial_metadata.send_initial_metadata->deadline(); - GPR_ASSERT(deadline == GRPC_MILLIS_INF_FUTURE); + GPR_ASSERT(!op->payload->send_initial_metadata.send_initial_metadata + ->get(grpc_core::GrpcTimeoutMetadata()) + .has_value()); } if (op->send_trailing_metadata) { - grpc_millis deadline = op->payload->send_trailing_metadata - .send_trailing_metadata->deadline(); - GPR_ASSERT(deadline == GRPC_MILLIS_INF_FUTURE); + GPR_ASSERT(!op->payload->send_trailing_metadata.send_trailing_metadata + ->get(grpc_core::GrpcTimeoutMetadata()) + .has_value()); } } diff --git a/src/core/ext/transport/chttp2/transport/hpack_encoder.cc b/src/core/ext/transport/chttp2/transport/hpack_encoder.cc index e45508885a4..965d88dda70 100644 --- a/src/core/ext/transport/chttp2/transport/hpack_encoder.cc +++ b/src/core/ext/transport/chttp2/transport/hpack_encoder.cc @@ -448,7 +448,8 @@ void HPackCompressor::Framer::EncodeDynamic(grpc_mdelem elem) { } } -void HPackCompressor::Framer::EncodeDeadline(grpc_millis deadline) { +void HPackCompressor::Framer::Encode(GrpcTimeoutMetadata, + grpc_millis deadline) { char timeout_str[GRPC_HTTP2_TIMEOUT_ENCODE_MIN_BUFSIZE]; grpc_mdelem mdelem; grpc_http2_encode_timeout(deadline - grpc_core::ExecCtx::Get()->Now(), diff --git a/src/core/ext/transport/chttp2/transport/hpack_encoder.h b/src/core/ext/transport/chttp2/transport/hpack_encoder.h index b2b4ee32116..e7eb982c974 100644 --- a/src/core/ext/transport/chttp2/transport/hpack_encoder.h +++ b/src/core/ext/transport/chttp2/transport/hpack_encoder.h @@ -117,7 +117,7 @@ class HPackCompressor { Framer& operator=(const Framer&) = delete; void Encode(grpc_mdelem md); - void EncodeDeadline(grpc_millis deadline); + void Encode(GrpcTimeoutMetadata, grpc_millis deadline); private: struct FramePrefix { diff --git a/src/core/ext/transport/chttp2/transport/incoming_metadata.cc b/src/core/ext/transport/chttp2/transport/incoming_metadata.cc index 7fc2dad09e8..65725ef255d 100644 --- a/src/core/ext/transport/chttp2/transport/incoming_metadata.cc +++ b/src/core/ext/transport/chttp2/transport/incoming_metadata.cc @@ -53,7 +53,11 @@ grpc_error_handle grpc_chttp2_incoming_metadata_buffer_replace_or_add( void grpc_chttp2_incoming_metadata_buffer_set_deadline( grpc_chttp2_incoming_metadata_buffer* buffer, grpc_millis deadline) { - buffer->batch.SetDeadline(deadline); + if (deadline != GRPC_MILLIS_INF_FUTURE) { + buffer->batch.Set(grpc_core::GrpcTimeoutMetadata(), deadline); + } else { + buffer->batch.Remove(grpc_core::GrpcTimeoutMetadata()); + } } void grpc_chttp2_incoming_metadata_buffer_publish( diff --git a/src/core/ext/transport/inproc/inproc_transport.cc b/src/core/ext/transport/inproc/inproc_transport.cc index 8d1c5279e09..83a80eea967 100644 --- a/src/core/ext/transport/inproc/inproc_transport.cc +++ b/src/core/ext/transport/inproc/inproc_transport.cc @@ -694,8 +694,11 @@ void op_state_machine_locked(inproc_stream* s, grpc_error_handle error) { .recv_initial_metadata, s->recv_initial_md_op->payload->recv_initial_metadata.recv_flags, nullptr); - s->recv_initial_md_op->payload->recv_initial_metadata - .recv_initial_metadata->SetDeadline(s->deadline); + if (s->deadline != GRPC_MILLIS_INF_FUTURE) { + s->recv_initial_md_op->payload->recv_initial_metadata + .recv_initial_metadata->Set(grpc_core::GrpcTimeoutMetadata(), + s->deadline); + } if (s->recv_initial_md_op->payload->recv_initial_metadata .trailing_metadata_available != nullptr) { *s->recv_initial_md_op->payload->recv_initial_metadata @@ -1016,8 +1019,10 @@ void perform_stream_op(grpc_transport* gt, grpc_stream* gs, if (s->t->is_client) { grpc_millis* dl = (other == nullptr) ? &s->write_buffer_deadline : &other->deadline; - *dl = std::min(*dl, op->payload->send_initial_metadata - .send_initial_metadata->deadline()); + *dl = std::min( + *dl, op->payload->send_initial_metadata.send_initial_metadata + ->get(grpc_core::GrpcTimeoutMetadata()) + .value_or(GRPC_MILLIS_INF_FUTURE)); s->initial_md_sent = true; } } diff --git a/src/core/lib/gprpp/bitset.h b/src/core/lib/gprpp/bitset.h index b8078746271..397fe394a79 100644 --- a/src/core/lib/gprpp/bitset.h +++ b/src/core/lib/gprpp/bitset.h @@ -169,6 +169,20 @@ class BitSet { Uint units_[kUnits]; }; +// Zero-size specialization of BitSet. +// Useful for generic programming. +// Make a compile time error out of get/set type accesses, and hard-codes +// queries that do make sense. +template <> +class BitSet<0> { + public: + constexpr BitSet() {} + + bool all() const { return true; } + bool none() const { return true; } + uint32_t count() const { return 0; } +}; + } // namespace grpc_core #endif // GRPC_CORE_LIB_GPRPP_BITSET_H diff --git a/src/core/lib/gprpp/table.h b/src/core/lib/gprpp/table.h index e51055f41fd..43953d1ca7b 100644 --- a/src/core/lib/gprpp/table.h +++ b/src/core/lib/gprpp/table.h @@ -375,7 +375,7 @@ class Table { // destructor. template void Destruct(absl::index_sequence) { - table_detail::do_these_things( + table_detail::do_these_things( {(table_detail::DestructIfNotNull(get()), 1)...}); } @@ -383,21 +383,21 @@ class Table { // or_clear as per CopyIf(). template void Copy(absl::index_sequence, const Table& rhs) { - table_detail::do_these_things({(CopyIf(rhs), 1)...}); + table_detail::do_these_things({(CopyIf(rhs), 1)...}); } // For each field (element I=0, 1, ...) move that field into this table - // or_clear as per MoveIf(). template void Move(absl::index_sequence, Table&& rhs) { - table_detail::do_these_things( + table_detail::do_these_things( {(MoveIf(std::forward(rhs)), 1)...}); } // For each field (element I=0, 1, ...) if that field is present, call f. template void ForEachImpl(F f, absl::index_sequence) const { - table_detail::do_these_things({(CallIf(&f), 1)...}); + table_detail::do_these_things({(CallIf(&f), 1)...}); } // Bit field indicating which elements are set. diff --git a/src/core/lib/surface/call.cc b/src/core/lib/surface/call.cc index 268ec985cf5..df09c1931a7 100644 --- a/src/core/lib/surface/call.cc +++ b/src/core/lib/surface/call.cc @@ -1470,9 +1470,10 @@ static void receiving_initial_metadata_ready(void* bctlp, GPR_TIMER_SCOPE("validate_filtered_metadata", 0); validate_filtered_metadata(bctl); - grpc_millis deadline = md->deadline(); - if (deadline != GRPC_MILLIS_INF_FUTURE && !call->is_client) { - call->send_deadline = deadline; + absl::optional deadline = + md->get(grpc_core::GrpcTimeoutMetadata()); + if (deadline.has_value() && !call->is_client) { + call->send_deadline = *deadline; } } else { if (bctl->batch_error.ok()) { @@ -1653,8 +1654,9 @@ static grpc_call_error call_start_batch(grpc_call* call, const grpc_op* ops, goto done_with_error; } /* TODO(ctiller): just make these the same variable? */ - if (call->is_client) { - call->metadata_batch[0][0].SetDeadline(call->send_deadline); + if (call->is_client && call->send_deadline != GRPC_MILLIS_INF_FUTURE) { + call->metadata_batch[0][0].Set(grpc_core::GrpcTimeoutMetadata(), + call->send_deadline); } stream_op_payload->send_initial_metadata.send_initial_metadata = &call->metadata_batch[0 /* is_receiving */][0 /* is_trailing */]; diff --git a/src/core/lib/surface/server.cc b/src/core/lib/surface/server.cc index e07772ec283..afe7162f842 100644 --- a/src/core/lib/surface/server.cc +++ b/src/core/lib/surface/server.cc @@ -1379,7 +1379,6 @@ void Server::CallData::RecvInitialMetadataReady(void* arg, grpc_error_handle error) { grpc_call_element* elem = static_cast(arg); CallData* calld = static_cast(elem->call_data); - grpc_millis op_deadline; if (error == GRPC_ERROR_NONE) { GPR_DEBUG_ASSERT( calld->recv_initial_metadata_->legacy_index()->named.path != nullptr); @@ -1395,9 +1394,9 @@ void Server::CallData::RecvInitialMetadataReady(void* arg, } else { GRPC_ERROR_REF(error); } - op_deadline = calld->recv_initial_metadata_->deadline(); - if (op_deadline != GRPC_MILLIS_INF_FUTURE) { - calld->deadline_ = op_deadline; + auto op_deadline = calld->recv_initial_metadata_->get(GrpcTimeoutMetadata()); + if (op_deadline.has_value()) { + calld->deadline_ = *op_deadline; } if (calld->host_.has_value() && calld->path_.has_value()) { /* do nothing */ diff --git a/src/core/lib/transport/metadata_batch.cc b/src/core/lib/transport/metadata_batch.cc index a4dfb2b4401..a0e00d2f7ac 100644 --- a/src/core/lib/transport/metadata_batch.cc +++ b/src/core/lib/transport/metadata_batch.cc @@ -45,7 +45,9 @@ void grpc_metadata_batch_copy(grpc_metadata_batch* src, grpc_metadata_batch* dst, grpc_linked_mdelem* storage) { dst->Clear(); - dst->SetDeadline(src->deadline()); + if (auto* p = src->get_pointer(grpc_core::GrpcTimeoutMetadata())) { + dst->Set(grpc_core::GrpcTimeoutMetadata(), *p); + } size_t i = 0; src->ForEach([&](grpc_mdelem md) { // If the mdelem is not external, take a ref. diff --git a/src/core/lib/transport/metadata_batch.h b/src/core/lib/transport/metadata_batch.h index 525e6f67960..d7bd53435a7 100644 --- a/src/core/lib/transport/metadata_batch.h +++ b/src/core/lib/transport/metadata_batch.h @@ -30,6 +30,7 @@ #include #include +#include "src/core/lib/gprpp/table.h" #include "src/core/lib/iomgr/exec_ctx.h" #include "src/core/lib/transport/metadata.h" #include "src/core/lib/transport/static_metadata.h" @@ -66,6 +67,18 @@ grpc_error_handle grpc_attach_md_to_error(grpc_error_handle src, namespace grpc_core { +// grpc-timeout metadata trait. +// ValueType is defined as grpc_millis - an absolute timestamp (i.e. a +// deadline!), that is converted to a duration by transports before being +// sent. +// TODO(ctiller): Move this elsewhere. During the transition we need to be able +// to name this in MetadataMap, but ultimately once the transition is done we +// should not need to. +struct GrpcTimeoutMetadata { + using ValueType = grpc_millis; + static const char* key() { return "grpc-timeout"; } +}; + // MetadataMap encodes the mapping of metadata keys to metadata values. // Right now the API presented is the minimal one that will allow us to // substitute this type for grpc_metadata_batch in a relatively easy fashion. At @@ -73,9 +86,20 @@ namespace grpc_core { // again, whilst minimally holding the performance bar already set (and // hopefully improving some things). // In the meantime, we're not going to invest much time in ephemeral API -// documentation, so if you must use one of these API's and it's not obvious +// documentation, so if you must use one of these APIs and it's not obvious // how, reach out to ctiller. -template +// +// MetadataMap takes a list of traits. Each of these trait objects defines +// one metadata field that is used by core, and so should have more specialized +// handling than just using the generic APIs. +// +// Each trait object has the following signature: +// // Traits for the grpc-xyz metadata field: +// struct GrpcXyzMetadata { +// using ValueType = ...; +// static constexpr char* key() { return "grpc-xyz"; } +// }; +template class MetadataMap { public: MetadataMap(); @@ -86,14 +110,72 @@ class MetadataMap { MetadataMap(MetadataMap&&) noexcept; MetadataMap& operator=(MetadataMap&&) noexcept; + // Encode this metadata map into some encoder. + // For each field that is set in the MetadataMap, call + // encoder->Encode. + // + // For fields for which we have traits, this will be a method with + // the signature: + // void Encode(TraitsType, typename TraitsType::ValueType value); + // For fields for which we do not have traits, this will be a method + // with the signature: + // void Encode(grpc_mdelem md); + // TODO(ctiller): It's expected that the latter Encode method will + // become Encode(Slice, Slice) by the end of the current metadata API + // transitions. template void Encode(Encoder* encoder) const { for (auto* l = list_.head; l; l = l->next) { encoder->Encode(l->md); } - if (deadline_ != GRPC_MILLIS_INF_FUTURE) encoder->EncodeDeadline(deadline_); + table_.ForEach(EncodeWrapper{encoder}); + } + + // Get the pointer to the value of some known metadata. + // Returns nullptr if the metadata is not present. + // Causes a compilation error if Which is not an element of Traits. + template + const typename Which::ValueType* get_pointer(Which) const { + if (auto* p = table_.template get>()) return &p->value; + return nullptr; + } + + // Get the pointer to the value of some known metadata. + // Returns nullptr if the metadata is not present. + // Causes a compilation error if Which is not an element of Traits. + template + typename Which::ValueType* get_pointer(Which) { + if (auto* p = table_.template get>()) return &p->value; + return nullptr; + } + + // Get the value of some known metadata. + // Returns nullopt if the metadata is not present. + // Causes a compilation error if Which is not an element of Traits. + template + absl::optional get(Which) const { + if (auto* p = table_.template get>()) return p->value; + return absl::nullopt; } + // Set the value of some known metadata. + // Returns a pointer to the new value. + template + typename Which::ValueType* Set(Which, Args&&... args) { + return &table_.template set>(std::forward(args)...) + ->value; + } + + // Remove a specific piece of known metadata. + template + void Remove(Which) { + table_.template clear>(); + } + + // + // All APIs below this point are subject to change. + // + template void ForEach(F f) const { for (auto* l = list_.head; l; l = l->next) { @@ -132,9 +214,7 @@ class MetadataMap { void Clear(); bool empty() const { return count() == 0; } - size_t count() const { - return list_.count + (deadline_ == GRPC_MILLIS_INF_FUTURE ? 0 : 1); - } + size_t count() const { return list_.count + table_.count(); } size_t non_deadline_count() const { return list_.count; } size_t default_count() const { return list_.default_count; } @@ -175,13 +255,37 @@ class MetadataMap { void AssertOk() {} #endif - grpc_millis deadline() const { return deadline_; } - void SetDeadline(grpc_millis deadline) { deadline_ = deadline; } - void ClearDeadline() { SetDeadline(GRPC_MILLIS_INF_FUTURE); } + // TODO(ctiller): the following explicit deadline handling methods are + // deprecated in terms of the traits based APIs. + grpc_millis deadline() const { + return get(GrpcTimeoutMetadata()).value_or(GRPC_MILLIS_INF_FUTURE); + }; const grpc_metadata_batch_callouts* legacy_index() const { return &idx_; } private: + // Generate a strong type for metadata values per trait. + template + struct Value { + Value() = default; + explicit Value(const typename Which::ValueType& value) : value(value) {} + Value(const Value&) = default; + Value& operator=(const Value&) = default; + Value(Value&&) noexcept = default; + Value& operator=(Value&&) noexcept = default; + GPR_NO_UNIQUE_ADDRESS typename Which::ValueType value; + }; + // Callable for the ForEach in Encode() -- for each value, call the + // appropriate encoder method. + template + struct EncodeWrapper { + Encoder* encoder; + template + void operator()(const Value& which) { + encoder->Encode(Which(), which.value); + } + }; + void AssertValidCallouts(); grpc_error_handle LinkCallout(grpc_linked_mdelem* storage, grpc_metadata_batch_callouts_index idx) @@ -272,13 +376,11 @@ class MetadataMap { assert_valid_list(list); } + // Table of known metadata types. + Table...> table_; /** Metadata elements in this batch */ grpc_mdelem_list list_; grpc_metadata_batch_callouts idx_; - /** Used to calculate grpc-timeout at the point of sending, - or GRPC_MILLIS_INF_FUTURE if this batch does not need to send a - grpc-timeout */ - grpc_millis deadline_; }; template @@ -307,29 +409,26 @@ template MetadataMap::MetadataMap() { memset(&list_, 0, sizeof(list_)); memset(&idx_, 0, sizeof(idx_)); - deadline_ = GRPC_MILLIS_INF_FUTURE; } template -MetadataMap::MetadataMap(MetadataMap&& other) noexcept { +MetadataMap::MetadataMap(MetadataMap&& other) noexcept + : table_(std::move(other.table_)) { list_ = other.list_; idx_ = other.idx_; - deadline_ = other.deadline_; memset(&other.list_, 0, sizeof(list_)); memset(&other.idx_, 0, sizeof(idx_)); - other.deadline_ = GRPC_MILLIS_INF_FUTURE; } template MetadataMap& MetadataMap::operator=( MetadataMap&& other) noexcept { Clear(); + table_ = std::move(other.table_); list_ = other.list_; idx_ = other.idx_; - deadline_ = other.deadline_; memset(&other.list_, 0, sizeof(list_)); memset(&other.idx_, 0, sizeof(idx_)); - other.deadline_ = GRPC_MILLIS_INF_FUTURE; return *this; } @@ -567,7 +666,8 @@ bool MetadataMap::ReplaceIfExists(grpc_slice key, grpc_slice value) { } // namespace grpc_core -using grpc_metadata_batch = grpc_core::MetadataMap<>; +using grpc_metadata_batch = + grpc_core::MetadataMap; inline void grpc_metadata_batch_clear(grpc_metadata_batch* batch) { batch->Clear(); diff --git a/src/core/lib/transport/transport_op_string.cc b/src/core/lib/transport/transport_op_string.cc index c383337a12a..253563b4495 100644 --- a/src/core/lib/transport/transport_op_string.cc +++ b/src/core/lib/transport/transport_op_string.cc @@ -40,28 +40,38 @@ /* These routines are here to facilitate debugging - they produce string representations of various transport data structures */ -static void put_metadata(grpc_mdelem md, std::vector* out) { - out->push_back("key="); - char* dump = grpc_dump_slice(GRPC_MDKEY(md), GPR_DUMP_HEX | GPR_DUMP_ASCII); - out->push_back(dump); - gpr_free(dump); - out->push_back(" value="); - dump = grpc_dump_slice(GRPC_MDVALUE(md), GPR_DUMP_HEX | GPR_DUMP_ASCII); - out->push_back(dump); - gpr_free(dump); -} - static void put_metadata_list(const grpc_metadata_batch& md, std::vector* out) { - bool first = true; - md.ForEach([&](grpc_mdelem elem) { - if (!first) out->push_back(", "); - first = false; - put_metadata(elem, out); - }); - if (md.deadline() != GRPC_MILLIS_INF_FUTURE) { - out->push_back(absl::StrFormat(" deadline=%" PRId64, md.deadline())); - } + class Encoder { + public: + explicit Encoder(std::vector* out) : out_(out) {} + void Encode(const grpc_mdelem& md) { + MaybeAddComma(); + out_->push_back("key="); + char* dump = + grpc_dump_slice(GRPC_MDKEY(md), GPR_DUMP_HEX | GPR_DUMP_ASCII); + out_->push_back(dump); + gpr_free(dump); + out_->push_back(" value="); + dump = grpc_dump_slice(GRPC_MDVALUE(md), GPR_DUMP_HEX | GPR_DUMP_ASCII); + out_->push_back(dump); + gpr_free(dump); + } + + void Encode(grpc_core::GrpcTimeoutMetadata, grpc_millis deadline) { + MaybeAddComma(); + out_->push_back(absl::StrFormat("deadline=%" PRId64, deadline)); + } + + private: + void MaybeAddComma() { + if (out_->size() != initial_size_) out_->push_back(", "); + } + std::vector* const out_; + const size_t initial_size_ = out_->size(); + }; + Encoder encoder(out); + md.Encode(&encoder); } std::string grpc_transport_stream_op_batch_string( diff --git a/test/core/gprpp/bitset_test.cc b/test/core/gprpp/bitset_test.cc index 3362a3669d0..dd6204ecef1 100644 --- a/test/core/gprpp/bitset_test.cc +++ b/test/core/gprpp/bitset_test.cc @@ -90,6 +90,13 @@ TYPED_TEST(BitSetTest, Count) { } } +TEST(EmptyBitSet, Empty) { + BitSet<0> b; + EXPECT_TRUE(b.all()); + EXPECT_TRUE(b.none()); + EXPECT_EQ(b.count(), 0); +} + } // namespace testing } // namespace grpc_core diff --git a/test/core/gprpp/table_test.cc b/test/core/gprpp/table_test.cc index 4c76ecb63ea..a355a9cc8fa 100644 --- a/test/core/gprpp/table_test.cc +++ b/test/core/gprpp/table_test.cc @@ -24,6 +24,8 @@ namespace grpc_core { namespace testing { +TEST(Table, InstantiateEmpty) { Table<>(); } + TEST(Table, NoOp) { Table t; EXPECT_EQ(t.get(), nullptr); diff --git a/test/core/transport/BUILD b/test/core/transport/BUILD index 3eeb4f9f73b..4eceba322e9 100644 --- a/test/core/transport/BUILD +++ b/test/core/transport/BUILD @@ -77,6 +77,20 @@ grpc_cc_test( ], ) +grpc_cc_test( + name = "metadata_map_test", + srcs = ["metadata_map_test.cc"], + external_deps = [ + "gtest", + ], + language = "C++", + deps = [ + "//:gpr", + "//:grpc", + "//test/core/util:grpc_test_util", + ], +) + grpc_cc_test( name = "metadata_test", srcs = ["metadata_test.cc"], diff --git a/test/core/transport/metadata_map_test.cc b/test/core/transport/metadata_map_test.cc new file mode 100644 index 00000000000..0daed470c81 --- /dev/null +++ b/test/core/transport/metadata_map_test.cc @@ -0,0 +1,86 @@ +// +// Copyright 2021 gRPC authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// + +#include + +#include "src/core/lib/slice/slice_internal.h" +#include "src/core/lib/transport/metadata_batch.h" +#include "test/core/util/test_config.h" + +namespace grpc_core { +namespace testing { + +TEST(MetadataMapTest, Noop) { MetadataMap<>(); } + +TEST(MetadataMapTest, NoopWithDeadline) { MetadataMap(); } + +TEST(MetadataMapTest, SimpleOps) { + MetadataMap map; + EXPECT_EQ(map.get_pointer(GrpcTimeoutMetadata()), nullptr); + EXPECT_EQ(map.get(GrpcTimeoutMetadata()), absl::nullopt); + map.Set(GrpcTimeoutMetadata(), 1234); + EXPECT_NE(map.get_pointer(GrpcTimeoutMetadata()), nullptr); + EXPECT_EQ(*map.get_pointer(GrpcTimeoutMetadata()), 1234); + EXPECT_EQ(map.get(GrpcTimeoutMetadata()), 1234); + map.Remove(GrpcTimeoutMetadata()); + EXPECT_EQ(map.get_pointer(GrpcTimeoutMetadata()), nullptr); + EXPECT_EQ(map.get(GrpcTimeoutMetadata()), absl::nullopt); +} + +// Target for MetadataMap::Encode. +// Writes down some string representation of what it receives, so we can +// EXPECT_EQ it later. +class FakeEncoder { + public: + std::string output() { return output_; } + + void Encode(grpc_mdelem md) { + output_ += + absl::StrCat("LEGACY CALL: key=", StringViewFromSlice(GRPC_MDKEY(md)), + " value=", StringViewFromSlice(GRPC_MDVALUE(md)), "\n"); + } + + void Encode(GrpcTimeoutMetadata, grpc_millis deadline) { + output_ += absl::StrCat("grpc-timeout: deadline=", deadline, "\n"); + } + + private: + std::string output_; +}; + +TEST(MetadataMapTest, EmptyEncodeTest) { + FakeEncoder encoder; + MetadataMap map; + map.Encode(&encoder); + EXPECT_EQ(encoder.output(), ""); +} + +TEST(MetadataMapTest, TimeoutEncodeTest) { + FakeEncoder encoder; + MetadataMap map; + map.Set(GrpcTimeoutMetadata(), 1234); + map.Encode(&encoder); + EXPECT_EQ(encoder.output(), "grpc-timeout: deadline=1234\n"); +} + +} // namespace testing +} // namespace grpc_core + +int main(int argc, char** argv) { + testing::InitGoogleTest(&argc, argv); + grpc::testing::TestEnvironment env(argc, argv); + return RUN_ALL_TESTS(); +}; diff --git a/test/cpp/microbenchmarks/bm_chttp2_hpack.cc b/test/cpp/microbenchmarks/bm_chttp2_hpack.cc index 0951e1d283b..0a101ce35d5 100644 --- a/test/cpp/microbenchmarks/bm_chttp2_hpack.cc +++ b/test/cpp/microbenchmarks/bm_chttp2_hpack.cc @@ -70,7 +70,7 @@ static void BM_HpackEncoderEncodeDeadline(benchmark::State& state) { grpc_millis saved_now = grpc_core::ExecCtx::Get()->Now(); grpc_metadata_batch b; - b.SetDeadline(saved_now + 30 * 1000); + b.Set(grpc_core::GrpcTimeoutMetadata(), saved_now + 30 * 1000); grpc_core::HPackCompressor c; grpc_transport_one_way_stats stats; diff --git a/tools/doxygen/Doxyfile.c++.internal b/tools/doxygen/Doxyfile.c++.internal index 54bd9b6d4b8..a54b9adf997 100644 --- a/tools/doxygen/Doxyfile.c++.internal +++ b/tools/doxygen/Doxyfile.c++.internal @@ -1796,6 +1796,7 @@ src/core/lib/gprpp/stat_windows.cc \ src/core/lib/gprpp/status_helper.cc \ src/core/lib/gprpp/status_helper.h \ src/core/lib/gprpp/sync.h \ +src/core/lib/gprpp/table.h \ src/core/lib/gprpp/thd.h \ src/core/lib/gprpp/thd_posix.cc \ src/core/lib/gprpp/thd_windows.cc \ diff --git a/tools/doxygen/Doxyfile.core.internal b/tools/doxygen/Doxyfile.core.internal index 30de30b6b13..3957adc2ce5 100644 --- a/tools/doxygen/Doxyfile.core.internal +++ b/tools/doxygen/Doxyfile.core.internal @@ -1635,6 +1635,7 @@ src/core/lib/gprpp/stat_windows.cc \ src/core/lib/gprpp/status_helper.cc \ src/core/lib/gprpp/status_helper.h \ src/core/lib/gprpp/sync.h \ +src/core/lib/gprpp/table.h \ src/core/lib/gprpp/thd.h \ src/core/lib/gprpp/thd_posix.cc \ src/core/lib/gprpp/thd_windows.cc \ diff --git a/tools/run_tests/generated/tests.json b/tools/run_tests/generated/tests.json index 2373978317a..5250f6f0192 100644 --- a/tools/run_tests/generated/tests.json +++ b/tools/run_tests/generated/tests.json @@ -5681,6 +5681,30 @@ ], "uses_polling": true }, + { + "args": [], + "benchmark": false, + "ci_platforms": [ + "linux", + "mac", + "posix", + "windows" + ], + "cpu_cost": 1.0, + "exclude_configs": [], + "exclude_iomgrs": [], + "flaky": false, + "gtest": true, + "language": "c++", + "name": "metadata_map_test", + "platforms": [ + "linux", + "mac", + "posix", + "windows" + ], + "uses_polling": true + }, { "args": [], "benchmark": false,