From fe913387da494713a945f7842669bd3bf3c27a03 Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Fri, 4 Mar 2022 14:42:59 -0800 Subject: [PATCH] Remove idempotent/cacheable requests (#28922) * Remove idempotent/cacheable requests * more cleanup * bump core version * fix * fix * fix * review feedback * fixes * fix * remove more * objc * fix * fix * fix * scrub * Modify XdsRbacTests Co-authored-by: Yash Tibrewal --- CMakeLists.txt | 4 - build_autogenerated.yaml | 4 - gRPC-Core.podspec | 2 - grpc.gyp | 4 - include/grpc/impl/codegen/grpc_types.h | 14 +- include/grpcpp/impl/codegen/client_context.h | 18 +- .../filters/http/client/http_client_filter.cc | 213 +----------- .../filters/http/client/http_client_filter.h | 3 - .../filters/http/server/http_server_filter.cc | 118 +------ .../chttp2/transport/hpack_encoder.cc | 8 +- .../cronet/transport/cronet_transport.cc | 7 +- src/core/lib/surface/call.cc | 7 +- src/core/lib/surface/server.cc | 16 +- src/core/lib/surface/server.h | 3 +- src/core/lib/transport/metadata_batch.h | 7 - src/cpp/client/client_context.cc | 2 - src/objective-c/GRPCClient/GRPCCall.m | 4 - src/objective-c/GRPCClient/GRPCCallLegacy.m | 30 +- src/objective-c/GRPCClient/GRPCTypes.h | 7 - .../private/GRPCCore/GRPCCallInternal.m | 6 +- src/objective-c/ProtoRPC/ProtoRPC.m | 6 +- .../InterceptorSample/CacheInterceptor.h | 92 ------ .../InterceptorSample/CacheInterceptor.m | 306 ------------------ .../InterceptorSample/ViewController.m | 4 +- src/objective-c/tests/UnitTests/APIv2Tests.m | 46 --- .../tests/UnitTests/GRPCClientTests.m | 37 --- test/core/end2end/end2end_nosec_tests.cc | 16 - test/core/end2end/end2end_tests.cc | 16 - test/core/end2end/generate_tests.bzl | 2 - test/core/end2end/tests/idempotent_request.cc | 239 -------------- .../end2end/tests/simple_cacheable_request.cc | 274 ---------------- test/core/surface/server_test.cc | 11 - test/cpp/end2end/xds/xds_end2end_test.cc | 108 +------ test/cpp/interop/client.cc | 2 - test/cpp/interop/interop_client.cc | 65 ---- test/cpp/interop/interop_client.h | 1 - 36 files changed, 39 insertions(+), 1663 deletions(-) delete mode 100644 src/objective-c/examples/InterceptorSample/InterceptorSample/CacheInterceptor.h delete mode 100644 src/objective-c/examples/InterceptorSample/InterceptorSample/CacheInterceptor.m delete mode 100644 test/core/end2end/tests/idempotent_request.cc delete mode 100644 test/core/end2end/tests/simple_cacheable_request.cc diff --git a/CMakeLists.txt b/CMakeLists.txt index ad69cad0a35..0b12007cb79 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -1143,7 +1143,6 @@ add_library(end2end_nosec_tests test/core/end2end/tests/graceful_server_shutdown.cc test/core/end2end/tests/high_initial_seqno.cc test/core/end2end/tests/hpack_size.cc - test/core/end2end/tests/idempotent_request.cc test/core/end2end/tests/invoke_large_request.cc test/core/end2end/tests/keepalive_timeout.cc test/core/end2end/tests/large_metadata.cc @@ -1200,7 +1199,6 @@ add_library(end2end_nosec_tests test/core/end2end/tests/server_streaming.cc test/core/end2end/tests/shutdown_finishes_calls.cc test/core/end2end/tests/shutdown_finishes_tags.cc - test/core/end2end/tests/simple_cacheable_request.cc test/core/end2end/tests/simple_delayed_request.cc test/core/end2end/tests/simple_metadata.cc test/core/end2end/tests/simple_request.cc @@ -1292,7 +1290,6 @@ add_library(end2end_tests test/core/end2end/tests/grpc_authz.cc test/core/end2end/tests/high_initial_seqno.cc test/core/end2end/tests/hpack_size.cc - test/core/end2end/tests/idempotent_request.cc test/core/end2end/tests/invoke_large_request.cc test/core/end2end/tests/keepalive_timeout.cc test/core/end2end/tests/large_metadata.cc @@ -1349,7 +1346,6 @@ add_library(end2end_tests test/core/end2end/tests/server_streaming.cc test/core/end2end/tests/shutdown_finishes_calls.cc test/core/end2end/tests/shutdown_finishes_tags.cc - test/core/end2end/tests/simple_cacheable_request.cc test/core/end2end/tests/simple_delayed_request.cc test/core/end2end/tests/simple_metadata.cc test/core/end2end/tests/simple_request.cc diff --git a/build_autogenerated.yaml b/build_autogenerated.yaml index f2ae97e39aa..3486037cef3 100644 --- a/build_autogenerated.yaml +++ b/build_autogenerated.yaml @@ -65,7 +65,6 @@ libs: - test/core/end2end/tests/graceful_server_shutdown.cc - test/core/end2end/tests/high_initial_seqno.cc - test/core/end2end/tests/hpack_size.cc - - test/core/end2end/tests/idempotent_request.cc - test/core/end2end/tests/invoke_large_request.cc - test/core/end2end/tests/keepalive_timeout.cc - test/core/end2end/tests/large_metadata.cc @@ -122,7 +121,6 @@ libs: - test/core/end2end/tests/server_streaming.cc - test/core/end2end/tests/shutdown_finishes_calls.cc - test/core/end2end/tests/shutdown_finishes_tags.cc - - test/core/end2end/tests/simple_cacheable_request.cc - test/core/end2end/tests/simple_delayed_request.cc - test/core/end2end/tests/simple_metadata.cc - test/core/end2end/tests/simple_request.cc @@ -205,7 +203,6 @@ libs: - test/core/end2end/tests/grpc_authz.cc - test/core/end2end/tests/high_initial_seqno.cc - test/core/end2end/tests/hpack_size.cc - - test/core/end2end/tests/idempotent_request.cc - test/core/end2end/tests/invoke_large_request.cc - test/core/end2end/tests/keepalive_timeout.cc - test/core/end2end/tests/large_metadata.cc @@ -262,7 +259,6 @@ libs: - test/core/end2end/tests/server_streaming.cc - test/core/end2end/tests/shutdown_finishes_calls.cc - test/core/end2end/tests/shutdown_finishes_tags.cc - - test/core/end2end/tests/simple_cacheable_request.cc - test/core/end2end/tests/simple_delayed_request.cc - test/core/end2end/tests/simple_metadata.cc - test/core/end2end/tests/simple_request.cc diff --git a/gRPC-Core.podspec b/gRPC-Core.podspec index 238cee95a3d..6b13c95beb8 100644 --- a/gRPC-Core.podspec +++ b/gRPC-Core.podspec @@ -2457,7 +2457,6 @@ Pod::Spec.new do |s| 'test/core/end2end/tests/grpc_authz.cc', 'test/core/end2end/tests/high_initial_seqno.cc', 'test/core/end2end/tests/hpack_size.cc', - 'test/core/end2end/tests/idempotent_request.cc', 'test/core/end2end/tests/invoke_large_request.cc', 'test/core/end2end/tests/keepalive_timeout.cc', 'test/core/end2end/tests/large_metadata.cc', @@ -2514,7 +2513,6 @@ Pod::Spec.new do |s| 'test/core/end2end/tests/server_streaming.cc', 'test/core/end2end/tests/shutdown_finishes_calls.cc', 'test/core/end2end/tests/shutdown_finishes_tags.cc', - 'test/core/end2end/tests/simple_cacheable_request.cc', 'test/core/end2end/tests/simple_delayed_request.cc', 'test/core/end2end/tests/simple_metadata.cc', 'test/core/end2end/tests/simple_request.cc', diff --git a/grpc.gyp b/grpc.gyp index 2c89374f8aa..9a7a36cb898 100644 --- a/grpc.gyp +++ b/grpc.gyp @@ -218,7 +218,6 @@ 'test/core/end2end/tests/graceful_server_shutdown.cc', 'test/core/end2end/tests/high_initial_seqno.cc', 'test/core/end2end/tests/hpack_size.cc', - 'test/core/end2end/tests/idempotent_request.cc', 'test/core/end2end/tests/invoke_large_request.cc', 'test/core/end2end/tests/keepalive_timeout.cc', 'test/core/end2end/tests/large_metadata.cc', @@ -275,7 +274,6 @@ 'test/core/end2end/tests/server_streaming.cc', 'test/core/end2end/tests/shutdown_finishes_calls.cc', 'test/core/end2end/tests/shutdown_finishes_tags.cc', - 'test/core/end2end/tests/simple_cacheable_request.cc', 'test/core/end2end/tests/simple_delayed_request.cc', 'test/core/end2end/tests/simple_metadata.cc', 'test/core/end2end/tests/simple_request.cc', @@ -335,7 +333,6 @@ 'test/core/end2end/tests/grpc_authz.cc', 'test/core/end2end/tests/high_initial_seqno.cc', 'test/core/end2end/tests/hpack_size.cc', - 'test/core/end2end/tests/idempotent_request.cc', 'test/core/end2end/tests/invoke_large_request.cc', 'test/core/end2end/tests/keepalive_timeout.cc', 'test/core/end2end/tests/large_metadata.cc', @@ -392,7 +389,6 @@ 'test/core/end2end/tests/server_streaming.cc', 'test/core/end2end/tests/shutdown_finishes_calls.cc', 'test/core/end2end/tests/shutdown_finishes_tags.cc', - 'test/core/end2end/tests/simple_cacheable_request.cc', 'test/core/end2end/tests/simple_delayed_request.cc', 'test/core/end2end/tests/simple_metadata.cc', 'test/core/end2end/tests/simple_request.cc', diff --git a/include/grpc/impl/codegen/grpc_types.h b/include/grpc/impl/codegen/grpc_types.h index 40f3075c236..3fa90346b4b 100644 --- a/include/grpc/impl/codegen/grpc_types.h +++ b/include/grpc/impl/codegen/grpc_types.h @@ -518,12 +518,8 @@ typedef enum grpc_call_error { /** Initial metadata flags */ /** These flags are to be passed to the `grpc_op::flags` field */ -/** Signal that the call is idempotent */ -#define GRPC_INITIAL_METADATA_IDEMPOTENT_REQUEST (0x00000010u) /** Signal that the call should not return UNAVAILABLE before it has started */ #define GRPC_INITIAL_METADATA_WAIT_FOR_READY (0x00000020u) -/** Signal that the call is cacheable. GRPC is free to use GET verb */ -#define GRPC_INITIAL_METADATA_CACHEABLE_REQUEST (0x00000040u) /** Signal that GRPC_INITIAL_METADATA_WAIT_FOR_READY was explicitly set by the calling application. */ #define GRPC_INITIAL_METADATA_WAIT_FOR_READY_EXPLICITLY_SET (0x00000080u) @@ -531,12 +527,10 @@ typedef enum grpc_call_error { #define GRPC_INITIAL_METADATA_CORKED (0x00000100u) /** Mask of all valid flags */ -#define GRPC_INITIAL_METADATA_USED_MASK \ - (GRPC_INITIAL_METADATA_IDEMPOTENT_REQUEST | \ - GRPC_INITIAL_METADATA_WAIT_FOR_READY | \ - GRPC_INITIAL_METADATA_CACHEABLE_REQUEST | \ - GRPC_INITIAL_METADATA_WAIT_FOR_READY_EXPLICITLY_SET | \ - GRPC_INITIAL_METADATA_CORKED | GRPC_WRITE_THROUGH) +#define GRPC_INITIAL_METADATA_USED_MASK \ + (GRPC_INITIAL_METADATA_WAIT_FOR_READY_EXPLICITLY_SET | \ + GRPC_INITIAL_METADATA_WAIT_FOR_READY | GRPC_INITIAL_METADATA_CORKED | \ + GRPC_WRITE_THROUGH) /** A single metadata element */ typedef struct grpc_metadata { diff --git a/include/grpcpp/impl/codegen/client_context.h b/include/grpcpp/impl/codegen/client_context.h index 41cb9b1535a..9d616532961 100644 --- a/include/grpcpp/impl/codegen/client_context.h +++ b/include/grpcpp/impl/codegen/client_context.h @@ -276,18 +276,6 @@ class ClientContext { deadline_ = deadline_tp.raw_time(); } - /// EXPERIMENTAL: Indicate that this request is idempotent. - /// By default, RPCs are assumed to not be idempotent. - /// - /// If true, the gRPC library assumes that it's safe to initiate - /// this RPC multiple times. - void set_idempotent(bool idempotent) { idempotent_ = idempotent; } - - /// EXPERIMENTAL: Set this request to be cacheable. - /// If set, grpc is free to use the HTTP GET verb for sending the request, - /// with the possibility of receiving a cached response. - void set_cacheable(bool cacheable) { cacheable_ = cacheable; } - /// Trigger wait-for-ready or not on this request. /// See https://github.com/grpc/grpc/blob/master/doc/wait-for-ready.md. /// If set, if an RPC is made when a channel's connectivity state is @@ -484,9 +472,7 @@ class ClientContext { } uint32_t initial_metadata_flags() const { - return (idempotent_ ? GRPC_INITIAL_METADATA_IDEMPOTENT_REQUEST : 0) | - (wait_for_ready_ ? GRPC_INITIAL_METADATA_WAIT_FOR_READY : 0) | - (cacheable_ ? GRPC_INITIAL_METADATA_CACHEABLE_REQUEST : 0) | + return (wait_for_ready_ ? GRPC_INITIAL_METADATA_WAIT_FOR_READY : 0) | (wait_for_ready_explicitly_set_ ? GRPC_INITIAL_METADATA_WAIT_FOR_READY_EXPLICITLY_SET : 0) | @@ -504,8 +490,6 @@ class ClientContext { bool initial_metadata_received_; bool wait_for_ready_; bool wait_for_ready_explicitly_set_; - bool idempotent_; - bool cacheable_; std::shared_ptr channel_; grpc::internal::Mutex mu_; grpc_call* call_; diff --git a/src/core/ext/filters/http/client/http_client_filter.cc b/src/core/ext/filters/http/client/http_client_filter.cc index c38420b012b..8d58c979b7c 100644 --- a/src/core/ext/filters/http/client/http_client_filter.cc +++ b/src/core/ext/filters/http/client/http_client_filter.cc @@ -46,15 +46,10 @@ #define EXPECTED_CONTENT_TYPE "application/grpc" #define EXPECTED_CONTENT_TYPE_LENGTH (sizeof(EXPECTED_CONTENT_TYPE) - 1) -/* default maximum size of payload eligible for GET request */ -static constexpr size_t kMaxPayloadSizeForGet = 2048; - static void recv_initial_metadata_ready(void* user_data, grpc_error_handle error); static void recv_trailing_metadata_ready(void* user_data, grpc_error_handle error); -static void on_send_message_next_done(void* arg, grpc_error_handle error); -static void send_message_on_complete(void* arg, grpc_error_handle error); namespace { struct call_data { @@ -66,10 +61,6 @@ struct call_data { GRPC_CLOSURE_INIT(&recv_trailing_metadata_ready, ::recv_trailing_metadata_ready, elem, grpc_schedule_on_exec_ctx); - GRPC_CLOSURE_INIT(&on_send_message_next_done, ::on_send_message_next_done, - elem, grpc_schedule_on_exec_ctx); - GRPC_CLOSURE_INIT(&send_message_on_complete, ::send_message_on_complete, - elem, grpc_schedule_on_exec_ctx); } ~call_data() { GRPC_ERROR_UNREF(recv_initial_metadata_error); } @@ -86,21 +77,11 @@ struct call_data { grpc_closure recv_trailing_metadata_ready; grpc_error_handle recv_trailing_metadata_error = GRPC_ERROR_NONE; bool seen_recv_trailing_metadata_ready = false; - // State for handling send_message ops. - grpc_transport_stream_op_batch* send_message_batch; - size_t send_message_bytes_read = 0; - grpc_core::ManualConstructor send_message_cache; - grpc_core::ManualConstructor - send_message_caching_stream; - grpc_closure on_send_message_next_done; - grpc_closure* original_send_message_on_complete; - grpc_closure send_message_on_complete; }; struct channel_data { grpc_core::HttpSchemeMetadata::ValueType static_scheme; grpc_core::Slice user_agent; - size_t max_payload_size_for_get; }; } // namespace @@ -185,126 +166,6 @@ static void recv_trailing_metadata_ready(void* user_data, calld->original_recv_trailing_metadata_ready, error); } -static void send_message_on_complete(void* arg, grpc_error_handle error) { - grpc_call_element* elem = static_cast(arg); - call_data* calld = static_cast(elem->call_data); - calld->send_message_cache.Destroy(); - // Set the batch's send_message bit back to true, so the retry code - // above knows what was in this batch. - calld->send_message_batch->send_message = true; - grpc_core::Closure::Run(DEBUG_LOCATION, - calld->original_send_message_on_complete, - GRPC_ERROR_REF(error)); -} - -// Pulls a slice from the send_message byte stream, updating -// calld->send_message_bytes_read. -static grpc_error_handle pull_slice_from_send_message(call_data* calld) { - grpc_slice incoming_slice; - grpc_error_handle error = - calld->send_message_caching_stream->Pull(&incoming_slice); - if (error == GRPC_ERROR_NONE) { - calld->send_message_bytes_read += GRPC_SLICE_LENGTH(incoming_slice); - grpc_slice_unref_internal(incoming_slice); - } - return error; -} - -// Reads as many slices as possible from the send_message byte stream. -// Upon successful return, if calld->send_message_bytes_read == -// calld->send_message_caching_stream->length(), then we have completed -// reading from the byte stream; otherwise, an async read has been dispatched -// and on_send_message_next_done() will be invoked when it is complete. -static grpc_error_handle read_all_available_send_message_data( - call_data* calld) { - while (calld->send_message_caching_stream->Next( - SIZE_MAX, &calld->on_send_message_next_done)) { - grpc_error_handle error = pull_slice_from_send_message(calld); - if (error != GRPC_ERROR_NONE) return error; - if (calld->send_message_bytes_read == - calld->send_message_caching_stream->length()) { - break; - } - } - return GRPC_ERROR_NONE; -} - -// Async callback for ByteStream::Next(). -static void on_send_message_next_done(void* arg, grpc_error_handle error) { - grpc_call_element* elem = static_cast(arg); - call_data* calld = static_cast(elem->call_data); - if (error != GRPC_ERROR_NONE) { - grpc_transport_stream_op_batch_finish_with_failure( - calld->send_message_batch, error, calld->call_combiner); - return; - } - error = pull_slice_from_send_message(calld); - if (error != GRPC_ERROR_NONE) { - grpc_transport_stream_op_batch_finish_with_failure( - calld->send_message_batch, error, calld->call_combiner); - return; - } - // There may or may not be more to read, but we don't care. If we got - // here, then we know that all of the data was not available - // synchronously, so we were not able to do a cached call. Instead, - // we just reset the byte stream and then send down the batch as-is. - calld->send_message_caching_stream->Reset(); - grpc_call_next_op(elem, calld->send_message_batch); -} - -static char* slice_buffer_to_string(grpc_slice_buffer* slice_buffer) { - char* payload_bytes = - static_cast(gpr_malloc(slice_buffer->length + 1)); - size_t offset = 0; - for (size_t i = 0; i < slice_buffer->count; ++i) { - memcpy(payload_bytes + offset, - GRPC_SLICE_START_PTR(slice_buffer->slices[i]), - GRPC_SLICE_LENGTH(slice_buffer->slices[i])); - offset += GRPC_SLICE_LENGTH(slice_buffer->slices[i]); - } - *(payload_bytes + offset) = '\0'; - return payload_bytes; -} - -// Modifies the path entry in the batch's send_initial_metadata to -// append the base64-encoded query for a GET request. -static void update_path_for_get(grpc_call_element* elem, - grpc_transport_stream_op_batch* batch) { - grpc_metadata_batch* b = - batch->payload->send_initial_metadata.send_initial_metadata; - call_data* calld = static_cast(elem->call_data); - const grpc_core::Slice& path_slice = - *b->get_pointer(grpc_core::HttpPathMetadata()); - /* sum up individual component's lengths and allocate enough memory to - * hold combined path+query */ - size_t estimated_len = path_slice.size(); - estimated_len++; /* for the '?' */ - estimated_len += grpc_base64_estimate_encoded_size( - batch->payload->send_message.send_message->length(), - false /* multi_line */); - grpc_core::MutableSlice path_with_query_slice = - grpc_core::MutableSlice::CreateUninitialized(estimated_len); - /* memcopy individual pieces into this slice */ - uint8_t* write_ptr = path_with_query_slice.begin(); - const uint8_t* original_path = path_slice.data(); - memcpy(write_ptr, original_path, path_slice.size()); - write_ptr += path_slice.size(); - *write_ptr++ = '?'; - char* payload_bytes = - slice_buffer_to_string(calld->send_message_cache->cache_buffer()); - grpc_base64_encode_core(reinterpret_cast(write_ptr), payload_bytes, - batch->payload->send_message.send_message->length(), - true /* url_safe */, false /* multi_line */); - gpr_free(payload_bytes); - char* t = reinterpret_cast(path_with_query_slice.begin()) + - path_slice.size(); - /* safe to use strlen since base64_encode will always add '\0' */ - /* substitute previous path with the new path+query */ - b->Set(grpc_core::HttpPathMetadata(), - grpc_core::Slice(path_with_query_slice.TakeSubSlice( - 0, path_slice.size() + strlen(t)))); -} - static void http_client_start_transport_stream_op_batch( grpc_call_element* elem, grpc_transport_stream_op_batch* batch) { call_data* calld = static_cast(elem->call_data); @@ -331,57 +192,11 @@ static void http_client_start_transport_stream_op_batch( &calld->recv_trailing_metadata_ready; } - grpc_error_handle error = GRPC_ERROR_NONE; - bool batch_will_be_handled_asynchronously = false; if (batch->send_initial_metadata) { - // Decide which HTTP VERB to use. We use GET if the request is marked - // cacheable, and the operation contains both initial metadata and send - // message, and the payload is below the size threshold, and all the data - // for this request is immediately available. - grpc_core::HttpMethodMetadata::ValueType method = - grpc_core::HttpMethodMetadata::kPost; - if (batch->send_message && - (batch->payload->send_initial_metadata.send_initial_metadata_flags & - GRPC_INITIAL_METADATA_CACHEABLE_REQUEST) && - batch->payload->send_message.send_message->length() < - channeld->max_payload_size_for_get) { - calld->send_message_bytes_read = 0; - calld->send_message_cache.Init( - std::move(batch->payload->send_message.send_message)); - calld->send_message_caching_stream.Init(calld->send_message_cache.get()); - batch->payload->send_message.send_message.reset( - calld->send_message_caching_stream.get()); - calld->original_send_message_on_complete = batch->on_complete; - batch->on_complete = &calld->send_message_on_complete; - calld->send_message_batch = batch; - error = read_all_available_send_message_data(calld); - if (error != GRPC_ERROR_NONE) goto done; - // If all the data has been read, then we can use GET. - if (calld->send_message_bytes_read == - calld->send_message_caching_stream->length()) { - method = grpc_core::HttpMethodMetadata::kGet; - update_path_for_get(elem, batch); - batch->send_message = false; - calld->send_message_caching_stream->Orphan(); - } else { - // Not all data is available. The batch will be sent down - // asynchronously in on_send_message_next_done(). - batch_will_be_handled_asynchronously = true; - // Fall back to POST. - gpr_log(GPR_DEBUG, - "Request is marked Cacheable but not all data is available. " - "Falling back to POST"); - } - } else if (batch->payload->send_initial_metadata - .send_initial_metadata_flags & - GRPC_INITIAL_METADATA_IDEMPOTENT_REQUEST) { - method = grpc_core::HttpMethodMetadata::kPut; - } - /* Send : prefixed headers, which have to be before any application layer headers. */ batch->payload->send_initial_metadata.send_initial_metadata->Set( - grpc_core::HttpMethodMetadata(), method); + grpc_core::HttpMethodMetadata(), grpc_core::HttpMethodMetadata::kPost); batch->payload->send_initial_metadata.send_initial_metadata->Set( grpc_core::HttpSchemeMetadata(), channeld->static_scheme); batch->payload->send_initial_metadata.send_initial_metadata->Set( @@ -393,13 +208,7 @@ static void http_client_start_transport_stream_op_batch( grpc_core::UserAgentMetadata(), channeld->user_agent.Ref()); } -done: - if (error != GRPC_ERROR_NONE) { - grpc_transport_stream_op_batch_finish_with_failure(batch, error, - calld->call_combiner); - } else if (!batch_will_be_handled_asynchronously) { - grpc_call_next_op(elem, batch); - } + grpc_call_next_op(elem, batch); } /* Constructor for call_data */ @@ -434,22 +243,6 @@ static grpc_core::HttpSchemeMetadata::ValueType scheme_from_args( return grpc_core::HttpSchemeMetadata::kHttp; } -static size_t max_payload_size_from_args(const grpc_channel_args* args) { - if (args != nullptr) { - for (size_t i = 0; i < args->num_args; ++i) { - if (0 == strcmp(args->args[i].key, GRPC_ARG_MAX_PAYLOAD_SIZE_FOR_GET)) { - if (args->args[i].type != GRPC_ARG_INTEGER) { - gpr_log(GPR_ERROR, "%s: must be an integer", - GRPC_ARG_MAX_PAYLOAD_SIZE_FOR_GET); - } else { - return static_cast(args->args[i].value.integer); - } - } - } - } - return kMaxPayloadSizeForGet; -} - static grpc_core::Slice user_agent_from_args(const grpc_channel_args* args, const char* transport_name) { std::vector user_agent_fields; @@ -494,8 +287,6 @@ static grpc_error_handle http_client_init_channel_elem( args->channel_args, GRPC_ARG_TRANSPORT); GPR_ASSERT(transport != nullptr); chand->static_scheme = scheme_from_args(args->channel_args); - chand->max_payload_size_for_get = - max_payload_size_from_args(args->channel_args); chand->user_agent = grpc_core::Slice( user_agent_from_args(args->channel_args, transport->vtable->name)); return GRPC_ERROR_NONE; diff --git a/src/core/ext/filters/http/client/http_client_filter.h b/src/core/ext/filters/http/client/http_client_filter.h index a2f16ddbfbe..ce577b44178 100644 --- a/src/core/ext/filters/http/client/http_client_filter.h +++ b/src/core/ext/filters/http/client/http_client_filter.h @@ -25,7 +25,4 @@ /* Processes metadata on the client side for HTTP2 transports */ extern const grpc_channel_filter grpc_http_client_filter; -/* Channel arg to determine maximum size of payload eligible for GET request */ -#define GRPC_ARG_MAX_PAYLOAD_SIZE_FOR_GET "grpc.max_payload_size_for_get" - #endif /* GRPC_CORE_EXT_FILTERS_HTTP_CLIENT_HTTP_CLIENT_FILTER_H */ diff --git a/src/core/ext/filters/http/server/http_server_filter.cc b/src/core/ext/filters/http/server/http_server_filter.cc index f39547bfec4..5df249bf604 100644 --- a/src/core/ext/filters/http/server/http_server_filter.cc +++ b/src/core/ext/filters/http/server/http_server_filter.cc @@ -37,7 +37,6 @@ static void hs_recv_initial_metadata_ready(void* user_data, grpc_error_handle err); static void hs_recv_trailing_metadata_ready(void* user_data, grpc_error_handle err); -static void hs_recv_message_ready(void* user_data, grpc_error_handle err); namespace { @@ -47,27 +46,15 @@ struct call_data { GRPC_CLOSURE_INIT(&recv_initial_metadata_ready, hs_recv_initial_metadata_ready, elem, grpc_schedule_on_exec_ctx); - GRPC_CLOSURE_INIT(&recv_message_ready, hs_recv_message_ready, elem, - grpc_schedule_on_exec_ctx); GRPC_CLOSURE_INIT(&recv_trailing_metadata_ready, hs_recv_trailing_metadata_ready, elem, grpc_schedule_on_exec_ctx); } - ~call_data() { - GRPC_ERROR_UNREF(recv_initial_metadata_ready_error); - if (have_read_stream) { - read_stream->Orphan(); - } - } + ~call_data() { GRPC_ERROR_UNREF(recv_initial_metadata_ready_error); } grpc_core::CallCombiner* call_combiner; - // If we see the recv_message contents in the GET query string, we - // store it here. - grpc_core::ManualConstructor read_stream; - bool have_read_stream = false; - // State for intercepting recv_initial_metadata. grpc_closure recv_initial_metadata_ready; grpc_error_handle recv_initial_metadata_ready_error = GRPC_ERROR_NONE; @@ -76,12 +63,6 @@ struct call_data { uint32_t* recv_initial_metadata_flags; bool seen_recv_initial_metadata_ready = false; - // State for intercepting recv_message. - grpc_closure* original_recv_message_ready; - grpc_closure recv_message_ready; - grpc_core::OrphanablePtr* recv_message; - bool seen_recv_message_ready = false; - // State for intercepting recv_trailing_metadata grpc_closure recv_trailing_metadata_ready; grpc_closure* original_recv_trailing_metadata_ready; @@ -115,7 +96,6 @@ static void hs_add_error(const char* error_name, grpc_error_handle* cumulative, static grpc_error_handle hs_filter_incoming_metadata(grpc_call_element* elem, grpc_metadata_batch* b) { - call_data* calld = static_cast(elem->call_data); grpc_error_handle error = GRPC_ERROR_NONE; static const char* error_name = "Failed processing incoming headers"; @@ -123,23 +103,9 @@ static grpc_error_handle hs_filter_incoming_metadata(grpc_call_element* elem, if (method.has_value()) { switch (*method) { case grpc_core::HttpMethodMetadata::kPost: - *calld->recv_initial_metadata_flags &= - ~(GRPC_INITIAL_METADATA_CACHEABLE_REQUEST | - GRPC_INITIAL_METADATA_IDEMPOTENT_REQUEST); - break; - case grpc_core::HttpMethodMetadata::kPut: - *calld->recv_initial_metadata_flags &= - ~GRPC_INITIAL_METADATA_CACHEABLE_REQUEST; - *calld->recv_initial_metadata_flags |= - GRPC_INITIAL_METADATA_IDEMPOTENT_REQUEST; - break; - case grpc_core::HttpMethodMetadata::kGet: - *calld->recv_initial_metadata_flags |= - GRPC_INITIAL_METADATA_CACHEABLE_REQUEST; - *calld->recv_initial_metadata_flags &= - ~GRPC_INITIAL_METADATA_IDEMPOTENT_REQUEST; break; case grpc_core::HttpMethodMetadata::kInvalid: + case grpc_core::HttpMethodMetadata::kGet: hs_add_error(error_name, &error, GRPC_ERROR_CREATE_FROM_STATIC_STRING("Bad method header")); break; @@ -185,38 +151,6 @@ static grpc_error_handle hs_filter_incoming_metadata(grpc_call_element* elem, grpc_error_set_str( GRPC_ERROR_CREATE_FROM_STATIC_STRING("Missing header"), GRPC_ERROR_STR_KEY, ":path")); - } else if (*calld->recv_initial_metadata_flags & - GRPC_INITIAL_METADATA_CACHEABLE_REQUEST) { - /* We have a cacheable request made with GET verb. The path contains the - * query parameter which is base64 encoded request payload. */ - static const char kQuerySeparator = '?'; - /* offset of the character '?' */ - auto it = - std::find(path_slice->begin(), path_slice->end(), kQuerySeparator); - if (it != path_slice->end()) { - const auto query_start = it - path_slice->begin() + 1; - auto query_slice = path_slice->RefSubSlice( - query_start, path_slice->size() - query_start); - - /* substitute path metadata with just the path (not query) */ - auto path_without_query = path_slice->TakeSubSlice(0, query_start - 1); - *path_slice = std::move(path_without_query); - - /* decode payload from query and add to the slice buffer to be returned */ - const int k_url_safe = 1; - grpc_slice_buffer read_slice_buffer; - grpc_slice_buffer_init(&read_slice_buffer); - grpc_slice_buffer_add( - &read_slice_buffer, - grpc_base64_decode_with_len( - reinterpret_cast(query_slice.begin()), - query_slice.size(), k_url_safe)); - calld->read_stream.Init(&read_slice_buffer, 0); - grpc_slice_buffer_destroy_internal(&read_slice_buffer); - calld->have_read_stream = true; - } else { - gpr_log(GPR_ERROR, "GET request without QUERY"); - } } if (b->get_pointer(grpc_core::HttpAuthorityMetadata()) == nullptr) { @@ -249,22 +183,6 @@ static void hs_recv_initial_metadata_ready(void* user_data, if (err == GRPC_ERROR_NONE) { err = hs_filter_incoming_metadata(elem, calld->recv_initial_metadata); calld->recv_initial_metadata_ready_error = GRPC_ERROR_REF(err); - if (calld->seen_recv_message_ready) { - // We've already seen the recv_message callback, but we previously - // deferred it, so we need to return it here. - // Replace the recv_message byte stream if needed. - if (calld->have_read_stream) { - calld->recv_message->reset(calld->read_stream.get()); - calld->have_read_stream = false; - } - // Re-enter call combiner for original_recv_message_ready, since the - // surface code will release the call combiner for each callback it - // receives. - GRPC_CALL_COMBINER_START( - calld->call_combiner, calld->original_recv_message_ready, - GRPC_ERROR_REF(err), - "resuming recv_message_ready from recv_initial_metadata_ready"); - } } else { (void)GRPC_ERROR_REF(err); } @@ -279,31 +197,6 @@ static void hs_recv_initial_metadata_ready(void* user_data, calld->original_recv_initial_metadata_ready, err); } -static void hs_recv_message_ready(void* user_data, grpc_error_handle err) { - grpc_call_element* elem = static_cast(user_data); - call_data* calld = static_cast(elem->call_data); - calld->seen_recv_message_ready = true; - if (calld->seen_recv_initial_metadata_ready) { - // We've already seen the recv_initial_metadata callback, so - // replace the recv_message byte stream if needed and invoke the - // original recv_message callback immediately. - if (calld->have_read_stream) { - calld->recv_message->reset(calld->read_stream.get()); - calld->have_read_stream = false; - } - grpc_core::Closure::Run(DEBUG_LOCATION, calld->original_recv_message_ready, - GRPC_ERROR_REF(err)); - } else { - // We have not yet seen the recv_initial_metadata callback, so we - // need to wait to see if this is a GET request. - // Note that we release the call combiner here, so that other - // callbacks can run. - GRPC_CALL_COMBINER_STOP( - calld->call_combiner, - "pausing recv_message_ready until recv_initial_metadata_ready"); - } -} - static void hs_recv_trailing_metadata_ready(void* user_data, grpc_error_handle err) { grpc_call_element* elem = static_cast(user_data); @@ -355,13 +248,6 @@ static grpc_error_handle hs_mutate_op(grpc_call_element* elem, &calld->recv_initial_metadata_ready; } - if (op->recv_message) { - calld->recv_message = op->payload->recv_message.recv_message; - calld->original_recv_message_ready = - op->payload->recv_message.recv_message_ready; - op->payload->recv_message.recv_message_ready = &calld->recv_message_ready; - } - if (op->recv_trailing_metadata) { calld->original_recv_trailing_metadata_ready = op->payload->recv_trailing_metadata.recv_trailing_metadata_ready; diff --git a/src/core/ext/transport/chttp2/transport/hpack_encoder.cc b/src/core/ext/transport/chttp2/transport/hpack_encoder.cc index 7ad68ab1b1f..d9347bb2029 100644 --- a/src/core/ext/transport/chttp2/transport/hpack_encoder.cc +++ b/src/core/ext/transport/chttp2/transport/hpack_encoder.cc @@ -481,15 +481,11 @@ void HPackCompressor::Framer::Encode(HttpStatusMetadata, uint32_t status) { void HPackCompressor::Framer::Encode(HttpMethodMetadata, HttpMethodMetadata::ValueType method) { switch (method) { - case HttpMethodMetadata::ValueType::kGet: - EmitIndexed(2); // :method: GET - break; case HttpMethodMetadata::ValueType::kPost: EmitIndexed(3); // :method: POST break; - case HttpMethodMetadata::ValueType::kPut: - EmitLitHdrWithNonBinaryStringKeyNotIdx(Slice::FromStaticString(":method"), - Slice::FromStaticString("PUT")); + case HttpMethodMetadata::ValueType::kGet: + EmitIndexed(2); // :method: GET break; case HttpMethodMetadata::ValueType::kInvalid: GPR_ASSERT(false); diff --git a/src/core/ext/transport/cronet/transport/cronet_transport.cc b/src/core/ext/transport/cronet/transport/cronet_transport.cc index 64fe89bf53f..7c6b01960c2 100644 --- a/src/core/ext/transport/cronet/transport/cronet_transport.cc +++ b/src/core/ext/transport/cronet/transport/cronet_transport.cc @@ -739,13 +739,8 @@ class CronetMetadataEncoder { case grpc_core::HttpMethodMetadata::kPost: *method_ = "POST"; break; - case grpc_core::HttpMethodMetadata::kPut: - *method_ = "PUT"; - break; - case grpc_core::HttpMethodMetadata::kGet: - *method_ = "GET"; - break; case grpc_core::HttpMethodMetadata::kInvalid: + case grpc_core::HttpMethodMetadata::kGet: abort(); } } diff --git a/src/core/lib/surface/call.cc b/src/core/lib/surface/call.cc index 8698d400797..18245b7f310 100644 --- a/src/core/lib/surface/call.cc +++ b/src/core/lib/surface/call.cc @@ -948,12 +948,9 @@ static bool are_write_flags_valid(uint32_t flags) { return !(flags & invalid_positions); } -static bool are_initial_metadata_flags_valid(uint32_t flags, bool is_client) { +static bool are_initial_metadata_flags_valid(uint32_t flags) { /* check that only bits in GRPC_WRITE_(INTERNAL?)_USED_MASK are set */ uint32_t invalid_positions = ~GRPC_INITIAL_METADATA_USED_MASK; - if (!is_client) { - invalid_positions |= GRPC_INITIAL_METADATA_IDEMPOTENT_REQUEST; - } return !(flags & invalid_positions); } @@ -1391,7 +1388,7 @@ static grpc_call_error call_start_batch(grpc_call* call, const grpc_op* ops, switch (op->op) { case GRPC_OP_SEND_INITIAL_METADATA: { /* Flag validation: currently allow no flags */ - if (!are_initial_metadata_flags_valid(op->flags, call->is_client)) { + if (!are_initial_metadata_flags_valid(op->flags)) { error = GRPC_CALL_ERROR_INVALID_FLAGS; goto done_with_error; } diff --git a/src/core/lib/surface/server.cc b/src/core/lib/surface/server.cc index baff98fc305..a276d9c6d50 100644 --- a/src/core/lib/surface/server.cc +++ b/src/core/lib/surface/server.cc @@ -685,7 +685,7 @@ Server::RegisteredMethod* Server::RegisterMethod( return nullptr; } } - if ((flags & ~GRPC_INITIAL_METADATA_USED_MASK) != 0) { + if (flags != 0) { gpr_log(GPR_ERROR, "grpc_server_register_method invalid flags 0x%08x", flags); return nullptr; @@ -1062,7 +1062,7 @@ void Server::ChannelData::InitTransport(RefCountedPtr server, } Server::ChannelRegisteredMethod* Server::ChannelData::GetRegisteredMethod( - const grpc_slice& host, const grpc_slice& path, bool is_idempotent) { + const grpc_slice& host, const grpc_slice& path) { if (registered_methods_ == nullptr) return nullptr; /* TODO(ctiller): unify these two searches */ /* check for an exact match with host */ @@ -1075,10 +1075,6 @@ Server::ChannelRegisteredMethod* Server::ChannelData::GetRegisteredMethod( if (!rm->has_host) continue; if (rm->host != host) continue; if (rm->method != path) continue; - if ((rm->flags & GRPC_INITIAL_METADATA_IDEMPOTENT_REQUEST) && - !is_idempotent) { - continue; - } return rm; } /* check for a wildcard method definition (no host set) */ @@ -1089,10 +1085,6 @@ Server::ChannelRegisteredMethod* Server::ChannelData::GetRegisteredMethod( if (rm->server_registered_method == nullptr) break; if (rm->has_host) continue; if (rm->method != path) continue; - if ((rm->flags & GRPC_INITIAL_METADATA_IDEMPOTENT_REQUEST) && - !is_idempotent) { - continue; - } return rm; } return nullptr; @@ -1299,9 +1291,7 @@ void Server::CallData::StartNewRpc(grpc_call_element* elem) { GRPC_SRM_PAYLOAD_NONE; if (path_.has_value() && host_.has_value()) { ChannelRegisteredMethod* rm = - chand->GetRegisteredMethod(host_->c_slice(), path_->c_slice(), - (recv_initial_metadata_flags_ & - GRPC_INITIAL_METADATA_IDEMPOTENT_REQUEST)); + chand->GetRegisteredMethod(host_->c_slice(), path_->c_slice()); if (rm != nullptr) { matcher_ = rm->server_registered_method->matcher.get(); payload_handling = rm->server_registered_method->payload_handling; diff --git a/src/core/lib/surface/server.h b/src/core/lib/surface/server.h index 76dcb288576..0af23190fa7 100644 --- a/src/core/lib/surface/server.h +++ b/src/core/lib/surface/server.h @@ -200,8 +200,7 @@ class Server : public InternallyRefCounted, size_t cq_idx() const { return cq_idx_; } ChannelRegisteredMethod* GetRegisteredMethod(const grpc_slice& host, - const grpc_slice& path, - bool is_idempotent); + const grpc_slice& path); // Filter vtable functions. static grpc_error_handle InitChannelElement( diff --git a/src/core/lib/transport/metadata_batch.h b/src/core/lib/transport/metadata_batch.h index fc031a8dac3..c64deb56970 100644 --- a/src/core/lib/transport/metadata_batch.h +++ b/src/core/lib/transport/metadata_batch.h @@ -224,7 +224,6 @@ struct HttpMethodMetadata { static constexpr bool kRepeatable = false; enum ValueType { kPost, - kPut, kGet, kInvalid, }; @@ -235,8 +234,6 @@ struct HttpMethodMetadata { auto value_string = value.as_string_view(); if (value_string == "POST") { out = kPost; - } else if (value_string == "PUT") { - out = kPut; } else if (value_string == "GET") { out = kGet; } else { @@ -251,8 +248,6 @@ struct HttpMethodMetadata { switch (x) { case kPost: return StaticSlice::FromStaticString("POST"); - case kPut: - return StaticSlice::FromStaticString("PUT"); case kGet: return StaticSlice::FromStaticString("GET"); default: @@ -263,8 +258,6 @@ struct HttpMethodMetadata { switch (content_type) { case kPost: return "POST"; - case kPut: - return "PUT"; case kGet: return "GET"; default: diff --git a/src/cpp/client/client_context.cc b/src/cpp/client/client_context.cc index d7a37f77ba8..72e6653f0e7 100644 --- a/src/cpp/client/client_context.cc +++ b/src/cpp/client/client_context.cc @@ -51,8 +51,6 @@ ClientContext::ClientContext() : initial_metadata_received_(false), wait_for_ready_(false), wait_for_ready_explicitly_set_(false), - idempotent_(false), - cacheable_(false), call_(nullptr), call_canceled_(false), deadline_(gpr_inf_future(GPR_CLOCK_REALTIME)), diff --git a/src/objective-c/GRPCClient/GRPCCall.m b/src/objective-c/GRPCClient/GRPCCall.m index de725953e12..60d8f156020 100644 --- a/src/objective-c/GRPCClient/GRPCCall.m +++ b/src/objective-c/GRPCClient/GRPCCall.m @@ -145,14 +145,10 @@ callOptions:(GRPCCallOptions *)callOptions { NSAssert(requestOptions.host.length != 0 && requestOptions.path.length != 0, @"Neither host nor path can be nil."); - NSAssert(requestOptions.safety <= GRPCCallSafetyCacheableRequest, @"Invalid call safety value."); NSAssert(responseHandler != nil, @"Response handler required."); if (requestOptions.host.length == 0 || requestOptions.path.length == 0) { return nil; } - if (requestOptions.safety > GRPCCallSafetyCacheableRequest) { - return nil; - } if (responseHandler == nil) { return nil; } diff --git a/src/objective-c/GRPCClient/GRPCCallLegacy.m b/src/objective-c/GRPCClient/GRPCCallLegacy.m index c33f3a66dc4..b3cb11a3222 100644 --- a/src/objective-c/GRPCClient/GRPCCallLegacy.m +++ b/src/objective-c/GRPCClient/GRPCCallLegacy.m @@ -147,12 +147,6 @@ static NSString *const kBearerPrefix = @"Bearer "; case GRPCCallSafetyDefault: callFlags[hostAndPath] = @0; break; - case GRPCCallSafetyIdempotentRequest: - callFlags[hostAndPath] = @GRPC_INITIAL_METADATA_IDEMPOTENT_REQUEST; - break; - case GRPCCallSafetyCacheableRequest: - callFlags[hostAndPath] = @GRPC_INITIAL_METADATA_CACHEABLE_REQUEST; - break; default: break; } @@ -185,15 +179,12 @@ static NSString *const kBearerPrefix = @"Bearer "; writeDone:(void (^)(void))writeDone { // Purposely using pointer rather than length (host.length == 0) for backwards compatibility. NSAssert(host != nil && path != nil, @"Neither host nor path can be nil."); - NSAssert(safety <= GRPCCallSafetyCacheableRequest, @"Invalid call safety value."); + NSAssert(safety <= GRPCCallSafetyDefault, @"Invalid call safety value."); NSAssert(requestsWriter.state == GRXWriterStateNotStarted, @"The requests writer can't be already started."); if (!host || !path) { return nil; } - if (safety > GRPCCallSafetyCacheableRequest) { - return nil; - } if (requestsWriter.state != GRXWriterStateNotStarted) { return nil; } @@ -363,17 +354,6 @@ static NSString *const kBearerPrefix = @"Bearer "; - (void)sendHeaders { // TODO (mxyan): Remove after deprecated methods are removed uint32_t callSafetyFlags = 0; - switch (_callSafety) { - case GRPCCallSafetyDefault: - callSafetyFlags = 0; - break; - case GRPCCallSafetyIdempotentRequest: - callSafetyFlags = GRPC_INITIAL_METADATA_IDEMPOTENT_REQUEST; - break; - case GRPCCallSafetyCacheableRequest: - callSafetyFlags = GRPC_INITIAL_METADATA_CACHEABLE_REQUEST; - break; - } NSMutableDictionary *headers = [_requestHeaders mutableCopy]; NSString *fetchedOauth2AccessToken; @@ -603,14 +583,6 @@ static NSString *const kBearerPrefix = @"Bearer "; if (_timeout > 0) { callOptions.timeout = _timeout; } - uint32_t callFlags = [GRPCCall callFlagsForHost:_host path:_path]; - if (callFlags != 0) { - if (callFlags == GRPC_INITIAL_METADATA_IDEMPOTENT_REQUEST) { - _callSafety = GRPCCallSafetyIdempotentRequest; - } else if (callFlags == GRPC_INITIAL_METADATA_CACHEABLE_REQUEST) { - _callSafety = GRPCCallSafetyCacheableRequest; - } - } id tokenProvider = self.tokenProvider; if (tokenProvider != nil) { diff --git a/src/objective-c/GRPCClient/GRPCTypes.h b/src/objective-c/GRPCClient/GRPCTypes.h index ea0fe11803f..2aa9fd7945b 100644 --- a/src/objective-c/GRPCClient/GRPCTypes.h +++ b/src/objective-c/GRPCClient/GRPCTypes.h @@ -131,13 +131,6 @@ typedef NS_ENUM(NSUInteger, GRPCCallSafety) { * state. */ GRPCCallSafetyDefault = 0, - /** Signal that the call is idempotent. gRPC is free to use PUT verb. */ - GRPCCallSafetyIdempotentRequest = 1, - /** - * Signal that the call is cacheable and will not affect server state. gRPC is - * free to use GET verb. - */ - GRPCCallSafetyCacheableRequest = 2, }; /** diff --git a/src/objective-c/GRPCClient/private/GRPCCore/GRPCCallInternal.m b/src/objective-c/GRPCClient/private/GRPCCore/GRPCCallInternal.m index 4c42ba80399..c91e2f0ec33 100644 --- a/src/objective-c/GRPCClient/private/GRPCCore/GRPCCallInternal.m +++ b/src/objective-c/GRPCClient/private/GRPCCore/GRPCCallInternal.m @@ -82,15 +82,11 @@ callOptions:(GRPCCallOptions *)callOptions { NSAssert(requestOptions.host.length != 0 && requestOptions.path.length != 0, @"Neither host nor path can be nil."); - NSAssert(requestOptions.safety <= GRPCCallSafetyCacheableRequest, @"Invalid call safety value."); + NSAssert(requestOptions.safety <= GRPCCallSafetyDefault, @"Invalid call safety value."); if (requestOptions.host.length == 0 || requestOptions.path.length == 0) { NSLog(@"Invalid host and path."); return; } - if (requestOptions.safety > GRPCCallSafetyCacheableRequest) { - NSLog(@"Invalid call safety."); - return; - } GRPCCall *copiedCall = nil; @synchronized(self) { diff --git a/src/objective-c/ProtoRPC/ProtoRPC.m b/src/objective-c/ProtoRPC/ProtoRPC.m index 396f58f8835..fa79fa5b489 100644 --- a/src/objective-c/ProtoRPC/ProtoRPC.m +++ b/src/objective-c/ProtoRPC/ProtoRPC.m @@ -129,12 +129,10 @@ responseHandler:(id)handler callOptions:(GRPCCallOptions *)callOptions responseClass:(Class)responseClass { - NSAssert(requestOptions.host.length != 0 && requestOptions.path.length != 0 && - requestOptions.safety <= GRPCCallSafetyCacheableRequest, + NSAssert(requestOptions.host.length != 0 && requestOptions.path.length != 0, @"Invalid callOptions."); NSAssert(handler != nil, @"handler cannot be empty."); - if (requestOptions.host.length == 0 || requestOptions.path.length == 0 || - requestOptions.safety > GRPCCallSafetyCacheableRequest) { + if (requestOptions.host.length == 0 || requestOptions.path.length == 0) { return nil; } if (handler == nil) { diff --git a/src/objective-c/examples/InterceptorSample/InterceptorSample/CacheInterceptor.h b/src/objective-c/examples/InterceptorSample/InterceptorSample/CacheInterceptor.h deleted file mode 100644 index 861ffd75905..00000000000 --- a/src/objective-c/examples/InterceptorSample/InterceptorSample/CacheInterceptor.h +++ /dev/null @@ -1,92 +0,0 @@ -/* - * - * Copyright 2019 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. - * - */ - -#import - -NS_ASSUME_NONNULL_BEGIN - -@interface RequestCacheEntry : NSObject - -@property(readonly, copy, nullable) NSString *path; -@property(readonly, copy, nullable) id message; - -@end - -@interface MutableRequestCacheEntry : RequestCacheEntry - -@property(copy, nullable) NSString *path; -@property(copy, nullable) id message; - -@end - -@interface ResponseCacheEntry : NSObject - -@property(readonly, copy, nullable) NSDate *deadline; - -@property(readonly, copy, nullable) NSDictionary *headers; -@property(readonly, copy, nullable) id message; -@property(readonly, copy, nullable) NSDictionary *trailers; - -@end - -@interface MutableResponseCacheEntry : ResponseCacheEntry - -@property(copy, nullable) NSDate *deadline; - -@property(copy, nullable) NSDictionary *headers; -@property(copy, nullable) id message; -@property(copy, nullable) NSDictionary *trailers; - -@end - -@interface CacheContext : NSObject - -- (nullable instancetype)init; - -- (nullable ResponseCacheEntry *)getCachedResponseForRequest:(RequestCacheEntry *)request; - -- (void)setCachedResponse:(ResponseCacheEntry *)response forRequest:(RequestCacheEntry *)request; - -@end - -@interface CacheInterceptor : GRPCInterceptor - -- (instancetype)init NS_UNAVAILABLE; - -+ (instancetype)new NS_UNAVAILABLE; - -- (nullable instancetype)initWithInterceptorManager: - (GRPCInterceptorManager *_Nonnull)intercepterManager - cacheContext:(CacheContext *_Nonnull)cacheContext - NS_DESIGNATED_INITIALIZER; - -// implementation of GRPCInterceptorInterface -- (void)startWithRequestOptions:(GRPCRequestOptions *)requestOptions - callOptions:(GRPCCallOptions *)callOptions; -- (void)writeData:(id)data; -- (void)finish; - -// implementation of GRPCResponseHandler -- (void)didReceiveInitialMetadata:(nullable NSDictionary *)initialMetadata; -- (void)didReceiveData:(id)data; -- (void)didCloseWithTrailingMetadata:(nullable NSDictionary *)trailingMetadata - error:(nullable NSError *)error; - -@end - -NS_ASSUME_NONNULL_END diff --git a/src/objective-c/examples/InterceptorSample/InterceptorSample/CacheInterceptor.m b/src/objective-c/examples/InterceptorSample/InterceptorSample/CacheInterceptor.m deleted file mode 100644 index c8e584d37bc..00000000000 --- a/src/objective-c/examples/InterceptorSample/InterceptorSample/CacheInterceptor.m +++ /dev/null @@ -1,306 +0,0 @@ -/* - * - * Copyright 2019 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. - * - */ - -#import "CacheInterceptor.h" - -@implementation RequestCacheEntry { - @protected - NSString *_path; - id _message; -} - -@synthesize path = _path; -@synthesize message = _message; - -- (instancetype)initWithPath:(NSString *)path message:(id)message { - if ((self = [super init])) { - _path = [path copy]; - _message = [message copy]; - } - return self; -} - -- (id)copyWithZone:(NSZone *)zone { - return [[RequestCacheEntry allocWithZone:zone] initWithPath:_path message:_message]; -} - -- (BOOL)isEqual:(id)object { - if ([self class] != [object class]) return NO; - RequestCacheEntry *rhs = (RequestCacheEntry *)object; - return ([_path isEqualToString:rhs.path] && [_message isEqual:rhs.message]); -} - -- (NSUInteger)hash { - return _path.hash ^ _message.hash; -} - -@end - -@implementation MutableRequestCacheEntry - -@dynamic path; -@dynamic message; - -- (void)setPath:(NSString *)path { - _path = [path copy]; -} - -- (void)setMessage:(id)message { - _message = [message copy]; -} - -@end - -@implementation ResponseCacheEntry { - @protected - NSDate *_deadline; - NSDictionary *_headers; - id _message; - NSDictionary *_trailers; -} - -@synthesize deadline = _deadline; -@synthesize headers = _headers; -@synthesize message = _message; -@synthesize trailers = _trailers; - -- (instancetype)initWithDeadline:(NSDate *)deadline - headers:(NSDictionary *)headers - message:(id)message - trailers:(NSDictionary *)trailers { - if (([super init])) { - _deadline = [deadline copy]; - _headers = [[NSDictionary alloc] initWithDictionary:headers copyItems:YES]; - _message = [message copy]; - _trailers = [[NSDictionary alloc] initWithDictionary:trailers copyItems:YES]; - } - return self; -} - -- (id)copyWithZone:(NSZone *)zone { - return [[ResponseCacheEntry allocWithZone:zone] initWithDeadline:_deadline - headers:_headers - message:_message - trailers:_trailers]; -} - -@end - -@implementation MutableResponseCacheEntry - -@dynamic deadline; -@dynamic headers; -@dynamic message; -@dynamic trailers; - -- (void)setDeadline:(NSDate *)deadline { - _deadline = [deadline copy]; -} - -- (void)setHeaders:(NSDictionary *)headers { - _headers = [[NSDictionary alloc] initWithDictionary:headers copyItems:YES]; -} - -- (void)setMessage:(id)message { - _message = [message copy]; -} - -- (void)setTrailers:(NSDictionary *)trailers { - _trailers = [[NSDictionary alloc] initWithDictionary:trailers copyItems:YES]; -} - -@end - -@implementation CacheContext { - NSCache *_cache; -} - -- (instancetype)init { - if ((self = [super init])) { - _cache = [[NSCache alloc] init]; - } - return self; -} - -- (GRPCInterceptor *)createInterceptorWithManager:(GRPCInterceptorManager *)interceptorManager { - return [[CacheInterceptor alloc] initWithInterceptorManager:interceptorManager cacheContext:self]; -} - -- (ResponseCacheEntry *)getCachedResponseForRequest:(RequestCacheEntry *)request { - ResponseCacheEntry *response = nil; - @synchronized(self) { - response = [_cache objectForKey:request]; - if ([response.deadline timeIntervalSinceNow] < 0) { - [_cache removeObjectForKey:request]; - response = nil; - } - } - return response; -} - -- (void)setCachedResponse:(ResponseCacheEntry *)response forRequest:(RequestCacheEntry *)request { - @synchronized(self) { - [_cache setObject:response forKey:request]; - } -} - -@end - -@implementation CacheInterceptor { - GRPCInterceptorManager *_manager; - CacheContext *_context; - dispatch_queue_t _dispatchQueue; - - BOOL _cacheable; - BOOL _writeMessageSeen; - BOOL _readMessageSeen; - GRPCCallOptions *_callOptions; - GRPCRequestOptions *_requestOptions; - id _requestMessage; - MutableRequestCacheEntry *_request; - MutableResponseCacheEntry *_response; -} - -- (dispatch_queue_t)requestDispatchQueue { - return _dispatchQueue; -} - -- (dispatch_queue_t)dispatchQueue { - return _dispatchQueue; -} - -- (instancetype)initWithInterceptorManager:(GRPCInterceptorManager *_Nonnull)intercepterManager - cacheContext:(CacheContext *_Nonnull)cacheContext { - if ((self = [super initWithInterceptorManager:intercepterManager - requestDispatchQueue:dispatch_get_main_queue() - responseDispatchQueue:dispatch_get_main_queue()])) { - _manager = intercepterManager; - _context = cacheContext; - _dispatchQueue = dispatch_queue_create(NULL, DISPATCH_QUEUE_SERIAL); - - _cacheable = YES; - _writeMessageSeen = NO; - _readMessageSeen = NO; - _request = nil; - _response = nil; - } - return self; -} - -- (void)startWithRequestOptions:(GRPCRequestOptions *)requestOptions - callOptions:(GRPCCallOptions *)callOptions { - if (requestOptions.safety != GRPCCallSafetyCacheableRequest) { - _cacheable = NO; - [_manager startNextInterceptorWithRequest:requestOptions callOptions:callOptions]; - } else { - _requestOptions = [requestOptions copy]; - _callOptions = [callOptions copy]; - } -} - -- (void)writeData:(id)data { - if (!_cacheable) { - [_manager writeNextInterceptorWithData:data]; - } else { - NSAssert(!_writeMessageSeen, @"CacheInterceptor does not support streaming call"); - if (_writeMessageSeen) { - NSLog(@"CacheInterceptor does not support streaming call"); - } - _writeMessageSeen = YES; - _requestMessage = [data copy]; - } -} - -- (void)finish { - if (!_cacheable) { - [_manager finishNextInterceptor]; - } else { - _request = [[MutableRequestCacheEntry alloc] init]; - _request.path = _requestOptions.path; - _request.message = [_requestMessage copy]; - _response = [[_context getCachedResponseForRequest:_request] copy]; - if (!_response) { - [_manager startNextInterceptorWithRequest:_requestOptions callOptions:_callOptions]; - [_manager writeNextInterceptorWithData:_requestMessage]; - [_manager finishNextInterceptor]; - } else { - [_manager forwardPreviousInterceptorWithInitialMetadata:_response.headers]; - [_manager forwardPreviousInterceptorWithData:_response.message]; - [_manager forwardPreviousInterceptorCloseWithTrailingMetadata:_response.trailers error:nil]; - [_manager shutDown]; - } - } -} - -- (void)didReceiveInitialMetadata:(NSDictionary *)initialMetadata { - if (_cacheable) { - NSDate *deadline = nil; - for (NSString *key in initialMetadata) { - if ([key.lowercaseString isEqualToString:@"cache-control"]) { - NSArray *cacheControls = [initialMetadata[key] componentsSeparatedByString:@","]; - for (NSString *option in cacheControls) { - NSString *trimmedOption = - [option stringByTrimmingCharactersInSet:[NSCharacterSet - characterSetWithCharactersInString:@" "]]; - if ([trimmedOption.lowercaseString isEqualToString:@"no-cache"] || - [trimmedOption.lowercaseString isEqualToString:@"no-store"] || - [trimmedOption.lowercaseString isEqualToString:@"no-transform"]) { - _cacheable = NO; - break; - } else if ([trimmedOption.lowercaseString hasPrefix:@"max-age="]) { - NSArray *components = [trimmedOption componentsSeparatedByString:@"="]; - if (components.count == 2) { - NSUInteger maxAge = components[1].intValue; - deadline = [NSDate dateWithTimeIntervalSinceNow:maxAge]; - } - } - } - } - } - if (_cacheable) { - _response = [[MutableResponseCacheEntry alloc] init]; - _response.headers = [initialMetadata copy]; - _response.deadline = deadline; - } - } - [_manager forwardPreviousInterceptorWithInitialMetadata:initialMetadata]; -} - -- (void)didReceiveData:(id)data { - if (_cacheable) { - NSAssert(!_readMessageSeen, @"CacheInterceptor does not support streaming call"); - if (_readMessageSeen) { - NSLog(@"CacheInterceptor does not support streaming call"); - } - _readMessageSeen = YES; - _response.message = [data copy]; - } - [_manager forwardPreviousInterceptorWithData:data]; -} - -- (void)didCloseWithTrailingMetadata:(NSDictionary *)trailingMetadata error:(NSError *)error { - if (error == nil && _cacheable) { - _response.trailers = [trailingMetadata copy]; - [_context setCachedResponse:_response forRequest:_request]; - NSLog(@"Write cache for %@", _request); - } - [_manager forwardPreviousInterceptorCloseWithTrailingMetadata:trailingMetadata error:error]; - [_manager shutDown]; -} - -@end diff --git a/src/objective-c/examples/InterceptorSample/InterceptorSample/ViewController.m b/src/objective-c/examples/InterceptorSample/InterceptorSample/ViewController.m index b302a754335..1d6122c7e7a 100644 --- a/src/objective-c/examples/InterceptorSample/InterceptorSample/ViewController.m +++ b/src/objective-c/examples/InterceptorSample/InterceptorSample/ViewController.m @@ -27,8 +27,6 @@ #import "src/objective-c/examples/RemoteTestClient/Test.pbrpc.h" #endif -#import "CacheInterceptor.h" - static NSString *const kPackage = @"grpc.testing"; static NSString *const kService = @"TestService"; @@ -58,7 +56,7 @@ static NSString *const kService = @"TestService"; GRPCRequestOptions *requestOptions = [[GRPCRequestOptions alloc] initWithHost:@"grpc-test.sandbox.googleapis.com" path:kUnaryCallMethod.HTTPPath - safety:GRPCCallSafetyCacheableRequest]; + safety:GRPCCallSafetyDefault]; GRPCCall2 *call = [[GRPCCall2 alloc] initWithRequestOptions:requestOptions responseHandler:self diff --git a/src/objective-c/tests/UnitTests/APIv2Tests.m b/src/objective-c/tests/UnitTests/APIv2Tests.m index 1d769aa3177..1d3be4e1302 100644 --- a/src/objective-c/tests/UnitTests/APIv2Tests.m +++ b/src/objective-c/tests/UnitTests/APIv2Tests.m @@ -345,52 +345,6 @@ static const NSTimeInterval kInvertedTimeout = 2; [self waitForExpectationsWithTimeout:kTestTimeout handler:nil]; } -- (void)testIdempotentProtoRPC { - __weak XCTestExpectation *response = [self expectationWithDescription:@"Expected response."]; - __weak XCTestExpectation *completion = [self expectationWithDescription:@"RPC completed."]; - - RMTSimpleRequest *request = [RMTSimpleRequest message]; - request.responseSize = kSimpleDataLength; - request.fillUsername = YES; - request.fillOauthScope = YES; - GRPCRequestOptions *requestOptions = - [[GRPCRequestOptions alloc] initWithHost:kHostAddress - path:kUnaryCallMethod.HTTPPath - safety:GRPCCallSafetyIdempotentRequest]; - - GRPCMutableCallOptions *options = [[GRPCMutableCallOptions alloc] init]; - options.transportType = GRPCTransportTypeInsecure; - GRPCCall2 *call = [[GRPCCall2 alloc] - initWithRequestOptions:requestOptions - responseHandler:[[ClientTestsBlockCallbacks alloc] initWithInitialMetadataCallback:nil - messageCallback:^(id message) { - NSData *data = (NSData *)message; - XCTAssertNotNil(data, @"nil value received as response."); - XCTAssertGreaterThan(data.length, 0, - @"Empty response received."); - RMTSimpleResponse *responseProto = - [RMTSimpleResponse parseFromData:data error:NULL]; - // We expect empty strings, not nil: - XCTAssertNotNil(responseProto.username, - @"Response's username is nil."); - XCTAssertNotNil(responseProto.oauthScope, - @"Response's OAuth scope is nil."); - [response fulfill]; - } - closeCallback:^(NSDictionary *trailingMetadata, NSError *error) { - XCTAssertNil(error, @"Finished with unexpected error: %@", - error); - [completion fulfill]; - }] - callOptions:options]; - - [call start]; - [call writeData:[request data]]; - [call finish]; - - [self waitForExpectationsWithTimeout:kTestTimeout handler:nil]; -} - - (void)testTimeout { __weak XCTestExpectation *completion = [self expectationWithDescription:@"RPC completed."]; diff --git a/src/objective-c/tests/UnitTests/GRPCClientTests.m b/src/objective-c/tests/UnitTests/GRPCClientTests.m index 012b4b012ae..88aef0f2e5f 100644 --- a/src/objective-c/tests/UnitTests/GRPCClientTests.m +++ b/src/objective-c/tests/UnitTests/GRPCClientTests.m @@ -396,43 +396,6 @@ static GRPCProtoMethod *kFullDuplexCallMethod; } } -- (void)testIdempotentProtoRPC { - __weak XCTestExpectation *response = [self expectationWithDescription:@"Expected response."]; - __weak XCTestExpectation *completion = [self expectationWithDescription:@"RPC completed."]; - - RMTSimpleRequest *request = [RMTSimpleRequest message]; - request.responseSize = 100; - request.fillUsername = YES; - request.fillOauthScope = YES; - GRXWriter *requestsWriter = [GRXWriter writerWithValue:[request data]]; - - GRPCCall *call = [[GRPCCall alloc] initWithHost:kHostAddress - path:kUnaryCallMethod.HTTPPath - requestsWriter:requestsWriter]; - [GRPCCall setCallSafety:GRPCCallSafetyIdempotentRequest - host:kHostAddress - path:kUnaryCallMethod.HTTPPath]; - - id responsesWriteable = [[GRXWriteable alloc] - initWithValueHandler:^(NSData *value) { - XCTAssertNotNil(value, @"nil value received as response."); - XCTAssertGreaterThan(value.length, 0, @"Empty response received."); - RMTSimpleResponse *responseProto = [RMTSimpleResponse parseFromData:value error:NULL]; - // We expect empty strings, not nil: - XCTAssertNotNil(responseProto.username, @"Response's username is nil."); - XCTAssertNotNil(responseProto.oauthScope, @"Response's OAuth scope is nil."); - [response fulfill]; - } - completionHandler:^(NSError *errorOrNil) { - XCTAssertNil(errorOrNil, @"Finished with unexpected error: %@", errorOrNil); - [completion fulfill]; - }]; - - [call startWithWriteable:responsesWriteable]; - - [self waitForExpectationsWithTimeout:TEST_TIMEOUT handler:nil]; -} - - (void)testAlternateDispatchQueue { const int32_t kPayloadSize = 100; RMTSimpleRequest *request = [RMTSimpleRequest message]; diff --git a/test/core/end2end/end2end_nosec_tests.cc b/test/core/end2end/end2end_nosec_tests.cc index cddd3d190f2..10705c53afc 100644 --- a/test/core/end2end/end2end_nosec_tests.cc +++ b/test/core/end2end/end2end_nosec_tests.cc @@ -83,8 +83,6 @@ extern void high_initial_seqno(grpc_end2end_test_config config); extern void high_initial_seqno_pre_init(void); extern void hpack_size(grpc_end2end_test_config config); extern void hpack_size_pre_init(void); -extern void idempotent_request(grpc_end2end_test_config config); -extern void idempotent_request_pre_init(void); extern void invoke_large_request(grpc_end2end_test_config config); extern void invoke_large_request_pre_init(void); extern void keepalive_timeout(grpc_end2end_test_config config); @@ -197,8 +195,6 @@ extern void shutdown_finishes_calls(grpc_end2end_test_config config); extern void shutdown_finishes_calls_pre_init(void); extern void shutdown_finishes_tags(grpc_end2end_test_config config); extern void shutdown_finishes_tags_pre_init(void); -extern void simple_cacheable_request(grpc_end2end_test_config config); -extern void simple_cacheable_request_pre_init(void); extern void simple_delayed_request(grpc_end2end_test_config config); extern void simple_delayed_request_pre_init(void); extern void simple_metadata(grpc_end2end_test_config config); @@ -244,7 +240,6 @@ void grpc_end2end_tests_pre_init(void) { graceful_server_shutdown_pre_init(); high_initial_seqno_pre_init(); hpack_size_pre_init(); - idempotent_request_pre_init(); invoke_large_request_pre_init(); keepalive_timeout_pre_init(); large_metadata_pre_init(); @@ -301,7 +296,6 @@ void grpc_end2end_tests_pre_init(void) { server_streaming_pre_init(); shutdown_finishes_calls_pre_init(); shutdown_finishes_tags_pre_init(); - simple_cacheable_request_pre_init(); simple_delayed_request_pre_init(); simple_metadata_pre_init(); simple_request_pre_init(); @@ -346,7 +340,6 @@ void grpc_end2end_tests(int argc, char **argv, graceful_server_shutdown(config); high_initial_seqno(config); hpack_size(config); - idempotent_request(config); invoke_large_request(config); keepalive_timeout(config); large_metadata(config); @@ -403,7 +396,6 @@ void grpc_end2end_tests(int argc, char **argv, server_streaming(config); shutdown_finishes_calls(config); shutdown_finishes_tags(config); - simple_cacheable_request(config); simple_delayed_request(config); simple_metadata(config); simple_request(config); @@ -523,10 +515,6 @@ void grpc_end2end_tests(int argc, char **argv, hpack_size(config); continue; } - if (0 == strcmp("idempotent_request", argv[i])) { - idempotent_request(config); - continue; - } if (0 == strcmp("invoke_large_request", argv[i])) { invoke_large_request(config); continue; @@ -751,10 +739,6 @@ void grpc_end2end_tests(int argc, char **argv, shutdown_finishes_tags(config); continue; } - if (0 == strcmp("simple_cacheable_request", argv[i])) { - simple_cacheable_request(config); - continue; - } if (0 == strcmp("simple_delayed_request", argv[i])) { simple_delayed_request(config); continue; diff --git a/test/core/end2end/end2end_tests.cc b/test/core/end2end/end2end_tests.cc index 81c07fbd4ad..eede2c6e82d 100644 --- a/test/core/end2end/end2end_tests.cc +++ b/test/core/end2end/end2end_tests.cc @@ -87,8 +87,6 @@ extern void high_initial_seqno(grpc_end2end_test_config config); extern void high_initial_seqno_pre_init(void); extern void hpack_size(grpc_end2end_test_config config); extern void hpack_size_pre_init(void); -extern void idempotent_request(grpc_end2end_test_config config); -extern void idempotent_request_pre_init(void); extern void invoke_large_request(grpc_end2end_test_config config); extern void invoke_large_request_pre_init(void); extern void keepalive_timeout(grpc_end2end_test_config config); @@ -201,8 +199,6 @@ extern void shutdown_finishes_calls(grpc_end2end_test_config config); extern void shutdown_finishes_calls_pre_init(void); extern void shutdown_finishes_tags(grpc_end2end_test_config config); extern void shutdown_finishes_tags_pre_init(void); -extern void simple_cacheable_request(grpc_end2end_test_config config); -extern void simple_cacheable_request_pre_init(void); extern void simple_delayed_request(grpc_end2end_test_config config); extern void simple_delayed_request_pre_init(void); extern void simple_metadata(grpc_end2end_test_config config); @@ -250,7 +246,6 @@ void grpc_end2end_tests_pre_init(void) { grpc_authz_pre_init(); high_initial_seqno_pre_init(); hpack_size_pre_init(); - idempotent_request_pre_init(); invoke_large_request_pre_init(); keepalive_timeout_pre_init(); large_metadata_pre_init(); @@ -307,7 +302,6 @@ void grpc_end2end_tests_pre_init(void) { server_streaming_pre_init(); shutdown_finishes_calls_pre_init(); shutdown_finishes_tags_pre_init(); - simple_cacheable_request_pre_init(); simple_delayed_request_pre_init(); simple_metadata_pre_init(); simple_request_pre_init(); @@ -354,7 +348,6 @@ void grpc_end2end_tests(int argc, char **argv, grpc_authz(config); high_initial_seqno(config); hpack_size(config); - idempotent_request(config); invoke_large_request(config); keepalive_timeout(config); large_metadata(config); @@ -411,7 +404,6 @@ void grpc_end2end_tests(int argc, char **argv, server_streaming(config); shutdown_finishes_calls(config); shutdown_finishes_tags(config); - simple_cacheable_request(config); simple_delayed_request(config); simple_metadata(config); simple_request(config); @@ -539,10 +531,6 @@ void grpc_end2end_tests(int argc, char **argv, hpack_size(config); continue; } - if (0 == strcmp("idempotent_request", argv[i])) { - idempotent_request(config); - continue; - } if (0 == strcmp("invoke_large_request", argv[i])) { invoke_large_request(config); continue; @@ -767,10 +755,6 @@ void grpc_end2end_tests(int argc, char **argv, shutdown_finishes_tags(config); continue; } - if (0 == strcmp("simple_cacheable_request", argv[i])) { - simple_cacheable_request(config); - continue; - } if (0 == strcmp("simple_delayed_request", argv[i])) { simple_delayed_request(config); continue; diff --git a/test/core/end2end/generate_tests.bzl b/test/core/end2end/generate_tests.bzl index 6f856b9fb40..861f87e8410 100755 --- a/test/core/end2end/generate_tests.bzl +++ b/test/core/end2end/generate_tests.bzl @@ -273,7 +273,6 @@ END2END_TESTS = { exclude_inproc = True, ), "high_initial_seqno": _test_options(), - "idempotent_request": _test_options(), "invoke_large_request": _test_options(exclude_1byte = True), "keepalive_timeout": _test_options(proxyable = False, needs_http2 = True), "large_metadata": _test_options(exclude_1byte = True), @@ -379,7 +378,6 @@ END2END_TESTS = { "server_streaming": _test_options(needs_http2 = True), "shutdown_finishes_calls": _test_options(), "shutdown_finishes_tags": _test_options(), - "simple_cacheable_request": _test_options(), "simple_delayed_request": _test_options(needs_fullstack = True), "simple_metadata": _test_options(), "simple_request": _test_options(), diff --git a/test/core/end2end/tests/idempotent_request.cc b/test/core/end2end/tests/idempotent_request.cc deleted file mode 100644 index a271c96c97a..00000000000 --- a/test/core/end2end/tests/idempotent_request.cc +++ /dev/null @@ -1,239 +0,0 @@ -/* - * - * Copyright 2015 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 - -#include -#include -#include -#include -#include - -#include "src/core/lib/gpr/string.h" -#include "test/core/end2end/cq_verifier.h" -#include "test/core/end2end/end2end_tests.h" - -static void* tag(intptr_t t) { return reinterpret_cast(t); } - -static grpc_end2end_test_fixture begin_test(grpc_end2end_test_config config, - const char* test_name, - grpc_channel_args* client_args, - grpc_channel_args* server_args) { - grpc_end2end_test_fixture f; - gpr_log(GPR_INFO, "Running test: %s/%s", test_name, config.name); - f = config.create_fixture(client_args, server_args); - config.init_server(&f, server_args); - config.init_client(&f, client_args); - return f; -} - -static gpr_timespec n_seconds_from_now(int n) { - return grpc_timeout_seconds_to_deadline(n); -} - -static gpr_timespec five_seconds_from_now(void) { - return n_seconds_from_now(5); -} - -static void drain_cq(grpc_completion_queue* cq) { - grpc_event ev; - do { - ev = grpc_completion_queue_next(cq, five_seconds_from_now(), nullptr); - } while (ev.type != GRPC_QUEUE_SHUTDOWN); -} - -static void shutdown_server(grpc_end2end_test_fixture* f) { - if (!f->server) return; - grpc_server_shutdown_and_notify(f->server, f->shutdown_cq, tag(1000)); - GPR_ASSERT(grpc_completion_queue_pluck(f->shutdown_cq, tag(1000), - grpc_timeout_seconds_to_deadline(5), - nullptr) - .type == GRPC_OP_COMPLETE); - grpc_server_destroy(f->server); - f->server = nullptr; -} - -static void shutdown_client(grpc_end2end_test_fixture* f) { - if (!f->client) return; - grpc_channel_destroy(f->client); - f->client = nullptr; -} - -static void end_test(grpc_end2end_test_fixture* f) { - shutdown_server(f); - shutdown_client(f); - - grpc_completion_queue_shutdown(f->cq); - drain_cq(f->cq); - grpc_completion_queue_destroy(f->cq); - grpc_completion_queue_destroy(f->shutdown_cq); -} - -static void simple_request_body(grpc_end2end_test_config /*config*/, - grpc_end2end_test_fixture f) { - grpc_call* c; - grpc_call* s; - cq_verifier* cqv = cq_verifier_create(f.cq); - grpc_op ops[6]; - grpc_op* op; - grpc_metadata_array initial_metadata_recv; - grpc_metadata_array trailing_metadata_recv; - grpc_metadata_array request_metadata_recv; - grpc_call_details call_details; - grpc_status_code status; - grpc_call_error error; - grpc_slice details; - int was_cancelled = 2; - char* peer; - - gpr_timespec deadline = five_seconds_from_now(); - c = grpc_channel_create_call(f.client, nullptr, GRPC_PROPAGATE_DEFAULTS, f.cq, - grpc_slice_from_static_string("/foo"), nullptr, - deadline, nullptr); - GPR_ASSERT(c); - - peer = grpc_call_get_peer(c); - GPR_ASSERT(peer != nullptr); - gpr_log(GPR_DEBUG, "client_peer_before_call=%s", peer); - gpr_free(peer); - - grpc_metadata_array_init(&initial_metadata_recv); - grpc_metadata_array_init(&trailing_metadata_recv); - grpc_metadata_array_init(&request_metadata_recv); - grpc_call_details_init(&call_details); - - memset(ops, 0, sizeof(ops)); - op = ops; - op->op = GRPC_OP_SEND_INITIAL_METADATA; - op->data.send_initial_metadata.count = 0; - op->flags = GRPC_INITIAL_METADATA_IDEMPOTENT_REQUEST; - op->reserved = nullptr; - op++; - op->op = GRPC_OP_SEND_CLOSE_FROM_CLIENT; - op->flags = 0; - op->reserved = nullptr; - op++; - op->op = GRPC_OP_RECV_INITIAL_METADATA; - op->data.recv_initial_metadata.recv_initial_metadata = &initial_metadata_recv; - op->flags = 0; - op->reserved = nullptr; - op++; - op->op = GRPC_OP_RECV_STATUS_ON_CLIENT; - op->data.recv_status_on_client.trailing_metadata = &trailing_metadata_recv; - op->data.recv_status_on_client.status = &status; - op->data.recv_status_on_client.status_details = &details; - op->flags = 0; - op->reserved = nullptr; - op++; - error = grpc_call_start_batch(c, ops, static_cast(op - ops), tag(1), - nullptr); - GPR_ASSERT(GRPC_CALL_OK == error); - - error = - grpc_server_request_call(f.server, &s, &call_details, - &request_metadata_recv, f.cq, f.cq, tag(101)); - GPR_ASSERT(GRPC_CALL_OK == error); - CQ_EXPECT_COMPLETION(cqv, tag(101), 1); - cq_verify(cqv); - - peer = grpc_call_get_peer(s); - GPR_ASSERT(peer != nullptr); - gpr_log(GPR_DEBUG, "server_peer=%s", peer); - gpr_free(peer); - peer = grpc_call_get_peer(c); - GPR_ASSERT(peer != nullptr); - gpr_log(GPR_DEBUG, "client_peer=%s", peer); - gpr_free(peer); - - memset(ops, 0, sizeof(ops)); - op = ops; - op->op = GRPC_OP_SEND_INITIAL_METADATA; - op->data.send_initial_metadata.count = 0; - op->flags = 0; - op->reserved = nullptr; - op++; - op->op = GRPC_OP_SEND_STATUS_FROM_SERVER; - op->data.send_status_from_server.trailing_metadata_count = 0; - op->data.send_status_from_server.status = GRPC_STATUS_UNIMPLEMENTED; - grpc_slice status_details = grpc_slice_from_static_string("xyz"); - op->data.send_status_from_server.status_details = &status_details; - op->flags = 0; - op->reserved = nullptr; - op++; - op->op = GRPC_OP_RECV_CLOSE_ON_SERVER; - op->data.recv_close_on_server.cancelled = &was_cancelled; - op->flags = 0; - op->reserved = nullptr; - op++; - error = grpc_call_start_batch(s, ops, static_cast(op - ops), tag(102), - nullptr); - GPR_ASSERT(GRPC_CALL_OK == error); - - CQ_EXPECT_COMPLETION(cqv, tag(102), 1); - CQ_EXPECT_COMPLETION(cqv, tag(1), 1); - cq_verify(cqv); - - GPR_ASSERT(status == GRPC_STATUS_UNIMPLEMENTED); - GPR_ASSERT(0 == grpc_slice_str_cmp(details, "xyz")); - GPR_ASSERT(0 == grpc_slice_str_cmp(call_details.method, "/foo")); - GPR_ASSERT(GRPC_INITIAL_METADATA_IDEMPOTENT_REQUEST == call_details.flags); - GPR_ASSERT(was_cancelled == 0); - - grpc_slice_unref(details); - grpc_metadata_array_destroy(&initial_metadata_recv); - grpc_metadata_array_destroy(&trailing_metadata_recv); - grpc_metadata_array_destroy(&request_metadata_recv); - grpc_call_details_destroy(&call_details); - - grpc_call_unref(c); - grpc_call_unref(s); - - cq_verifier_destroy(cqv); -} - -static void test_invoke_simple_request(grpc_end2end_test_config config) { - grpc_end2end_test_fixture f; - - f = begin_test(config, "test_invoke_simple_request", nullptr, nullptr); - simple_request_body(config, f); - end_test(&f); - config.tear_down_data(&f); -} - -static void test_invoke_10_simple_requests(grpc_end2end_test_config config) { - int i; - grpc_end2end_test_fixture f = - begin_test(config, "test_invoke_10_simple_requests", nullptr, nullptr); - for (i = 0; i < 10; i++) { - simple_request_body(config, f); - gpr_log(GPR_INFO, "Passed simple request %d", i); - } - end_test(&f); - config.tear_down_data(&f); -} - -void idempotent_request(grpc_end2end_test_config config) { - int i; - for (i = 0; i < 10; i++) { - test_invoke_simple_request(config); - } - test_invoke_10_simple_requests(config); -} - -void idempotent_request_pre_init(void) {} diff --git a/test/core/end2end/tests/simple_cacheable_request.cc b/test/core/end2end/tests/simple_cacheable_request.cc deleted file mode 100644 index f14df66a9cc..00000000000 --- a/test/core/end2end/tests/simple_cacheable_request.cc +++ /dev/null @@ -1,274 +0,0 @@ -/* - * - * Copyright 2015 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 - -#include -#include -#include -#include - -#include "test/core/end2end/cq_verifier.h" -#include "test/core/end2end/end2end_tests.h" - -enum { TIMEOUT = 200000 }; - -static void* tag(intptr_t t) { return reinterpret_cast(t); } - -static grpc_end2end_test_fixture begin_test(grpc_end2end_test_config config, - const char* test_name, - grpc_channel_args* client_args, - grpc_channel_args* server_args) { - grpc_end2end_test_fixture f; - gpr_log(GPR_INFO, "Running test: %s/%s", test_name, config.name); - f = config.create_fixture(client_args, server_args); - config.init_server(&f, server_args); - config.init_client(&f, client_args); - return f; -} - -static gpr_timespec n_seconds_from_now(int n) { - return grpc_timeout_seconds_to_deadline(n); -} - -static gpr_timespec five_seconds_from_now(void) { - return n_seconds_from_now(5); -} - -static void drain_cq(grpc_completion_queue* cq) { - grpc_event ev; - do { - ev = grpc_completion_queue_next(cq, five_seconds_from_now(), nullptr); - } while (ev.type != GRPC_QUEUE_SHUTDOWN); -} - -static void shutdown_server(grpc_end2end_test_fixture* f) { - if (!f->server) return; - grpc_server_shutdown_and_notify(f->server, f->shutdown_cq, tag(1000)); - GPR_ASSERT(grpc_completion_queue_pluck(f->shutdown_cq, tag(1000), - grpc_timeout_seconds_to_deadline(5), - nullptr) - .type == GRPC_OP_COMPLETE); - grpc_server_destroy(f->server); - f->server = nullptr; -} - -static void shutdown_client(grpc_end2end_test_fixture* f) { - if (!f->client) return; - grpc_channel_destroy(f->client); - f->client = nullptr; -} - -static void end_test(grpc_end2end_test_fixture* f) { - shutdown_server(f); - shutdown_client(f); - - grpc_completion_queue_shutdown(f->cq); - drain_cq(f->cq); - grpc_completion_queue_destroy(f->cq); - grpc_completion_queue_destroy(f->shutdown_cq); -} - -/* Request/response with metadata and payload.*/ -static void test_cacheable_request_response_with_metadata_and_payload( - grpc_end2end_test_config config) { - grpc_call* c; - grpc_call* s; - grpc_slice request_payload_slice = - grpc_slice_from_copied_string("hello world"); - grpc_slice response_payload_slice = - grpc_slice_from_copied_string("hello you"); - grpc_byte_buffer* request_payload = - grpc_raw_byte_buffer_create(&request_payload_slice, 1); - grpc_byte_buffer* response_payload = - grpc_raw_byte_buffer_create(&response_payload_slice, 1); - grpc_metadata meta_c[2] = {{grpc_slice_from_static_string("key1"), - grpc_slice_from_static_string("val1"), - {{nullptr, nullptr, nullptr, nullptr}}}, - {grpc_slice_from_static_string("key2"), - grpc_slice_from_static_string("val2"), - {{nullptr, nullptr, nullptr, nullptr}}}}; - grpc_metadata meta_s[2] = {{grpc_slice_from_static_string("key3"), - grpc_slice_from_static_string("val3"), - {{nullptr, nullptr, nullptr, nullptr}}}, - {grpc_slice_from_static_string("key4"), - grpc_slice_from_static_string("val4"), - {{nullptr, nullptr, nullptr, nullptr}}}}; - grpc_end2end_test_fixture f = begin_test( - config, "test_cacheable_request_response_with_metadata_and_payload", - nullptr, nullptr); - cq_verifier* cqv = cq_verifier_create(f.cq); - grpc_op ops[6]; - grpc_op* op; - grpc_metadata_array initial_metadata_recv; - grpc_metadata_array trailing_metadata_recv; - grpc_metadata_array request_metadata_recv; - grpc_byte_buffer* request_payload_recv = nullptr; - grpc_byte_buffer* response_payload_recv = nullptr; - grpc_call_details call_details; - grpc_status_code status; - grpc_call_error error; - grpc_slice details; - int was_cancelled = 2; - - gpr_timespec deadline = five_seconds_from_now(); - c = grpc_channel_create_call(f.client, nullptr, GRPC_PROPAGATE_DEFAULTS, f.cq, - grpc_slice_from_static_string("/foo"), nullptr, - deadline, nullptr); - GPR_ASSERT(c); - - grpc_metadata_array_init(&initial_metadata_recv); - grpc_metadata_array_init(&trailing_metadata_recv); - grpc_metadata_array_init(&request_metadata_recv); - grpc_call_details_init(&call_details); - - memset(ops, 0, sizeof(ops)); - op = ops; - op->op = GRPC_OP_SEND_INITIAL_METADATA; - op->data.send_initial_metadata.count = 2; - op->data.send_initial_metadata.metadata = meta_c; - op->flags = GRPC_INITIAL_METADATA_CACHEABLE_REQUEST; - op->reserved = nullptr; - op++; - op->op = GRPC_OP_SEND_MESSAGE; - op->data.send_message.send_message = request_payload; - op->flags = 0; - op->reserved = nullptr; - op++; - op->op = GRPC_OP_SEND_CLOSE_FROM_CLIENT; - op->flags = 0; - op->reserved = nullptr; - op++; - op->op = GRPC_OP_RECV_INITIAL_METADATA; - op->data.recv_initial_metadata.recv_initial_metadata = &initial_metadata_recv; - op->flags = 0; - op->reserved = nullptr; - op++; - op->op = GRPC_OP_RECV_MESSAGE; - op->data.recv_message.recv_message = &response_payload_recv; - op->flags = 0; - op->reserved = nullptr; - op++; - op->op = GRPC_OP_RECV_STATUS_ON_CLIENT; - op->data.recv_status_on_client.trailing_metadata = &trailing_metadata_recv; - op->data.recv_status_on_client.status = &status; - op->data.recv_status_on_client.status_details = &details; - op->flags = 0; - op->reserved = nullptr; - op++; - error = grpc_call_start_batch(c, ops, static_cast(op - ops), tag(1), - nullptr); - GPR_ASSERT(GRPC_CALL_OK == error); - - error = - grpc_server_request_call(f.server, &s, &call_details, - &request_metadata_recv, f.cq, f.cq, tag(101)); - GPR_ASSERT(GRPC_CALL_OK == error); - CQ_EXPECT_COMPLETION(cqv, tag(101), 1); - cq_verify(cqv); - - memset(ops, 0, sizeof(ops)); - op = ops; - op->op = GRPC_OP_SEND_INITIAL_METADATA; - op->data.send_initial_metadata.count = 2; - op->data.send_initial_metadata.metadata = meta_s; - op->flags = 0; - op->reserved = nullptr; - op++; - op->op = GRPC_OP_RECV_MESSAGE; - op->data.recv_message.recv_message = &request_payload_recv; - op->flags = 0; - op->reserved = nullptr; - op++; - error = grpc_call_start_batch(s, ops, static_cast(op - ops), tag(102), - nullptr); - GPR_ASSERT(GRPC_CALL_OK == error); - - CQ_EXPECT_COMPLETION(cqv, tag(102), 1); - cq_verify(cqv); - - memset(ops, 0, sizeof(ops)); - op = ops; - op->op = GRPC_OP_RECV_CLOSE_ON_SERVER; - op->data.recv_close_on_server.cancelled = &was_cancelled; - op->flags = 0; - op->reserved = nullptr; - op++; - op->op = GRPC_OP_SEND_MESSAGE; - op->data.send_message.send_message = response_payload; - op->flags = 0; - op->reserved = nullptr; - op++; - op->op = GRPC_OP_SEND_STATUS_FROM_SERVER; - op->data.send_status_from_server.trailing_metadata_count = 0; - op->data.send_status_from_server.status = GRPC_STATUS_OK; - grpc_slice status_details = grpc_slice_from_static_string("xyz"); - op->data.send_status_from_server.status_details = &status_details; - op->flags = 0; - op->reserved = nullptr; - op++; - error = grpc_call_start_batch(s, ops, static_cast(op - ops), tag(103), - nullptr); - GPR_ASSERT(GRPC_CALL_OK == error); - - CQ_EXPECT_COMPLETION(cqv, tag(103), 1); - CQ_EXPECT_COMPLETION(cqv, tag(1), 1); - cq_verify(cqv); - - GPR_ASSERT(status == GRPC_STATUS_OK); - GPR_ASSERT(0 == grpc_slice_str_cmp(details, "xyz")); - GPR_ASSERT(0 == grpc_slice_str_cmp(call_details.method, "/foo")); - if (config.feature_mask & FEATURE_MASK_SUPPORTS_REQUEST_PROXYING) { - // Our simple proxy does not support cacheable requests - } else { - GPR_ASSERT(GRPC_INITIAL_METADATA_CACHEABLE_REQUEST & call_details.flags); - } - GPR_ASSERT(was_cancelled == 0); - GPR_ASSERT(byte_buffer_eq_string(request_payload_recv, "hello world")); - GPR_ASSERT(byte_buffer_eq_string(response_payload_recv, "hello you")); - GPR_ASSERT(contains_metadata(&request_metadata_recv, "key1", "val1")); - GPR_ASSERT(contains_metadata(&request_metadata_recv, "key2", "val2")); - GPR_ASSERT(contains_metadata(&initial_metadata_recv, "key3", "val3")); - GPR_ASSERT(contains_metadata(&initial_metadata_recv, "key4", "val4")); - - grpc_slice_unref(details); - grpc_metadata_array_destroy(&initial_metadata_recv); - grpc_metadata_array_destroy(&trailing_metadata_recv); - grpc_metadata_array_destroy(&request_metadata_recv); - grpc_call_details_destroy(&call_details); - - grpc_call_unref(c); - grpc_call_unref(s); - - cq_verifier_destroy(cqv); - - grpc_byte_buffer_destroy(request_payload); - grpc_byte_buffer_destroy(response_payload); - grpc_byte_buffer_destroy(request_payload_recv); - grpc_byte_buffer_destroy(response_payload_recv); - - end_test(&f); - config.tear_down_data(&f); -} - -void simple_cacheable_request(grpc_end2end_test_config config) { - test_cacheable_request_response_with_metadata_and_payload(config); -} - -void simple_cacheable_request_pre_init(void) {} diff --git a/test/core/surface/server_test.cc b/test/core/surface/server_test.cc index 393bd827a07..b163fd2ef0f 100644 --- a/test/core/surface/server_test.cc +++ b/test/core/surface/server_test.cc @@ -44,17 +44,6 @@ void test_register_method_fail(void) { method = grpc_server_register_method( server, "m", "h", GRPC_SRM_PAYLOAD_READ_INITIAL_BYTE_BUFFER, 0); GPR_ASSERT(method == nullptr); - method_old = - grpc_server_register_method(server, "m2", "h2", GRPC_SRM_PAYLOAD_NONE, - GRPC_INITIAL_METADATA_IDEMPOTENT_REQUEST); - GPR_ASSERT(method_old != nullptr); - method = - grpc_server_register_method(server, "m2", "h2", GRPC_SRM_PAYLOAD_NONE, 0); - GPR_ASSERT(method == nullptr); - method = grpc_server_register_method( - server, "m2", "h2", GRPC_SRM_PAYLOAD_READ_INITIAL_BYTE_BUFFER, - GRPC_INITIAL_METADATA_IDEMPOTENT_REQUEST); - GPR_ASSERT(method == nullptr); grpc_server_destroy(server); } diff --git a/test/cpp/end2end/xds/xds_end2end_test.cc b/test/cpp/end2end/xds/xds_end2end_test.cc index c34b360a1c9..5b534b159f6 100644 --- a/test/cpp/end2end/xds/xds_end2end_test.cc +++ b/test/cpp/end2end/xds/xds_end2end_test.cc @@ -10515,21 +10515,8 @@ TEST_P(XdsRbacTestWithActionPermutations, MethodPostPermissionAnyPrincipal) { SendRpc([this]() { return CreateInsecureChannel(); }, {}, {}, /*test_expects_failure=*/GetParam().rbac_action() == RBAC_Action_DENY, grpc::StatusCode::PERMISSION_DENIED); - // Test an RPC with a different method type - auto stub = grpc::testing::EchoTestService::NewStub(CreateInsecureChannel()); - ClientContext context; - context.set_wait_for_ready(true); - context.set_deadline(grpc_timeout_milliseconds_to_deadline(2000)); - context.set_cacheable(true); - EchoRequest request; - request.set_message(kRequestMessage); - EchoResponse response; - Status status = stub->Echo(&context, request, &response); - EXPECT_EQ(status.error_code(), GetParam().rbac_action() == RBAC_Action_DENY - ? grpc::StatusCode::OK - : grpc::StatusCode::PERMISSION_DENIED) - << status.error_code() << ", " << status.error_message() << ", " - << status.error_details() << ", " << context.debug_error_string(); + // TODO(yashykt): When we start supporting GET/PUT requests in the future, + // this should be modified to test that they are NOT accepted with this rule. } TEST_P(XdsRbacTestWithActionPermutations, MethodGetPermissionAnyPrincipal) { @@ -10547,26 +10534,13 @@ TEST_P(XdsRbacTestWithActionPermutations, MethodGetPermissionAnyPrincipal) { backends_[0]->notifier()->WaitOnServingStatusChange( absl::StrCat(ipv6_only_ ? "[::1]:" : "127.0.0.1:", backends_[0]->port()), grpc::StatusCode::OK); - // Send a cacheable RPC so that GET method is used - auto stub = grpc::testing::EchoTestService::NewStub(CreateInsecureChannel()); - ClientContext context; - context.set_wait_for_ready(true); - context.set_deadline(grpc_timeout_milliseconds_to_deadline(2000)); - context.set_cacheable(true); - EchoRequest request; - request.set_message(kRequestMessage); - EchoResponse response; - Status status = stub->Echo(&context, request, &response); - EXPECT_EQ(status.error_code(), GetParam().rbac_action() == RBAC_Action_ALLOW - ? grpc::StatusCode::OK - : grpc::StatusCode::PERMISSION_DENIED) - << status.error_code() << ", " << status.error_message() << ", " - << status.error_details() << ", " << context.debug_error_string(); - // Test an RPC with a different method type + // Test that an RPC with a POST method gets rejected SendRpc( [this]() { return CreateInsecureChannel(); }, {}, {}, /*test_expects_failure=*/GetParam().rbac_action() == RBAC_Action_ALLOW, grpc::StatusCode::PERMISSION_DENIED); + // TODO(yashykt): When we start supporting GET requests in the future, this + // should be modified to test that they are accepted with this rule. } TEST_P(XdsRbacTestWithActionPermutations, MethodPutPermissionAnyPrincipal) { @@ -10584,26 +10558,13 @@ TEST_P(XdsRbacTestWithActionPermutations, MethodPutPermissionAnyPrincipal) { backends_[0]->notifier()->WaitOnServingStatusChange( absl::StrCat(ipv6_only_ ? "[::1]:" : "127.0.0.1:", backends_[0]->port()), grpc::StatusCode::OK); - // Send an idempotent RPC so that PUT method is used - auto stub = grpc::testing::EchoTestService::NewStub(CreateInsecureChannel()); - ClientContext context; - context.set_wait_for_ready(true); - context.set_deadline(grpc_timeout_milliseconds_to_deadline(2000)); - context.set_idempotent(true); - EchoRequest request; - request.set_message(kRequestMessage); - EchoResponse response; - Status status = stub->Echo(&context, request, &response); - EXPECT_EQ(status.error_code(), GetParam().rbac_action() == RBAC_Action_ALLOW - ? grpc::StatusCode::OK - : grpc::StatusCode::PERMISSION_DENIED) - << status.error_code() << ", " << status.error_message() << ", " - << status.error_details() << ", " << context.debug_error_string(); - // Test an RPC with a different method type + // Test that an RPC with a POST method gets rejected SendRpc( [this]() { return CreateInsecureChannel(); }, {}, {}, /*test_expects_failure=*/GetParam().rbac_action() == RBAC_Action_ALLOW, grpc::StatusCode::PERMISSION_DENIED); + // TODO(yashykt): When we start supporting PUT requests in the future, this + // should be modified to test that they are accepted with this rule. } TEST_P(XdsRbacTestWithActionPermutations, UrlPathPermissionAnyPrincipal) { @@ -10858,21 +10819,8 @@ TEST_P(XdsRbacTestWithActionPermutations, AnyPermissionMethodPostPrincipal) { SendRpc([this]() { return CreateInsecureChannel(); }, {}, {}, /*test_expects_failure=*/GetParam().rbac_action() == RBAC_Action_DENY, grpc::StatusCode::PERMISSION_DENIED); - // Test an RPC with a different method type - auto stub = grpc::testing::EchoTestService::NewStub(CreateInsecureChannel()); - ClientContext context; - context.set_wait_for_ready(true); - context.set_deadline(grpc_timeout_milliseconds_to_deadline(2000)); - context.set_cacheable(true); - EchoRequest request; - request.set_message(kRequestMessage); - EchoResponse response; - Status status = stub->Echo(&context, request, &response); - EXPECT_EQ(status.error_code(), GetParam().rbac_action() == RBAC_Action_DENY - ? grpc::StatusCode::OK - : grpc::StatusCode::PERMISSION_DENIED) - << status.error_code() << ", " << status.error_message() << ", " - << status.error_details() << ", " << context.debug_error_string(); + // TODO(yashykt): When we start supporting GET/PUT requests in the future, + // this should be modified to test that they are NOT accepted with this rule. } TEST_P(XdsRbacTestWithActionPermutations, AnyPermissionMethodGetPrincipal) { @@ -10890,25 +10838,13 @@ TEST_P(XdsRbacTestWithActionPermutations, AnyPermissionMethodGetPrincipal) { backends_[0]->notifier()->WaitOnServingStatusChange( absl::StrCat(ipv6_only_ ? "[::1]:" : "127.0.0.1:", backends_[0]->port()), grpc::StatusCode::OK); - // Send a cacheable RPC so that GET method is used - auto stub = grpc::testing::EchoTestService::NewStub(CreateInsecureChannel()); - ClientContext context; - context.set_wait_for_ready(true); - context.set_deadline(grpc_timeout_milliseconds_to_deadline(2000)); - context.set_cacheable(true); - EchoRequest request; - request.set_message(kRequestMessage); - EchoResponse response; - Status status = stub->Echo(&context, request, &response); - EXPECT_TRUE(GetParam().rbac_action() == RBAC_Action_ALLOW ? status.ok() - : !status.ok()) - << status.error_code() << ", " << status.error_message() << ", " - << status.error_details() << ", " << context.debug_error_string(); - // Test an RPC with a different method type + // Test that an RPC with a POST method gets rejected SendRpc( [this]() { return CreateInsecureChannel(); }, {}, {}, /*test_expects_failure=*/GetParam().rbac_action() == RBAC_Action_ALLOW, grpc::StatusCode::PERMISSION_DENIED); + // TODO(yashykt): When we start supporting GET requests in the future, this + // should be modified to test that they are accepted with this rule. } TEST_P(XdsRbacTestWithActionPermutations, AnyPermissionMethodPutPrincipal) { @@ -10926,25 +10862,13 @@ TEST_P(XdsRbacTestWithActionPermutations, AnyPermissionMethodPutPrincipal) { backends_[0]->notifier()->WaitOnServingStatusChange( absl::StrCat(ipv6_only_ ? "[::1]:" : "127.0.0.1:", backends_[0]->port()), grpc::StatusCode::OK); - // Send an idempotent RPC so that PUT method is used - auto stub = grpc::testing::EchoTestService::NewStub(CreateInsecureChannel()); - ClientContext context; - context.set_wait_for_ready(true); - context.set_deadline(grpc_timeout_milliseconds_to_deadline(2000)); - context.set_idempotent(true); - EchoRequest request; - request.set_message(kRequestMessage); - EchoResponse response; - Status status = stub->Echo(&context, request, &response); - EXPECT_TRUE(GetParam().rbac_action() == RBAC_Action_ALLOW ? status.ok() - : !status.ok()) - << status.error_code() << ", " << status.error_message() << ", " - << status.error_details() << ", " << context.debug_error_string(); - // Test an RPC with a different method type + // Test that an RPC with a POST method gets rejected SendRpc( [this]() { return CreateInsecureChannel(); }, {}, {}, /*test_expects_failure=*/GetParam().rbac_action() == RBAC_Action_ALLOW, grpc::StatusCode::PERMISSION_DENIED); + // TODO(yashykt): When we start supporting PUT requests in the future, this + // should be modified to test that they are accepted with this rule. } TEST_P(XdsRbacTestWithActionPermutations, AnyPermissionUrlPathPrincipal) { diff --git a/test/cpp/interop/client.cc b/test/cpp/interop/client.cc index e923f080e2a..77766b77fcc 100644 --- a/test/cpp/interop/client.cc +++ b/test/cpp/interop/client.cc @@ -295,8 +295,6 @@ int main(int argc, char** argv) { std::bind(&grpc::testing::InteropClient::DoUnimplementedMethod, &client); actions["unimplemented_service"] = std::bind(&grpc::testing::InteropClient::DoUnimplementedService, &client); - actions["cacheable_unary"] = - std::bind(&grpc::testing::InteropClient::DoCacheableUnary, &client); actions["channel_soak"] = std::bind( &grpc::testing::InteropClient::DoChannelSoakTest, &client, absl::GetFlag(FLAGS_soak_iterations), diff --git a/test/cpp/interop/interop_client.cc b/test/cpp/interop/interop_client.cc index c29f6eb9793..d52c789dc78 100644 --- a/test/cpp/interop/interop_client.cc +++ b/test/cpp/interop/interop_client.cc @@ -904,71 +904,6 @@ bool InteropClient::DoSpecialStatusMessage() { return true; } -bool InteropClient::DoCacheableUnary() { - gpr_log(GPR_DEBUG, "Sending RPC with cacheable response"); - - // Create request with current timestamp - gpr_timespec ts = gpr_now(GPR_CLOCK_PRECISE); - std::string timestamp = - std::to_string(static_cast(ts.tv_nsec)); - SimpleRequest request; - request.mutable_payload()->set_body(timestamp.c_str(), timestamp.size()); - - // Request 1 - ClientContext context1; - SimpleResponse response1; - context1.set_cacheable(true); - // Add fake user IP since some proxy's (GFE) won't cache requests from - // localhost. - context1.AddMetadata("x-user-ip", "1.2.3.4"); - Status s1 = - serviceStub_.Get()->CacheableUnaryCall(&context1, request, &response1); - if (!AssertStatusOk(s1, context1.debug_error_string())) { - return false; - } - gpr_log(GPR_DEBUG, "response 1 payload: %s", - response1.payload().body().c_str()); - - // Request 2 - ClientContext context2; - SimpleResponse response2; - context2.set_cacheable(true); - context2.AddMetadata("x-user-ip", "1.2.3.4"); - Status s2 = - serviceStub_.Get()->CacheableUnaryCall(&context2, request, &response2); - if (!AssertStatusOk(s2, context2.debug_error_string())) { - return false; - } - gpr_log(GPR_DEBUG, "response 2 payload: %s", - response2.payload().body().c_str()); - - // Check that the body is same for both requests. It will be the same if the - // second response is a cached copy of the first response - GPR_ASSERT(response2.payload().body() == response1.payload().body()); - - // Request 3 - // Modify the request body so it will not get a cache hit - ts = gpr_now(GPR_CLOCK_PRECISE); - timestamp = std::to_string(static_cast(ts.tv_nsec)); - SimpleRequest request1; - request1.mutable_payload()->set_body(timestamp.c_str(), timestamp.size()); - ClientContext context3; - SimpleResponse response3; - context3.set_cacheable(true); - context3.AddMetadata("x-user-ip", "1.2.3.4"); - Status s3 = - serviceStub_.Get()->CacheableUnaryCall(&context3, request1, &response3); - if (!AssertStatusOk(s3, context3.debug_error_string())) { - return false; - } - gpr_log(GPR_DEBUG, "response 3 payload: %s", - response3.payload().body().c_str()); - - // Check that the response is different from the previous response. - GPR_ASSERT(response3.payload().body() != response1.payload().body()); - return true; -} - bool InteropClient::DoPickFirstUnary() { const int rpcCount = 100; SimpleRequest request; diff --git a/test/cpp/interop/interop_client.h b/test/cpp/interop/interop_client.h index 8975815fb05..cbaf3a4d064 100644 --- a/test/cpp/interop/interop_client.h +++ b/test/cpp/interop/interop_client.h @@ -71,7 +71,6 @@ class InteropClient { bool DoCustomMetadata(); bool DoUnimplementedMethod(); bool DoUnimplementedService(); - bool DoCacheableUnary(); // all requests are sent to one server despite multiple servers are resolved bool DoPickFirstUnary();