diff --git a/.github/ISSUE_TEMPLATE/bug_report.md b/.github/ISSUE_TEMPLATE/bug_report.md index 2692dc7faa2..dbec90aefc9 100644 --- a/.github/ISSUE_TEMPLATE/bug_report.md +++ b/.github/ISSUE_TEMPLATE/bug_report.md @@ -2,7 +2,7 @@ name: Report a bug about: Create a report to help us improve labels: kind/bug, priority/P2 -assignees: karthikravis +assignees: veblush --- diff --git a/.github/ISSUE_TEMPLATE/cleanup_request.md b/.github/ISSUE_TEMPLATE/cleanup_request.md index bea10f3ef6a..d821fef2cf9 100644 --- a/.github/ISSUE_TEMPLATE/cleanup_request.md +++ b/.github/ISSUE_TEMPLATE/cleanup_request.md @@ -2,7 +2,7 @@ name: Request a cleanup about: Suggest a cleanup in our repository labels: kind/internal cleanup, priority/P2 -assignees: karthikravis +assignees: veblush --- diff --git a/.github/ISSUE_TEMPLATE/feature_request.md b/.github/ISSUE_TEMPLATE/feature_request.md index e11349754ba..5a2f61d118a 100644 --- a/.github/ISSUE_TEMPLATE/feature_request.md +++ b/.github/ISSUE_TEMPLATE/feature_request.md @@ -2,7 +2,7 @@ name: Request a feature about: Suggest an idea for this project labels: kind/enhancement, priority/P2 -assignees: karthikravis +assignees: veblush --- diff --git a/.github/ISSUE_TEMPLATE/question.md b/.github/ISSUE_TEMPLATE/question.md index db52e41626c..68f5dfb29bf 100644 --- a/.github/ISSUE_TEMPLATE/question.md +++ b/.github/ISSUE_TEMPLATE/question.md @@ -2,7 +2,7 @@ name: Ask a question about: Ask a question labels: kind/question, priority/P3 -assignees: karthikravis +assignees: veblush --- diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index 681d2f819ce..366b68604df 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -8,4 +8,4 @@ If you know who should review your pull request, please remove the mentioning be --> -@karthikravis +@veblush diff --git a/.gitignore b/.gitignore index 32d26a4bf2b..c0ec88139f2 100644 --- a/.gitignore +++ b/.gitignore @@ -23,6 +23,7 @@ a.out src/python/grpcio_*/LICENSE src/python/grpcio_status/grpc_status/google/rpc/status.proto .pytype +*.egg-info # Node installation output node_modules diff --git a/Makefile b/Makefile index e47879b7f9d..963c2960f02 100644 --- a/Makefile +++ b/Makefile @@ -1910,8 +1910,6 @@ test_c: buildtests_c $(Q) $(BINDIR)/$(CONFIG)/combiner_test || ( echo test combiner_test failed ; exit 1 ) $(E) "[RUN] Testing compression_test" $(Q) $(BINDIR)/$(CONFIG)/compression_test || ( echo test compression_test failed ; exit 1 ) - $(E) "[RUN] Testing concurrent_connectivity_test" - $(Q) $(BINDIR)/$(CONFIG)/concurrent_connectivity_test || ( echo test concurrent_connectivity_test failed ; exit 1 ) $(E) "[RUN] Testing connection_refused_test" $(Q) $(BINDIR)/$(CONFIG)/connection_refused_test || ( echo test connection_refused_test failed ; exit 1 ) $(E) "[RUN] Testing cpu_test" @@ -2184,8 +2182,6 @@ test_cxx: buildtests_cxx $(Q) $(BINDIR)/$(CONFIG)/byte_buffer_test || ( echo test byte_buffer_test failed ; exit 1 ) $(E) "[RUN] Testing byte_stream_test" $(Q) $(BINDIR)/$(CONFIG)/byte_stream_test || ( echo test byte_stream_test failed ; exit 1 ) - $(E) "[RUN] Testing cancel_ares_query_test" - $(Q) $(BINDIR)/$(CONFIG)/cancel_ares_query_test || ( echo test cancel_ares_query_test failed ; exit 1 ) $(E) "[RUN] Testing channel_arguments_test" $(Q) $(BINDIR)/$(CONFIG)/channel_arguments_test || ( echo test channel_arguments_test failed ; exit 1 ) $(E) "[RUN] Testing channel_filter_test" diff --git a/build_autogenerated.yaml b/build_autogenerated.yaml index d65be68d6c9..631005fa5bc 100644 --- a/build_autogenerated.yaml +++ b/build_autogenerated.yaml @@ -3176,6 +3176,7 @@ targets: uses_polling: false - name: concurrent_connectivity_test build: test + run: false language: c headers: [] src: @@ -5348,6 +5349,7 @@ targets: - name: cancel_ares_query_test gtest: true build: test + run: false language: c++ headers: - test/core/end2end/cq_verifier.h diff --git a/examples/php/greeter_client.php b/examples/php/greeter_client.php index e01ff73e3d9..1c7ec2a24fa 100644 --- a/examples/php/greeter_client.php +++ b/examples/php/greeter_client.php @@ -17,24 +17,28 @@ * */ -// php:generate protoc --proto_path=./../protos --php_out=./ --grpc_out=./ --plugin=protoc-gen-grpc=./../../bins/opt/grpc_php_plugin ./../protos/helloworld.proto +// To generate the necessary proto classes: +// $ protoc --proto_path=../protos --php_out=. --grpc_out=. +// --plugin=protoc-gen-grpc=../../bins/opt/grpc_php_plugin +// ../protos/helloworld.proto require dirname(__FILE__).'/vendor/autoload.php'; -function greet($name) +function greet($hostname, $name) { - $client = new Helloworld\GreeterClient('localhost:50051', [ + $client = new Helloworld\GreeterClient($hostname, [ 'credentials' => Grpc\ChannelCredentials::createInsecure(), ]); $request = new Helloworld\HelloRequest(); $request->setName($name); - list($reply, $status) = $client->SayHello($request)->wait(); + list($response, $status) = $client->SayHello($request)->wait(); if ($status->code !== Grpc\STATUS_OK) { - echo "ERROR: ".$status->code.", ".$status->details."\n"; + echo "ERROR: " . $status->code . ", " . $status->details . PHP_EOL; exit(1); } - echo $reply->getMessage()."\n"; + echo $response->getMessage() . PHP_EOL; } $name = !empty($argv[1]) ? $argv[1] : 'world'; -greet($name); +$hostname = !empty($argv[2]) ? $argv[2] : 'localhost:50051'; +greet($hostname, $name); diff --git a/examples/php/greeter_proto_gen.sh b/examples/php/greeter_proto_gen.sh index a0d7b29c86e..4322bbda2e3 100755 --- a/examples/php/greeter_proto_gen.sh +++ b/examples/php/greeter_proto_gen.sh @@ -13,5 +13,4 @@ # See the License for the specific language governing permissions and # limitations under the License. -protoc --proto_path=./../protos --php_out=./ --grpc_out=./ --plugin=protoc-gen-grpc=./../../bins/opt/grpc_php_plugin ./../protos/helloworld.proto - +protoc --proto_path=../protos --php_out=. --grpc_out=. --plugin=protoc-gen-grpc=../../bins/opt/grpc_php_plugin ../protos/helloworld.proto diff --git a/examples/python/xds/README.md b/examples/python/xds/README.md index 28014c7c965..ceb59216ce1 100644 --- a/examples/python/xds/README.md +++ b/examples/python/xds/README.md @@ -57,7 +57,7 @@ export GRPC_XDS_BOOTSTRAP=/etc/xds-bootstrap.json Finally, run your client: ``` -python client.py xds-experimental:///my-backend +python client.py xds:///my-backend ``` Alternatively, `grpcurl` can be used to test your server. If you don't have it, diff --git a/examples/ruby/greeter_client.rb b/examples/ruby/greeter_client.rb index 507d254ec9b..56b41e370d6 100755 --- a/examples/ruby/greeter_client.rb +++ b/examples/ruby/greeter_client.rb @@ -26,8 +26,9 @@ require 'grpc' require 'helloworld_services_pb' def main - stub = Helloworld::Greeter::Stub.new('localhost:50051', :this_channel_is_insecure) user = ARGV.size > 0 ? ARGV[0] : 'world' + hostname = ARGV.size > 1 ? ARGV[1] : 'localhost:50051' + stub = Helloworld::Greeter::Stub.new(hostname, :this_channel_is_insecure) message = stub.say_hello(Helloworld::HelloRequest.new(name: user)).message p "Greeting: #{message}" end diff --git a/include/grpcpp/impl/codegen/client_callback_impl.h b/include/grpcpp/impl/codegen/client_callback_impl.h index 86a46d8c870..8e683743e06 100644 --- a/include/grpcpp/impl/codegen/client_callback_impl.h +++ b/include/grpcpp/impl/codegen/client_callback_impl.h @@ -461,51 +461,76 @@ class ClientCallbackReaderWriterImpl // 1. Send initial metadata (unless corked) + recv initial metadata // 2. Any read backlog // 3. Any write backlog - // 4. Recv trailing metadata (unless corked) + // 4. Recv trailing metadata, on_completion callback + started_ = true; + + start_tag_.Set(call_.call(), + [this](bool ok) { + reactor_->OnReadInitialMetadataDone(ok); + MaybeFinish(); + }, + &start_ops_, /*can_inline=*/false); if (!start_corked_) { start_ops_.SendInitialMetadata(&context_->send_initial_metadata_, context_->initial_metadata_flags()); } - + start_ops_.RecvInitialMetadata(context_); + start_ops_.set_core_cq_tag(&start_tag_); call_.PerformOps(&start_ops_); - { - grpc::internal::MutexLock lock(&start_mu_); - - if (backlog_.read_ops) { - call_.PerformOps(&read_ops_); - } - if (backlog_.write_ops) { - call_.PerformOps(&write_ops_); - } - if (backlog_.writes_done_ops) { - call_.PerformOps(&writes_done_ops_); - } - call_.PerformOps(&finish_ops_); - // The last thing in this critical section is to set started_ so that it - // can be used lock-free as well. - started_.store(true, std::memory_order_release); + // Also set up the read and write tags so that they don't have to be set up + // each time + write_tag_.Set(call_.call(), + [this](bool ok) { + reactor_->OnWriteDone(ok); + MaybeFinish(); + }, + &write_ops_, /*can_inline=*/false); + write_ops_.set_core_cq_tag(&write_tag_); + + read_tag_.Set(call_.call(), + [this](bool ok) { + reactor_->OnReadDone(ok); + MaybeFinish(); + }, + &read_ops_, /*can_inline=*/false); + read_ops_.set_core_cq_tag(&read_tag_); + if (read_ops_at_start_) { + call_.PerformOps(&read_ops_); + } + + if (write_ops_at_start_) { + call_.PerformOps(&write_ops_); + } + + if (writes_done_ops_at_start_) { + call_.PerformOps(&writes_done_ops_); } - // MaybeFinish outside the lock to make sure that destruction of this object - // doesn't take place while holding the lock (which would cause the lock to - // be released after destruction) - this->MaybeFinish(); + + finish_tag_.Set(call_.call(), [this](bool /*ok*/) { MaybeFinish(); }, + &finish_ops_, /*can_inline=*/false); + finish_ops_.ClientRecvStatus(context_, &finish_status_); + finish_ops_.set_core_cq_tag(&finish_tag_); + call_.PerformOps(&finish_ops_); } void Read(Response* msg) override { read_ops_.RecvMessage(msg); callbacks_outstanding_.fetch_add(1, std::memory_order_relaxed); - if (GPR_UNLIKELY(!started_.load(std::memory_order_acquire))) { - grpc::internal::MutexLock lock(&start_mu_); - if (GPR_LIKELY(!started_.load(std::memory_order_relaxed))) { - backlog_.read_ops = true; - return; - } + if (started_) { + call_.PerformOps(&read_ops_); + } else { + read_ops_at_start_ = true; } - call_.PerformOps(&read_ops_); } void Write(const Request* msg, ::grpc::WriteOptions options) override { + if (start_corked_) { + write_ops_.SendInitialMetadata(&context_->send_initial_metadata_, + context_->initial_metadata_flags()); + start_corked_ = false; + } + if (options.is_last_message()) { options.set_buffer_hint(); write_ops_.ClientSendClose(); @@ -513,22 +538,18 @@ class ClientCallbackReaderWriterImpl // TODO(vjpai): don't assert GPR_CODEGEN_ASSERT(write_ops_.SendMessagePtr(msg, options).ok()); callbacks_outstanding_.fetch_add(1, std::memory_order_relaxed); - if (GPR_UNLIKELY(corked_write_needed_)) { - write_ops_.SendInitialMetadata(&context_->send_initial_metadata_, - context_->initial_metadata_flags()); - corked_write_needed_ = false; - } - - if (GPR_UNLIKELY(!started_.load(std::memory_order_acquire))) { - grpc::internal::MutexLock lock(&start_mu_); - if (GPR_LIKELY(!started_.load(std::memory_order_relaxed))) { - backlog_.write_ops = true; - return; - } + if (started_) { + call_.PerformOps(&write_ops_); + } else { + write_ops_at_start_ = true; } - call_.PerformOps(&write_ops_); } void WritesDone() override { + if (start_corked_) { + writes_done_ops_.SendInitialMetadata(&context_->send_initial_metadata_, + context_->initial_metadata_flags()); + start_corked_ = false; + } writes_done_ops_.ClientSendClose(); writes_done_tag_.Set(call_.call(), [this](bool ok) { @@ -538,19 +559,11 @@ class ClientCallbackReaderWriterImpl &writes_done_ops_, /*can_inline=*/false); writes_done_ops_.set_core_cq_tag(&writes_done_tag_); callbacks_outstanding_.fetch_add(1, std::memory_order_relaxed); - if (GPR_UNLIKELY(corked_write_needed_)) { - writes_done_ops_.SendInitialMetadata(&context_->send_initial_metadata_, - context_->initial_metadata_flags()); - corked_write_needed_ = false; + if (started_) { + call_.PerformOps(&writes_done_ops_); + } else { + writes_done_ops_at_start_ = true; } - if (GPR_UNLIKELY(!started_.load(std::memory_order_acquire))) { - grpc::internal::MutexLock lock(&start_mu_); - if (GPR_LIKELY(!started_.load(std::memory_order_relaxed))) { - backlog_.writes_done_ops = true; - return; - } - } - call_.PerformOps(&writes_done_ops_); } void AddHold(int holds) override { @@ -567,42 +580,8 @@ class ClientCallbackReaderWriterImpl : context_(context), call_(call), reactor_(reactor), - start_corked_(context_->initial_metadata_corked_), - corked_write_needed_(start_corked_) { + start_corked_(context_->initial_metadata_corked_) { this->BindReactor(reactor); - - // Set up the unchanging parts of the start, read, and write tags and ops. - start_tag_.Set(call_.call(), - [this](bool ok) { - reactor_->OnReadInitialMetadataDone(ok); - MaybeFinish(); - }, - &start_ops_, /*can_inline=*/false); - start_ops_.RecvInitialMetadata(context_); - start_ops_.set_core_cq_tag(&start_tag_); - - write_tag_.Set(call_.call(), - [this](bool ok) { - reactor_->OnWriteDone(ok); - MaybeFinish(); - }, - &write_ops_, /*can_inline=*/false); - write_ops_.set_core_cq_tag(&write_tag_); - - read_tag_.Set(call_.call(), - [this](bool ok) { - reactor_->OnReadDone(ok); - MaybeFinish(); - }, - &read_ops_, /*can_inline=*/false); - read_ops_.set_core_cq_tag(&read_tag_); - - // Also set up the Finish tag and op set. - finish_tag_.Set(call_.call(), [this](bool /*ok*/) { MaybeFinish(); }, - &finish_ops_, - /*can_inline=*/false); - finish_ops_.ClientRecvStatus(context_, &finish_status_); - finish_ops_.set_core_cq_tag(&finish_tag_); } ::grpc_impl::ClientContext* const context_; @@ -613,9 +592,7 @@ class ClientCallbackReaderWriterImpl grpc::internal::CallOpRecvInitialMetadata> start_ops_; grpc::internal::CallbackWithSuccessTag start_tag_; - const bool start_corked_; - bool corked_write_needed_; // no lock needed since only accessed in - // Write/WritesDone which cannot be concurrent + bool start_corked_; grpc::internal::CallOpSet finish_ops_; grpc::internal::CallbackWithSuccessTag finish_tag_; @@ -626,27 +603,22 @@ class ClientCallbackReaderWriterImpl grpc::internal::CallOpClientSendClose> write_ops_; grpc::internal::CallbackWithSuccessTag write_tag_; + bool write_ops_at_start_{false}; grpc::internal::CallOpSet writes_done_ops_; grpc::internal::CallbackWithSuccessTag writes_done_tag_; + bool writes_done_ops_at_start_{false}; grpc::internal::CallOpSet> read_ops_; grpc::internal::CallbackWithSuccessTag read_tag_; + bool read_ops_at_start_{false}; - struct StartCallBacklog { - bool write_ops = false; - bool writes_done_ops = false; - bool read_ops = false; - }; - StartCallBacklog backlog_ /* GUARDED_BY(start_mu_) */; - - // Minimum of 3 callbacks to pre-register for start ops, StartCall, and finish - std::atomic callbacks_outstanding_{3}; - std::atomic_bool started_{false}; - grpc::internal::Mutex start_mu_; + // Minimum of 2 callbacks to pre-register for start and finish + std::atomic callbacks_outstanding_{2}; + bool started_{false}; }; template @@ -698,7 +670,8 @@ class ClientCallbackReaderImpl : public ClientCallbackReader { // This call initiates two batches, plus any backlog, each with a callback // 1. Send initial metadata (unless corked) + recv initial metadata // 2. Any backlog - // 3. Recv trailing metadata + // 3. Recv trailing metadata, on_completion callback + started_ = true; start_tag_.Set(call_.call(), [this](bool ok) { @@ -720,13 +693,8 @@ class ClientCallbackReaderImpl : public ClientCallbackReader { }, &read_ops_, /*can_inline=*/false); read_ops_.set_core_cq_tag(&read_tag_); - - { - grpc::internal::MutexLock lock(&start_mu_); - if (backlog_.read_ops) { - call_.PerformOps(&read_ops_); - } - started_.store(true, std::memory_order_release); + if (read_ops_at_start_) { + call_.PerformOps(&read_ops_); } finish_tag_.Set(call_.call(), [this](bool /*ok*/) { MaybeFinish(); }, @@ -739,14 +707,11 @@ class ClientCallbackReaderImpl : public ClientCallbackReader { void Read(Response* msg) override { read_ops_.RecvMessage(msg); callbacks_outstanding_.fetch_add(1, std::memory_order_relaxed); - if (GPR_UNLIKELY(!started_.load(std::memory_order_acquire))) { - grpc::internal::MutexLock lock(&start_mu_); - if (GPR_LIKELY(!started_.load(std::memory_order_relaxed))) { - backlog_.read_ops = true; - return; - } + if (started_) { + call_.PerformOps(&read_ops_); + } else { + read_ops_at_start_ = true; } - call_.PerformOps(&read_ops_); } void AddHold(int holds) override { @@ -787,16 +752,11 @@ class ClientCallbackReaderImpl : public ClientCallbackReader { grpc::internal::CallOpSet> read_ops_; grpc::internal::CallbackWithSuccessTag read_tag_; - - struct StartCallBacklog { - bool read_ops = false; - }; - StartCallBacklog backlog_ /* GUARDED_BY(start_mu_) */; + bool read_ops_at_start_{false}; // Minimum of 2 callbacks to pre-register for start and finish std::atomic callbacks_outstanding_{2}; - std::atomic_bool started_{false}; - grpc::internal::Mutex start_mu_; + bool started_{false}; }; template @@ -849,60 +809,74 @@ class ClientCallbackWriterImpl : public ClientCallbackWriter { // This call initiates two batches, plus any backlog, each with a callback // 1. Send initial metadata (unless corked) + recv initial metadata // 2. Any backlog - // 3. Recv trailing metadata + // 3. Recv trailing metadata, on_completion callback + started_ = true; + start_tag_.Set(call_.call(), + [this](bool ok) { + reactor_->OnReadInitialMetadataDone(ok); + MaybeFinish(); + }, + &start_ops_, /*can_inline=*/false); if (!start_corked_) { start_ops_.SendInitialMetadata(&context_->send_initial_metadata_, context_->initial_metadata_flags()); } + start_ops_.RecvInitialMetadata(context_); + start_ops_.set_core_cq_tag(&start_tag_); call_.PerformOps(&start_ops_); - { - grpc::internal::MutexLock lock(&start_mu_); - - if (backlog_.write_ops) { - call_.PerformOps(&write_ops_); - } - if (backlog_.writes_done_ops) { - call_.PerformOps(&writes_done_ops_); - } - call_.PerformOps(&finish_ops_); - // The last thing in this critical section is to set started_ so that it - // can be used lock-free as well. - started_.store(true, std::memory_order_release); + // Also set up the read and write tags so that they don't have to be set up + // each time + write_tag_.Set(call_.call(), + [this](bool ok) { + reactor_->OnWriteDone(ok); + MaybeFinish(); + }, + &write_ops_, /*can_inline=*/false); + write_ops_.set_core_cq_tag(&write_tag_); + + if (write_ops_at_start_) { + call_.PerformOps(&write_ops_); } - // MaybeFinish outside the lock to make sure that destruction of this object - // doesn't take place while holding the lock (which would cause the lock to - // be released after destruction) - this->MaybeFinish(); + + if (writes_done_ops_at_start_) { + call_.PerformOps(&writes_done_ops_); + } + + finish_tag_.Set(call_.call(), [this](bool /*ok*/) { MaybeFinish(); }, + &finish_ops_, /*can_inline=*/false); + finish_ops_.ClientRecvStatus(context_, &finish_status_); + finish_ops_.set_core_cq_tag(&finish_tag_); + call_.PerformOps(&finish_ops_); } void Write(const Request* msg, ::grpc::WriteOptions options) override { - if (GPR_UNLIKELY(options.is_last_message())) { + if (start_corked_) { + write_ops_.SendInitialMetadata(&context_->send_initial_metadata_, + context_->initial_metadata_flags()); + start_corked_ = false; + } + + if (options.is_last_message()) { options.set_buffer_hint(); write_ops_.ClientSendClose(); } // TODO(vjpai): don't assert GPR_CODEGEN_ASSERT(write_ops_.SendMessagePtr(msg, options).ok()); callbacks_outstanding_.fetch_add(1, std::memory_order_relaxed); - - if (GPR_UNLIKELY(corked_write_needed_)) { - write_ops_.SendInitialMetadata(&context_->send_initial_metadata_, - context_->initial_metadata_flags()); - corked_write_needed_ = false; + if (started_) { + call_.PerformOps(&write_ops_); + } else { + write_ops_at_start_ = true; } - - if (GPR_UNLIKELY(!started_.load(std::memory_order_acquire))) { - grpc::internal::MutexLock lock(&start_mu_); - if (GPR_LIKELY(!started_.load(std::memory_order_relaxed))) { - backlog_.write_ops = true; - return; - } - } - call_.PerformOps(&write_ops_); } - void WritesDone() override { + if (start_corked_) { + writes_done_ops_.SendInitialMetadata(&context_->send_initial_metadata_, + context_->initial_metadata_flags()); + start_corked_ = false; + } writes_done_ops_.ClientSendClose(); writes_done_tag_.Set(call_.call(), [this](bool ok) { @@ -912,21 +886,11 @@ class ClientCallbackWriterImpl : public ClientCallbackWriter { &writes_done_ops_, /*can_inline=*/false); writes_done_ops_.set_core_cq_tag(&writes_done_tag_); callbacks_outstanding_.fetch_add(1, std::memory_order_relaxed); - - if (GPR_UNLIKELY(corked_write_needed_)) { - writes_done_ops_.SendInitialMetadata(&context_->send_initial_metadata_, - context_->initial_metadata_flags()); - corked_write_needed_ = false; - } - - if (GPR_UNLIKELY(!started_.load(std::memory_order_acquire))) { - grpc::internal::MutexLock lock(&start_mu_); - if (GPR_LIKELY(!started_.load(std::memory_order_relaxed))) { - backlog_.writes_done_ops = true; - return; - } + if (started_) { + call_.PerformOps(&writes_done_ops_); + } else { + writes_done_ops_at_start_ = true; } - call_.PerformOps(&writes_done_ops_); } void AddHold(int holds) override { @@ -945,36 +909,10 @@ class ClientCallbackWriterImpl : public ClientCallbackWriter { : context_(context), call_(call), reactor_(reactor), - start_corked_(context_->initial_metadata_corked_), - corked_write_needed_(start_corked_) { + start_corked_(context_->initial_metadata_corked_) { this->BindReactor(reactor); - - // Set up the unchanging parts of the start and write tags and ops. - start_tag_.Set(call_.call(), - [this](bool ok) { - reactor_->OnReadInitialMetadataDone(ok); - MaybeFinish(); - }, - &start_ops_, /*can_inline=*/false); - start_ops_.RecvInitialMetadata(context_); - start_ops_.set_core_cq_tag(&start_tag_); - - write_tag_.Set(call_.call(), - [this](bool ok) { - reactor_->OnWriteDone(ok); - MaybeFinish(); - }, - &write_ops_, /*can_inline=*/false); - write_ops_.set_core_cq_tag(&write_tag_); - - // Also set up the Finish tag and op set. finish_ops_.RecvMessage(response); finish_ops_.AllowNoMessage(); - finish_tag_.Set(call_.call(), [this](bool /*ok*/) { MaybeFinish(); }, - &finish_ops_, - /*can_inline=*/false); - finish_ops_.ClientRecvStatus(context_, &finish_status_); - finish_ops_.set_core_cq_tag(&finish_tag_); } ::grpc_impl::ClientContext* const context_; @@ -985,9 +923,7 @@ class ClientCallbackWriterImpl : public ClientCallbackWriter { grpc::internal::CallOpRecvInitialMetadata> start_ops_; grpc::internal::CallbackWithSuccessTag start_tag_; - const bool start_corked_; - bool corked_write_needed_; // no lock needed since only accessed in - // Write/WritesDone which cannot be concurrent + bool start_corked_; grpc::internal::CallOpSet @@ -1000,22 +936,17 @@ class ClientCallbackWriterImpl : public ClientCallbackWriter { grpc::internal::CallOpClientSendClose> write_ops_; grpc::internal::CallbackWithSuccessTag write_tag_; + bool write_ops_at_start_{false}; grpc::internal::CallOpSet writes_done_ops_; grpc::internal::CallbackWithSuccessTag writes_done_tag_; + bool writes_done_ops_at_start_{false}; - struct StartCallBacklog { - bool write_ops = false; - bool writes_done_ops = false; - }; - StartCallBacklog backlog_ /* GUARDED_BY(start_mu_) */; - - // Minimum of 3 callbacks to pre-register for start ops, StartCall, and finish - std::atomic callbacks_outstanding_{3}; - std::atomic_bool started_{false}; - grpc::internal::Mutex start_mu_; + // Minimum of 2 callbacks to pre-register for start and finish + std::atomic callbacks_outstanding_{2}; + bool started_{false}; }; template @@ -1054,6 +985,7 @@ class ClientCallbackUnaryImpl final : public ClientCallbackUnary { // This call initiates two batches, each with a callback // 1. Send initial metadata + write + writes done + recv initial metadata // 2. Read message, recv trailing metadata + started_ = true; start_tag_.Set(call_.call(), [this](bool ok) { @@ -1121,6 +1053,7 @@ class ClientCallbackUnaryImpl final : public ClientCallbackUnary { // This call will have 2 callbacks: start and finish std::atomic callbacks_outstanding_{2}; + bool started_{false}; }; class ClientCallbackUnaryFactory { diff --git a/include/grpcpp/impl/codegen/interceptor.h b/include/grpcpp/impl/codegen/interceptor.h index 18c7cf33e2f..71c32d70f4c 100644 --- a/include/grpcpp/impl/codegen/interceptor.h +++ b/include/grpcpp/impl/codegen/interceptor.h @@ -19,6 +19,8 @@ #ifndef GRPCPP_IMPL_CODEGEN_INTERCEPTOR_H #define GRPCPP_IMPL_CODEGEN_INTERCEPTOR_H +#include + #include #include #include diff --git a/src/core/ext/filters/client_channel/client_channel.cc b/src/core/ext/filters/client_channel/client_channel.cc index 2b1be822bf3..d8af25d8f2f 100644 --- a/src/core/ext/filters/client_channel/client_channel.cc +++ b/src/core/ext/filters/client_channel/client_channel.cc @@ -3171,10 +3171,9 @@ void CallData::OnComplete(void* arg, grpc_error* error) { ChannelData* chand = static_cast(elem->channel_data); CallData* calld = static_cast(elem->call_data); if (GRPC_TRACE_FLAG_ENABLED(grpc_client_channel_call_trace)) { - char* batch_str = grpc_transport_stream_op_batch_string(&batch_data->batch); gpr_log(GPR_INFO, "chand=%p calld=%p: got on_complete, error=%s, batch=%s", - chand, calld, grpc_error_string(error), batch_str); - gpr_free(batch_str); + chand, calld, grpc_error_string(error), + grpc_transport_stream_op_batch_string(&batch_data->batch).c_str()); } SubchannelCallRetryState* retry_state = static_cast( @@ -3247,10 +3246,8 @@ void CallData::AddClosureForSubchannelBatch( GRPC_CLOSURE_INIT(&batch->handler_private.closure, StartBatchInCallCombiner, batch, grpc_schedule_on_exec_ctx); if (GRPC_TRACE_FLAG_ENABLED(grpc_client_channel_call_trace)) { - char* batch_str = grpc_transport_stream_op_batch_string(batch); gpr_log(GPR_INFO, "chand=%p calld=%p: starting subchannel batch: %s", chand, - this, batch_str); - gpr_free(batch_str); + this, grpc_transport_stream_op_batch_string(batch).c_str()); } closures->Add(&batch->handler_private.closure, GRPC_ERROR_NONE, "start_subchannel_batch"); diff --git a/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc b/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc index 91f6e6ad946..d2423571295 100644 --- a/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc +++ b/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc @@ -65,6 +65,8 @@ #include #include "absl/container/inlined_vector.h" +#include "absl/strings/str_format.h" +#include "absl/strings/str_join.h" #include #include @@ -235,7 +237,7 @@ class GrpcLb : public LoadBalancingPolicy { const std::vector& serverlist() const { return serverlist_; } // Returns a text representation suitable for logging. - grpc_core::UniquePtr AsText() const; + std::string AsText() const; // Extracts all non-drop entries into a ServerAddressList. ServerAddressList GetServerAddressList( @@ -445,9 +447,8 @@ void ParseServer(const GrpcLbServer& server, grpc_resolved_address* addr) { } } -grpc_core::UniquePtr GrpcLb::Serverlist::AsText() const { - gpr_strvec entries; - gpr_strvec_init(&entries); +std::string GrpcLb::Serverlist::AsText() const { + std::vector entries; for (size_t i = 0; i < serverlist_.size(); ++i) { const GrpcLbServer& server = serverlist_[i]; std::string ipport; @@ -458,14 +459,10 @@ grpc_core::UniquePtr GrpcLb::Serverlist::AsText() const { ParseServer(server, &addr); ipport = grpc_sockaddr_to_string(&addr, false); } - char* entry; - gpr_asprintf(&entry, " %" PRIuPTR ": %s token=%s\n", i, ipport.c_str(), - server.load_balance_token); - gpr_strvec_add(&entries, entry); + entries.push_back(absl::StrFormat(" %" PRIuPTR ": %s token=%s\n", i, + ipport, server.load_balance_token)); } - grpc_core::UniquePtr result(gpr_strvec_flatten(&entries, nullptr)); - gpr_strvec_destroy(&entries); - return result; + return absl::StrJoin(entries, ""); } // vtables for channel args for LB token and client stats. @@ -1057,14 +1054,12 @@ void GrpcLb::BalancerCallState::OnBalancerMessageReceivedLocked() { auto serverlist_wrapper = MakeRefCounted(std::move(response.serverlist)); if (GRPC_TRACE_FLAG_ENABLED(grpc_lb_glb_trace)) { - grpc_core::UniquePtr serverlist_text = - serverlist_wrapper->AsText(); gpr_log(GPR_INFO, "[grpclb %p] lb_calld=%p: Serverlist with %" PRIuPTR " servers received:\n%s", grpclb_policy(), this, serverlist_wrapper->serverlist().size(), - serverlist_text.get()); + serverlist_wrapper->AsText().c_str()); } seen_serverlist_ = true; // Start sending client load report only after we start using the diff --git a/src/core/ext/filters/client_channel/lb_policy/xds/cds.cc b/src/core/ext/filters/client_channel/lb_policy/xds/cds.cc index 3869f81a623..500f1ac056b 100644 --- a/src/core/ext/filters/client_channel/lb_policy/xds/cds.cc +++ b/src/core/ext/filters/client_channel/lb_policy/xds/cds.cc @@ -93,6 +93,8 @@ class CdsLb : public LoadBalancingPolicy { void ShutdownLocked() override; + void MaybeDestroyChildPolicyLocked(); + RefCountedPtr config_; // Current channel args from the resolver. @@ -226,6 +228,7 @@ void CdsLb::ClusterWatcher::OnResourceDoesNotExist() { absl::StrCat("CDS resource \"", parent_->config_->cluster(), "\" does not exist") .c_str()))); + parent_->MaybeDestroyChildPolicyLocked(); } // @@ -240,7 +243,7 @@ RefCountedPtr CdsLb::Helper::CreateSubchannel( void CdsLb::Helper::UpdateState(grpc_connectivity_state state, std::unique_ptr picker) { - if (parent_->shutting_down_) return; + if (parent_->shutting_down_ || parent_->child_policy_ == nullptr) return; if (GRPC_TRACE_FLAG_ENABLED(grpc_cds_lb_trace)) { gpr_log(GPR_INFO, "[cdslb %p] state updated by child: %s", this, ConnectivityStateName(state)); @@ -287,11 +290,7 @@ void CdsLb::ShutdownLocked() { gpr_log(GPR_INFO, "[cdslb %p] shutting down", this); } shutting_down_ = true; - if (child_policy_ != nullptr) { - grpc_pollset_set_del_pollset_set(child_policy_->interested_parties(), - interested_parties()); - child_policy_.reset(); - } + MaybeDestroyChildPolicyLocked(); if (xds_client_ != nullptr) { if (cluster_watcher_ != nullptr) { if (GRPC_TRACE_FLAG_ENABLED(grpc_cds_lb_trace)) { @@ -304,6 +303,14 @@ void CdsLb::ShutdownLocked() { } } +void CdsLb::MaybeDestroyChildPolicyLocked() { + if (child_policy_ != nullptr) { + grpc_pollset_set_del_pollset_set(child_policy_->interested_parties(), + interested_parties()); + child_policy_.reset(); + } +} + void CdsLb::ResetBackoffLocked() { if (child_policy_ != nullptr) child_policy_->ResetBackoffLocked(); } diff --git a/src/core/ext/filters/client_channel/lb_policy/xds/eds.cc b/src/core/ext/filters/client_channel/lb_policy/xds/eds.cc index 4521d842818..c1bbf742cee 100644 --- a/src/core/ext/filters/client_channel/lb_policy/xds/eds.cc +++ b/src/core/ext/filters/client_channel/lb_policy/xds/eds.cc @@ -32,6 +32,7 @@ #include "src/core/ext/filters/client_channel/lb_policy_factory.h" #include "src/core/ext/filters/client_channel/lb_policy_registry.h" #include "src/core/ext/filters/client_channel/server_address.h" +#include "src/core/ext/filters/client_channel/xds/xds_channel_args.h" #include "src/core/ext/filters/client_channel/xds/xds_client.h" #include "src/core/ext/filters/client_channel/xds/xds_client_stats.h" #include "src/core/lib/channel/channel_args.h" @@ -148,6 +149,8 @@ class EdsLb : public LoadBalancingPolicy { void ShutdownLocked() override; + void MaybeDestroyChildPolicyLocked(); + void UpdatePriorityList(XdsApi::PriorityListUpdate priority_list_update); void UpdateChildPolicyLocked(); OrphanablePtr CreateChildPolicyLocked( @@ -263,7 +266,9 @@ RefCountedPtr EdsLb::Helper::CreateSubchannel( void EdsLb::Helper::UpdateState(grpc_connectivity_state state, std::unique_ptr picker) { - if (eds_policy_->shutting_down_) return; + if (eds_policy_->shutting_down_ || eds_policy_->child_policy_ == nullptr) { + return; + } if (GRPC_TRACE_FLAG_ENABLED(grpc_lb_eds_trace)) { gpr_log(GPR_INFO, "[edslb %p] child policy updated state=%s picker=%p", eds_policy_.get(), ConnectivityStateName(state), picker.get()); @@ -351,6 +356,7 @@ class EdsLb::EndpointWatcher : public XdsClient::EndpointWatcherInterface { absl::make_unique( GRPC_ERROR_CREATE_FROM_STATIC_STRING( "EDS resource does not exist"))); + eds_policy_->MaybeDestroyChildPolicyLocked(); } private: @@ -397,11 +403,7 @@ void EdsLb::ShutdownLocked() { // Drop our ref to the child's picker, in case it's holding a ref to // the child. child_picker_.reset(); - if (child_policy_ != nullptr) { - grpc_pollset_set_del_pollset_set(child_policy_->interested_parties(), - interested_parties()); - child_policy_.reset(); - } + MaybeDestroyChildPolicyLocked(); drop_stats_.reset(); // Cancel the endpoint watch here instead of in our dtor if we are using the // xds resolver, because the watcher holds a ref to us and we might not be @@ -421,6 +423,14 @@ void EdsLb::ShutdownLocked() { xds_client_.reset(); } +void EdsLb::MaybeDestroyChildPolicyLocked() { + if (child_policy_ != nullptr) { + grpc_pollset_set_del_pollset_set(child_policy_->interested_parties(), + interested_parties()); + child_policy_.reset(); + } +} + void EdsLb::UpdateLocked(UpdateArgs args) { if (GRPC_TRACE_FLAG_ENABLED(grpc_lb_eds_trace)) { gpr_log(GPR_INFO, "[edslb %p] Received update", this); @@ -715,11 +725,18 @@ grpc_channel_args* EdsLb::CreateChildPolicyArgsLocked( grpc_channel_arg_integer_create( const_cast(GRPC_ARG_INHIBIT_HEALTH_CHECKING), 1), }; + absl::InlinedVector args_to_remove; if (xds_client_from_channel_ == nullptr) { args_to_add.emplace_back(xds_client_->MakeChannelArg()); - } - return grpc_channel_args_copy_and_add(args, args_to_add.data(), - args_to_add.size()); + } else if (!config_->lrs_load_reporting_server_name().has_value()) { + // Remove XdsClient from channel args, so that its presence doesn't + // prevent us from sharing subchannels between channels. + // If load reporting is enabled, this happens in the LRS policy instead. + args_to_remove.push_back(GRPC_ARG_XDS_CLIENT); + } + return grpc_channel_args_copy_and_add_and_remove( + args, args_to_remove.data(), args_to_remove.size(), args_to_add.data(), + args_to_add.size()); } OrphanablePtr EdsLb::CreateChildPolicyLocked( diff --git a/src/core/ext/filters/client_channel/lb_policy/xds/lrs.cc b/src/core/ext/filters/client_channel/lb_policy/xds/lrs.cc index 28a9d76c12d..0aca12dfed2 100644 --- a/src/core/ext/filters/client_channel/lb_policy/xds/lrs.cc +++ b/src/core/ext/filters/client_channel/lb_policy/xds/lrs.cc @@ -254,9 +254,11 @@ void LrsLb::UpdateLocked(UpdateArgs args) { config_->eds_service_name(), config_->locality_name()); MaybeUpdatePickerLocked(); } + // Remove XdsClient from channel args, so that its presence doesn't + // prevent us from sharing subchannels between channels. + grpc_channel_args* new_args = XdsClient::RemoveFromChannelArgs(*args.args); // Update child policy. - UpdateChildPolicyLocked(std::move(args.addresses), args.args); - args.args = nullptr; // Ownership passed to UpdateChildPolicyLocked(). + UpdateChildPolicyLocked(std::move(args.addresses), new_args); } void LrsLb::MaybeUpdatePickerLocked() { diff --git a/src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper.cc b/src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper.cc index aaf27d76e97..2d4141c357d 100644 --- a/src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper.cc +++ b/src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper.cc @@ -181,6 +181,12 @@ void grpc_ares_complete_request_locked(grpc_ares_request* r) { // TODO(apolcyn): allow c-ares to return a service config // with no addresses along side it } + if (r->balancer_addresses_out != nullptr) { + ServerAddressList* balancer_addresses = r->balancer_addresses_out->get(); + if (balancer_addresses != nullptr) { + grpc_cares_wrapper_address_sorting_sort(r, balancer_addresses); + } + } grpc_core::ExecCtx::Run(DEBUG_LOCATION, r->on_done, r->error); } diff --git a/src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc b/src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc index 28ea4bb5c17..d3b79e39e2c 100644 --- a/src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc +++ b/src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc @@ -144,6 +144,8 @@ void XdsResolver::StartLocked() { class XdsResolverFactory : public ResolverFactory { public: + explicit XdsResolverFactory(const char* scheme) : scheme_(scheme) {} + bool IsValidUri(const grpc_uri* uri) const override { if (GPR_UNLIKELY(0 != strcmp(uri->authority, ""))) { gpr_log(GPR_ERROR, "URI authority not supported"); @@ -157,16 +159,26 @@ class XdsResolverFactory : public ResolverFactory { return MakeOrphanable(std::move(args)); } - const char* scheme() const override { return "xds-experimental"; } + const char* scheme() const override { return scheme_; } + + private: + const char* scheme_; }; +constexpr char kXdsScheme[] = "xds"; +constexpr char kXdsExperimentalScheme[] = "xds-experimental"; + } // namespace } // namespace grpc_core void grpc_resolver_xds_init() { grpc_core::ResolverRegistry::Builder::RegisterResolverFactory( - absl::make_unique()); + absl::make_unique(grpc_core::kXdsScheme)); + // TODO(roth): Remov this in the 1.31 release. + grpc_core::ResolverRegistry::Builder::RegisterResolverFactory( + absl::make_unique( + grpc_core::kXdsExperimentalScheme)); } void grpc_resolver_xds_shutdown() {} diff --git a/src/core/ext/filters/client_channel/resolving_lb_policy.cc b/src/core/ext/filters/client_channel/resolving_lb_policy.cc index bc69bae01cc..74a5c29968b 100644 --- a/src/core/ext/filters/client_channel/resolving_lb_policy.cc +++ b/src/core/ext/filters/client_channel/resolving_lb_policy.cc @@ -26,6 +26,9 @@ #include #include +#include "absl/strings/str_cat.h" +#include "absl/strings/str_join.h" + #include #include #include @@ -241,7 +244,6 @@ void ResolvingLoadBalancingPolicy::CreateOrUpdateLbPolicyLocked( } // Creates a new LB policy. -// Updates trace_strings to indicate what was done. OrphanablePtr ResolvingLoadBalancingPolicy::CreateLbPolicyLocked( const grpc_channel_args& args) { @@ -265,31 +267,21 @@ void ResolvingLoadBalancingPolicy::MaybeAddTraceMessagesForAddressChangesLocked( bool resolution_contains_addresses, TraceStringVector* trace_strings) { if (!resolution_contains_addresses && previous_resolution_contained_addresses_) { - trace_strings->push_back(gpr_strdup("Address list became empty")); + trace_strings->push_back("Address list became empty"); } else if (resolution_contains_addresses && !previous_resolution_contained_addresses_) { - trace_strings->push_back(gpr_strdup("Address list became non-empty")); + trace_strings->push_back("Address list became non-empty"); } previous_resolution_contained_addresses_ = resolution_contains_addresses; } void ResolvingLoadBalancingPolicy::ConcatenateAndAddChannelTraceLocked( - TraceStringVector* trace_strings) const { - if (!trace_strings->empty()) { - gpr_strvec v; - gpr_strvec_init(&v); - gpr_strvec_add(&v, gpr_strdup("Resolution event: ")); - bool is_first = 1; - for (size_t i = 0; i < trace_strings->size(); ++i) { - if (!is_first) gpr_strvec_add(&v, gpr_strdup(", ")); - is_first = false; - gpr_strvec_add(&v, (*trace_strings)[i]); - } - size_t len = 0; - grpc_core::UniquePtr message(gpr_strvec_flatten(&v, &len)); + const TraceStringVector& trace_strings) const { + if (!trace_strings.empty()) { + std::string message = + absl::StrCat("Resolution event: ", absl::StrJoin(trace_strings, ", ")); channel_control_helper()->AddTraceEvent(ChannelControlHelper::TRACE_INFO, - absl::string_view(message.get())); - gpr_strvec_destroy(&v); + message); } } @@ -314,7 +306,7 @@ void ResolvingLoadBalancingPolicy::OnResolverResultChangedLocked( // Process the resolver result. RefCountedPtr lb_policy_config; bool service_config_changed = false; - char* service_config_error_string = nullptr; + std::string service_config_error_string; if (process_resolver_result_ != nullptr) { grpc_error* service_config_error = GRPC_ERROR_NONE; bool no_valid_service_config = false; @@ -322,8 +314,7 @@ void ResolvingLoadBalancingPolicy::OnResolverResultChangedLocked( process_resolver_result_user_data_, result, &lb_policy_config, &service_config_error, &no_valid_service_config); if (service_config_error != GRPC_ERROR_NONE) { - service_config_error_string = - gpr_strdup(grpc_error_string(service_config_error)); + service_config_error_string = grpc_error_string(service_config_error); if (no_valid_service_config) { // We received an invalid service config and we don't have a // fallback service config. @@ -344,15 +335,14 @@ void ResolvingLoadBalancingPolicy::OnResolverResultChangedLocked( if (service_config_changed) { // TODO(ncteisen): might be worth somehow including a snippet of the // config in the trace, at the risk of bloating the trace logs. - trace_strings.push_back(gpr_strdup("Service config changed")); + trace_strings.push_back("Service config changed"); } - if (service_config_error_string != nullptr) { - trace_strings.push_back(service_config_error_string); - service_config_error_string = nullptr; + if (!service_config_error_string.empty()) { + trace_strings.push_back(service_config_error_string.c_str()); } MaybeAddTraceMessagesForAddressChangesLocked(resolution_contains_addresses, &trace_strings); - ConcatenateAndAddChannelTraceLocked(&trace_strings); + ConcatenateAndAddChannelTraceLocked(trace_strings); } } // namespace grpc_core diff --git a/src/core/ext/filters/client_channel/resolving_lb_policy.h b/src/core/ext/filters/client_channel/resolving_lb_policy.h index 8a04491424f..39815e28039 100644 --- a/src/core/ext/filters/client_channel/resolving_lb_policy.h +++ b/src/core/ext/filters/client_channel/resolving_lb_policy.h @@ -81,7 +81,7 @@ class ResolvingLoadBalancingPolicy : public LoadBalancingPolicy { void ResetBackoffLocked() override; private: - using TraceStringVector = absl::InlinedVector; + using TraceStringVector = absl::InlinedVector; class ResolverResultHandler; class ResolvingControlHelper; @@ -99,7 +99,7 @@ class ResolvingLoadBalancingPolicy : public LoadBalancingPolicy { void MaybeAddTraceMessagesForAddressChangesLocked( bool resolution_contains_addresses, TraceStringVector* trace_strings); void ConcatenateAndAddChannelTraceLocked( - TraceStringVector* trace_strings) const; + const TraceStringVector& trace_strings) const; void OnResolverResultChangedLocked(Resolver::Result result); // Passed in from caller at construction time. diff --git a/src/core/ext/filters/client_channel/xds/xds_bootstrap.cc b/src/core/ext/filters/client_channel/xds/xds_bootstrap.cc index d7d39c5b6f6..25ce4a7f5d1 100644 --- a/src/core/ext/filters/client_channel/xds/xds_bootstrap.cc +++ b/src/core/ext/filters/client_channel/xds/xds_bootstrap.cc @@ -23,6 +23,8 @@ #include #include +#include "absl/strings/str_format.h" +#include "absl/strings/str_join.h" #include "absl/strings/string_view.h" #include @@ -36,47 +38,36 @@ namespace grpc_core { namespace { -UniquePtr BootstrapString(const XdsBootstrap& bootstrap) { - gpr_strvec v; - gpr_strvec_init(&v); - char* tmp; +std::string BootstrapString(const XdsBootstrap& bootstrap) { + std::vector parts; if (bootstrap.node() != nullptr) { - gpr_asprintf(&tmp, - "node={\n" - " id=\"%s\",\n" - " cluster=\"%s\",\n" - " locality={\n" - " region=\"%s\",\n" - " zone=\"%s\",\n" - " subzone=\"%s\"\n" - " },\n" - " metadata=%s,\n" - "},\n", - bootstrap.node()->id.c_str(), - bootstrap.node()->cluster.c_str(), - bootstrap.node()->locality_region.c_str(), - bootstrap.node()->locality_zone.c_str(), - bootstrap.node()->locality_subzone.c_str(), - bootstrap.node()->metadata.Dump().c_str()); - gpr_strvec_add(&v, tmp); + parts.push_back(absl::StrFormat( + "node={\n" + " id=\"%s\",\n" + " cluster=\"%s\",\n" + " locality={\n" + " region=\"%s\",\n" + " zone=\"%s\",\n" + " subzone=\"%s\"\n" + " },\n" + " metadata=%s,\n" + "},\n", + bootstrap.node()->id, bootstrap.node()->cluster, + bootstrap.node()->locality_region, bootstrap.node()->locality_zone, + bootstrap.node()->locality_subzone, bootstrap.node()->metadata.Dump())); } - gpr_asprintf(&tmp, - "servers=[\n" - " {\n" - " uri=\"%s\",\n" - " creds=[\n", - bootstrap.server().server_uri.c_str()); - gpr_strvec_add(&v, tmp); - for (size_t i = 0; i < bootstrap.server().channel_creds.size(); ++i) { - const auto& creds = bootstrap.server().channel_creds[i]; - gpr_asprintf(&tmp, " {type=\"%s\", config=%s},\n", creds.type.c_str(), - creds.config.Dump().c_str()); - gpr_strvec_add(&v, tmp); + parts.push_back( + absl::StrFormat("servers=[\n" + " {\n" + " uri=\"%s\",\n" + " creds=[\n", + bootstrap.server().server_uri)); + for (const auto& creds : bootstrap.server().channel_creds) { + parts.push_back(absl::StrFormat(" {type=\"%s\", config=%s},\n", + creds.type, creds.config.Dump())); } - gpr_strvec_add(&v, gpr_strdup(" ]\n }\n]")); - UniquePtr result(gpr_strvec_flatten(&v, nullptr)); - gpr_strvec_destroy(&v); - return result; + parts.push_back(" ]\n }\n]"); + return absl::StrJoin(parts, ""); } } // namespace @@ -121,7 +112,7 @@ std::unique_ptr XdsBootstrap::ReadFromFile(XdsClient* client, if (*error == GRPC_ERROR_NONE && GRPC_TRACE_FLAG_ENABLED(*tracer)) { gpr_log(GPR_INFO, "[xds_client %p] Bootstrap config for creating xds client:\n%s", - client, BootstrapString(*result).get()); + client, BootstrapString(*result).c_str()); } return result; } diff --git a/src/core/ext/filters/client_channel/xds/xds_client.cc b/src/core/ext/filters/client_channel/xds/xds_client.cc index 3edb0584e34..7a407da627f 100644 --- a/src/core/ext/filters/client_channel/xds/xds_client.cc +++ b/src/core/ext/filters/client_channel/xds/xds_client.cc @@ -2387,4 +2387,10 @@ RefCountedPtr XdsClient::GetFromChannelArgs( return nullptr; } +grpc_channel_args* XdsClient::RemoveFromChannelArgs( + const grpc_channel_args& args) { + const char* arg_name = GRPC_ARG_XDS_CLIENT; + return grpc_channel_args_copy_and_remove(&args, &arg_name, 1); +} + } // namespace grpc_core diff --git a/src/core/ext/filters/client_channel/xds/xds_client.h b/src/core/ext/filters/client_channel/xds/xds_client.h index 133e99c4918..34e921fe54b 100644 --- a/src/core/ext/filters/client_channel/xds/xds_client.h +++ b/src/core/ext/filters/client_channel/xds/xds_client.h @@ -142,6 +142,8 @@ class XdsClient : public InternallyRefCounted { grpc_arg MakeChannelArg() const; static RefCountedPtr GetFromChannelArgs( const grpc_channel_args& args); + static grpc_channel_args* RemoveFromChannelArgs( + const grpc_channel_args& args); private: // Contains a channel to the xds server and all the data related to the 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 a0bea07c2fc..ef773b3a3ef 100644 --- a/src/core/ext/filters/http/client/http_client_filter.cc +++ b/src/core/ext/filters/http/client/http_client_filter.cc @@ -17,11 +17,19 @@ #include +#include +#include + +#include +#include + +#include "absl/strings/str_format.h" +#include "absl/strings/str_join.h" + #include #include #include -#include -#include + #include "src/core/ext/filters/http/client/http_client_filter.h" #include "src/core/lib/gpr/string.h" #include "src/core/lib/gprpp/manual_constructor.h" @@ -520,50 +528,36 @@ static size_t max_payload_size_from_args(const grpc_channel_args* args) { static grpc_core::ManagedMemorySlice user_agent_from_args( const grpc_channel_args* args, const char* transport_name) { - gpr_strvec v; - size_t i; - int is_first = 1; - char* tmp; + std::vector user_agent_fields; - gpr_strvec_init(&v); - - for (i = 0; args && i < args->num_args; i++) { + for (size_t i = 0; args && i < args->num_args; i++) { if (0 == strcmp(args->args[i].key, GRPC_ARG_PRIMARY_USER_AGENT_STRING)) { if (args->args[i].type != GRPC_ARG_STRING) { gpr_log(GPR_ERROR, "Channel argument '%s' should be a string", GRPC_ARG_PRIMARY_USER_AGENT_STRING); } else { - if (!is_first) gpr_strvec_add(&v, gpr_strdup(" ")); - is_first = 0; - gpr_strvec_add(&v, gpr_strdup(args->args[i].value.string)); + user_agent_fields.push_back(args->args[i].value.string); } } } - gpr_asprintf(&tmp, "%sgrpc-c/%s (%s; %s)", is_first ? "" : " ", - grpc_version_string(), GPR_PLATFORM_STRING, transport_name); - is_first = 0; - gpr_strvec_add(&v, tmp); + user_agent_fields.push_back( + absl::StrFormat("grpc-c/%s (%s; %s)", grpc_version_string(), + GPR_PLATFORM_STRING, transport_name)); - for (i = 0; args && i < args->num_args; i++) { + for (size_t i = 0; args && i < args->num_args; i++) { if (0 == strcmp(args->args[i].key, GRPC_ARG_SECONDARY_USER_AGENT_STRING)) { if (args->args[i].type != GRPC_ARG_STRING) { gpr_log(GPR_ERROR, "Channel argument '%s' should be a string", GRPC_ARG_SECONDARY_USER_AGENT_STRING); } else { - if (!is_first) gpr_strvec_add(&v, gpr_strdup(" ")); - is_first = 0; - gpr_strvec_add(&v, gpr_strdup(args->args[i].value.string)); + user_agent_fields.push_back(args->args[i].value.string); } } } - tmp = gpr_strvec_flatten(&v, nullptr); - gpr_strvec_destroy(&v); - grpc_core::ManagedMemorySlice result(tmp); - gpr_free(tmp); - - return result; + std::string user_agent_string = absl::StrJoin(user_agent_fields, " "); + return grpc_core::ManagedMemorySlice(user_agent_string.c_str()); } /* Constructor for channel_data */ diff --git a/src/core/ext/transport/chttp2/transport/chttp2_transport.cc b/src/core/ext/transport/chttp2/transport/chttp2_transport.cc index 68279c63ba7..5c96910f5e0 100644 --- a/src/core/ext/transport/chttp2/transport/chttp2_transport.cc +++ b/src/core/ext/transport/chttp2/transport/chttp2_transport.cc @@ -1356,10 +1356,8 @@ static void perform_stream_op_locked(void* stream_op, s->context = op->payload->context; s->traced = op->is_traced; if (GRPC_TRACE_FLAG_ENABLED(grpc_http_trace)) { - char* str = grpc_transport_stream_op_batch_string(op); - gpr_log(GPR_INFO, "perform_stream_op_locked: %s; on_complete = %p", str, - op->on_complete); - gpr_free(str); + gpr_log(GPR_INFO, "perform_stream_op_locked: %s; on_complete = %p", + grpc_transport_stream_op_batch_string(op).c_str(), op->on_complete); if (op->send_initial_metadata) { log_metadata(op_payload->send_initial_metadata.send_initial_metadata, s->id, t->is_client, true); @@ -1654,9 +1652,8 @@ static void perform_stream_op(grpc_transport* gt, grpc_stream* gs, } if (GRPC_TRACE_FLAG_ENABLED(grpc_http_trace)) { - char* str = grpc_transport_stream_op_batch_string(op); - gpr_log(GPR_INFO, "perform_stream_op[s=%p]: %s", s, str); - gpr_free(str); + gpr_log(GPR_INFO, "perform_stream_op[s=%p]: %s", s, + grpc_transport_stream_op_batch_string(op).c_str()); } GRPC_CHTTP2_STREAM_REF(s, "perform_stream_op"); @@ -1845,9 +1842,8 @@ static void perform_transport_op_locked(void* stream_op, static void perform_transport_op(grpc_transport* gt, grpc_transport_op* op) { grpc_chttp2_transport* t = reinterpret_cast(gt); if (GRPC_TRACE_FLAG_ENABLED(grpc_http_trace)) { - char* msg = grpc_transport_op_string(op); - gpr_log(GPR_INFO, "perform_transport_op[t=%p]: %s", t, msg); - gpr_free(msg); + gpr_log(GPR_INFO, "perform_transport_op[t=%p]: %s", t, + grpc_transport_op_string(op).c_str()); } op->handler_private.extra_arg = gt; GRPC_CHTTP2_REF_TRANSPORT(t, "transport_op"); diff --git a/src/core/lib/channel/channel_args.cc b/src/core/lib/channel/channel_args.cc index 10b185f5962..47cfd868a9d 100644 --- a/src/core/lib/channel/channel_args.cc +++ b/src/core/lib/channel/channel_args.cc @@ -21,6 +21,11 @@ #include #include +#include + +#include "absl/strings/str_format.h" +#include "absl/strings/str_join.h" + #include #include #include @@ -336,32 +341,28 @@ grpc_arg grpc_channel_arg_pointer_create( return arg; } -char* grpc_channel_args_string(const grpc_channel_args* args) { - if (args == nullptr) return nullptr; - gpr_strvec v; - gpr_strvec_init(&v); +std::string grpc_channel_args_string(const grpc_channel_args* args) { + if (args == nullptr) return ""; + std::vector arg_strings; for (size_t i = 0; i < args->num_args; ++i) { const grpc_arg& arg = args->args[i]; - char* s; + std::string arg_string; switch (arg.type) { case GRPC_ARG_INTEGER: - gpr_asprintf(&s, "%s=%d", arg.key, arg.value.integer); + arg_string = absl::StrFormat("%s=%d", arg.key, arg.value.integer); break; case GRPC_ARG_STRING: - gpr_asprintf(&s, "%s=%s", arg.key, arg.value.string); + arg_string = absl::StrFormat("%s=%s", arg.key, arg.value.string); break; case GRPC_ARG_POINTER: - gpr_asprintf(&s, "%s=%p", arg.key, arg.value.pointer.p); + arg_string = absl::StrFormat("%s=%p", arg.key, arg.value.pointer.p); break; default: - gpr_asprintf(&s, "arg with unknown type"); + arg_string = "arg with unknown type"; } - gpr_strvec_add(&v, s); + arg_strings.push_back(arg_string); } - char* result = - gpr_strjoin_sep(const_cast(v.strs), v.count, ", ", nullptr); - gpr_strvec_destroy(&v); - return result; + return absl::StrJoin(arg_strings, ", "); } namespace { diff --git a/src/core/lib/channel/channel_args.h b/src/core/lib/channel/channel_args.h index df105fd944f..aed00891ae2 100644 --- a/src/core/lib/channel/channel_args.h +++ b/src/core/lib/channel/channel_args.h @@ -21,6 +21,8 @@ #include +#include + #include #include "src/core/lib/surface/channel_stack_type.h" @@ -116,7 +118,7 @@ grpc_arg grpc_channel_arg_pointer_create(char* name, void* value, // Returns a string representing channel args in human-readable form. // Callers takes ownership of result. -char* grpc_channel_args_string(const grpc_channel_args* args); +std::string grpc_channel_args_string(const grpc_channel_args* args); // Takes ownership of the old_args typedef grpc_channel_args* (*grpc_channel_args_client_channel_creation_mutator)( diff --git a/src/core/lib/channel/handshaker.cc b/src/core/lib/channel/handshaker.cc index 2bcc60abfa9..461a21db32f 100644 --- a/src/core/lib/channel/handshaker.cc +++ b/src/core/lib/channel/handshaker.cc @@ -20,6 +20,8 @@ #include +#include "absl/strings/str_format.h" + #include #include #include @@ -37,19 +39,16 @@ TraceFlag grpc_handshaker_trace(false, "handshaker"); namespace { -char* HandshakerArgsString(HandshakerArgs* args) { - char* args_str = grpc_channel_args_string(args->args); +std::string HandshakerArgsString(HandshakerArgs* args) { size_t num_args = args->args != nullptr ? args->args->num_args : 0; size_t read_buffer_length = args->read_buffer != nullptr ? args->read_buffer->length : 0; - char* str; - gpr_asprintf(&str, - "{endpoint=%p, args=%p {size=%" PRIuPTR - ": %s}, read_buffer=%p (length=%" PRIuPTR "), exit_early=%d}", - args->endpoint, args->args, num_args, args_str, - args->read_buffer, read_buffer_length, args->exit_early); - gpr_free(args_str); - return str; + return absl::StrFormat( + "{endpoint=%p, args=%p {size=%" PRIuPTR + ": %s}, read_buffer=%p (length=%" PRIuPTR "), exit_early=%d}", + args->endpoint, args->args, num_args, + grpc_channel_args_string(args->args), args->read_buffer, + read_buffer_length, args->exit_early); } } // namespace @@ -127,12 +126,11 @@ void HandshakeManager::Shutdown(grpc_error* why) { // Returns true if we've scheduled the on_handshake_done callback. bool HandshakeManager::CallNextHandshakerLocked(grpc_error* error) { if (GRPC_TRACE_FLAG_ENABLED(grpc_handshaker_trace)) { - char* args_str = HandshakerArgsString(&args_); gpr_log(GPR_INFO, "handshake_manager %p: error=%s shutdown=%d index=%" PRIuPTR ", args=%s", - this, grpc_error_string(error), is_shutdown_, index_, args_str); - gpr_free(args_str); + this, grpc_error_string(error), is_shutdown_, index_, + HandshakerArgsString(&args_).c_str()); } GPR_ASSERT(index_ <= handshakers_.size()); // If we got an error or we've been shut down or we're exiting early or diff --git a/src/core/lib/debug/stats.cc b/src/core/lib/debug/stats.cc index d8ddf03ac1c..7f602b6b9a1 100644 --- a/src/core/lib/debug/stats.cc +++ b/src/core/lib/debug/stats.cc @@ -23,6 +23,11 @@ #include #include +#include + +#include "absl/strings/str_format.h" +#include "absl/strings/str_join.h" + #include #include @@ -140,39 +145,28 @@ double grpc_stats_histo_percentile(const grpc_stats_data* stats, static_cast(count) * percentile / 100.0); } -char* grpc_stats_data_as_json(const grpc_stats_data* data) { - gpr_strvec v; - char* tmp; - bool is_first = true; - gpr_strvec_init(&v); - gpr_strvec_add(&v, gpr_strdup("{")); +std::string grpc_stats_data_as_json(const grpc_stats_data* data) { + std::vector parts; + parts.push_back("{"); for (size_t i = 0; i < GRPC_STATS_COUNTER_COUNT; i++) { - gpr_asprintf(&tmp, "%s\"%s\": %" PRIdPTR, is_first ? "" : ", ", - grpc_stats_counter_name[i], data->counters[i]); - gpr_strvec_add(&v, tmp); - is_first = false; + parts.push_back(absl::StrFormat( + "\"%s\": %" PRIdPTR, grpc_stats_counter_name[i], data->counters[i])); } for (size_t i = 0; i < GRPC_STATS_HISTOGRAM_COUNT; i++) { - gpr_asprintf(&tmp, "%s\"%s\": [", is_first ? "" : ", ", - grpc_stats_histogram_name[i]); - gpr_strvec_add(&v, tmp); + parts.push_back(absl::StrFormat("\"%s\": [", grpc_stats_histogram_name[i])); for (int j = 0; j < grpc_stats_histo_buckets[i]; j++) { - gpr_asprintf(&tmp, "%s%" PRIdPTR, j == 0 ? "" : ",", - data->histograms[grpc_stats_histo_start[i] + j]); - gpr_strvec_add(&v, tmp); + parts.push_back( + absl::StrFormat("%s%" PRIdPTR, j == 0 ? "" : ",", + data->histograms[grpc_stats_histo_start[i] + j])); } - gpr_asprintf(&tmp, "], \"%s_bkt\": [", grpc_stats_histogram_name[i]); - gpr_strvec_add(&v, tmp); + parts.push_back( + absl::StrFormat("], \"%s_bkt\": [", grpc_stats_histogram_name[i])); for (int j = 0; j < grpc_stats_histo_buckets[i]; j++) { - gpr_asprintf(&tmp, "%s%d", j == 0 ? "" : ",", - grpc_stats_histo_bucket_boundaries[i][j]); - gpr_strvec_add(&v, tmp); + parts.push_back(absl::StrFormat( + "%s%d", j == 0 ? "" : ",", grpc_stats_histo_bucket_boundaries[i][j])); } - gpr_strvec_add(&v, gpr_strdup("]")); - is_first = false; + parts.push_back("]"); } - gpr_strvec_add(&v, gpr_strdup("}")); - tmp = gpr_strvec_flatten(&v, nullptr); - gpr_strvec_destroy(&v); - return tmp; + parts.push_back("}"); + return absl::StrJoin(parts, ""); } diff --git a/src/core/lib/debug/stats.h b/src/core/lib/debug/stats.h index 9e88ad7000f..3d82886bc87 100644 --- a/src/core/lib/debug/stats.h +++ b/src/core/lib/debug/stats.h @@ -21,6 +21,8 @@ #include +#include + #include #include "src/core/lib/debug/stats_data.h" #include "src/core/lib/iomgr/exec_ctx.h" @@ -56,7 +58,7 @@ void grpc_stats_collect(grpc_stats_data* output); // c = b-a void grpc_stats_diff(const grpc_stats_data* b, const grpc_stats_data* a, grpc_stats_data* c); -char* grpc_stats_data_as_json(const grpc_stats_data* data); +std::string grpc_stats_data_as_json(const grpc_stats_data* data); int grpc_stats_histo_find_bucket_slow(int value, const int* table, int table_size); double grpc_stats_histo_percentile(const grpc_stats_data* data, diff --git a/src/core/lib/gpr/string.cc b/src/core/lib/gpr/string.cc index 7ff8c9c6a41..d2c93c625b1 100644 --- a/src/core/lib/gpr/string.cc +++ b/src/core/lib/gpr/string.cc @@ -265,29 +265,6 @@ char* gpr_strjoin_sep(const char** strs, size_t nstrs, const char* sep, return out; } -void gpr_strvec_init(gpr_strvec* sv) { memset(sv, 0, sizeof(*sv)); } - -void gpr_strvec_destroy(gpr_strvec* sv) { - size_t i; - for (i = 0; i < sv->count; i++) { - gpr_free(sv->strs[i]); - } - gpr_free(sv->strs); -} - -void gpr_strvec_add(gpr_strvec* sv, char* str) { - if (sv->count == sv->capacity) { - sv->capacity = GPR_MAX(sv->capacity + 8, sv->capacity * 2); - sv->strs = static_cast( - gpr_realloc(sv->strs, sizeof(char*) * sv->capacity)); - } - sv->strs[sv->count++] = str; -} - -char* gpr_strvec_flatten(gpr_strvec* sv, size_t* final_length) { - return gpr_strjoin((const char**)sv->strs, sv->count, final_length); -} - int gpr_strincmp(const char* a, const char* b, size_t n) { int ca, cb; do { diff --git a/src/core/lib/gpr/string.h b/src/core/lib/gpr/string.h index 9e17dc180fe..b7d76986e5c 100644 --- a/src/core/lib/gpr/string.h +++ b/src/core/lib/gpr/string.h @@ -96,21 +96,6 @@ void gpr_string_split(const char* input, const char* sep, char*** strs, 0, 3, 6 or 9 fractional digits. */ char* gpr_format_timespec(gpr_timespec); -/* A vector of strings... for building up a final string one piece at a time */ -struct gpr_strvec { - char** strs; - size_t count; - size_t capacity; -}; -/* Initialize/destroy */ -void gpr_strvec_init(gpr_strvec* strs); -void gpr_strvec_destroy(gpr_strvec* strs); -/* Add a string to a strvec, takes ownership of the string */ -void gpr_strvec_add(gpr_strvec* strs, char* add); -/* Return a joined string with all added substrings, optionally setting - total_length as per gpr_strjoin */ -char* gpr_strvec_flatten(gpr_strvec* strs, size_t* total_length); - /** Case insensitive string comparison... return <0 if lower(a)0 if lower(a)>lower(b) */ int gpr_stricmp(const char* a, const char* b); diff --git a/src/core/lib/http/format_request.cc b/src/core/lib/http/format_request.cc index 17123446483..947b19cbfac 100644 --- a/src/core/lib/http/format_request.cc +++ b/src/core/lib/http/format_request.cc @@ -24,99 +24,80 @@ #include #include +#include + +#include "absl/strings/str_format.h" +#include "absl/strings/str_join.h" + #include #include #include #include "src/core/lib/gpr/string.h" static void fill_common_header(const grpc_httpcli_request* request, - gpr_strvec* buf, bool connection_close) { - size_t i; - gpr_strvec_add(buf, gpr_strdup(request->http.path)); - gpr_strvec_add(buf, gpr_strdup(" HTTP/1.0\r\n")); + bool connection_close, + std::vector* buf) { + buf->push_back(request->http.path); + buf->push_back(" HTTP/1.0\r\n"); /* just in case some crazy server really expects HTTP/1.1 */ - gpr_strvec_add(buf, gpr_strdup("Host: ")); - gpr_strvec_add(buf, gpr_strdup(request->host)); - gpr_strvec_add(buf, gpr_strdup("\r\n")); - if (connection_close) - gpr_strvec_add(buf, gpr_strdup("Connection: close\r\n")); - gpr_strvec_add(buf, - gpr_strdup("User-Agent: " GRPC_HTTPCLI_USER_AGENT "\r\n")); + buf->push_back("Host: "); + buf->push_back(request->host); + buf->push_back("\r\n"); + if (connection_close) buf->push_back("Connection: close\r\n"); + buf->push_back("User-Agent: " GRPC_HTTPCLI_USER_AGENT "\r\n"); /* user supplied headers */ - for (i = 0; i < request->http.hdr_count; i++) { - gpr_strvec_add(buf, gpr_strdup(request->http.hdrs[i].key)); - gpr_strvec_add(buf, gpr_strdup(": ")); - gpr_strvec_add(buf, gpr_strdup(request->http.hdrs[i].value)); - gpr_strvec_add(buf, gpr_strdup("\r\n")); + for (size_t i = 0; i < request->http.hdr_count; i++) { + buf->push_back(request->http.hdrs[i].key); + buf->push_back(": "); + buf->push_back(request->http.hdrs[i].value); + buf->push_back("\r\n"); } } grpc_slice grpc_httpcli_format_get_request( const grpc_httpcli_request* request) { - gpr_strvec out; - char* flat; - size_t flat_len; - - gpr_strvec_init(&out); - gpr_strvec_add(&out, gpr_strdup("GET ")); - fill_common_header(request, &out, true); - gpr_strvec_add(&out, gpr_strdup("\r\n")); - - flat = gpr_strvec_flatten(&out, &flat_len); - gpr_strvec_destroy(&out); - - return grpc_slice_new(flat, flat_len, gpr_free); + std::vector out; + out.push_back("GET "); + fill_common_header(request, true, &out); + out.push_back("\r\n"); + std::string req = absl::StrJoin(out, ""); + return grpc_slice_from_copied_buffer(req.data(), req.size()); } grpc_slice grpc_httpcli_format_post_request(const grpc_httpcli_request* request, const char* body_bytes, size_t body_size) { - gpr_strvec out; - char* tmp; - size_t out_len; - size_t i; - - gpr_strvec_init(&out); - - gpr_strvec_add(&out, gpr_strdup("POST ")); - fill_common_header(request, &out, true); - if (body_bytes) { - uint8_t has_content_type = 0; - for (i = 0; i < request->http.hdr_count; i++) { + std::vector out; + out.push_back("POST "); + fill_common_header(request, true, &out); + if (body_bytes != nullptr) { + bool has_content_type = false; + for (size_t i = 0; i < request->http.hdr_count; i++) { if (strcmp(request->http.hdrs[i].key, "Content-Type") == 0) { - has_content_type = 1; + has_content_type = true; break; } } if (!has_content_type) { - gpr_strvec_add(&out, gpr_strdup("Content-Type: text/plain\r\n")); + out.push_back("Content-Type: text/plain\r\n"); } - gpr_asprintf(&tmp, "Content-Length: %lu\r\n", - static_cast(body_size)); - gpr_strvec_add(&out, tmp); + out.push_back(absl::StrFormat("Content-Length: %lu\r\n", + static_cast(body_size))); } - gpr_strvec_add(&out, gpr_strdup("\r\n")); - tmp = gpr_strvec_flatten(&out, &out_len); - gpr_strvec_destroy(&out); - - if (body_bytes) { - tmp = static_cast(gpr_realloc(tmp, out_len + body_size)); - memcpy(tmp + out_len, body_bytes, body_size); - out_len += body_size; + out.push_back("\r\n"); + std::string req = absl::StrJoin(out, ""); + if (body_bytes != nullptr) { + absl::StrAppend(&req, absl::string_view(body_bytes, body_size)); } - - return grpc_slice_new(tmp, out_len, gpr_free); + return grpc_slice_from_copied_buffer(req.data(), req.size()); } grpc_slice grpc_httpcli_format_connect_request( const grpc_httpcli_request* request) { - gpr_strvec out; - gpr_strvec_init(&out); - gpr_strvec_add(&out, gpr_strdup("CONNECT ")); - fill_common_header(request, &out, false); - gpr_strvec_add(&out, gpr_strdup("\r\n")); - size_t flat_len; - char* flat = gpr_strvec_flatten(&out, &flat_len); - gpr_strvec_destroy(&out); - return grpc_slice_new(flat, flat_len, gpr_free); + std::vector out; + out.push_back("CONNECT "); + fill_common_header(request, false, &out); + out.push_back("\r\n"); + std::string req = absl::StrJoin(out, ""); + return grpc_slice_from_copied_buffer(req.data(), req.size()); } diff --git a/src/core/lib/iomgr/ev_epoll1_linux.cc b/src/core/lib/iomgr/ev_epoll1_linux.cc index 392d38b5d17..04955e1505b 100644 --- a/src/core/lib/iomgr/ev_epoll1_linux.cc +++ b/src/core/lib/iomgr/ev_epoll1_linux.cc @@ -38,6 +38,11 @@ #include #include +#include + +#include "absl/strings/str_format.h" +#include "absl/strings/str_join.h" + #include #include #include @@ -1064,30 +1069,23 @@ static grpc_error* pollset_kick(grpc_pollset* pollset, GRPC_STATS_INC_POLLSET_KICK(); grpc_error* ret_err = GRPC_ERROR_NONE; if (GRPC_TRACE_FLAG_ENABLED(grpc_polling_trace)) { - gpr_strvec log; - gpr_strvec_init(&log); - char* tmp; - gpr_asprintf(&tmp, "PS:%p KICK:%p curps=%p curworker=%p root=%p", pollset, - specific_worker, (void*)gpr_tls_get(&g_current_thread_pollset), - (void*)gpr_tls_get(&g_current_thread_worker), - pollset->root_worker); - gpr_strvec_add(&log, tmp); + std::vector log; + log.push_back(absl::StrFormat( + "PS:%p KICK:%p curps=%p curworker=%p root=%p", pollset, specific_worker, + (void*)gpr_tls_get(&g_current_thread_pollset), + (void*)gpr_tls_get(&g_current_thread_worker), pollset->root_worker)); if (pollset->root_worker != nullptr) { - gpr_asprintf(&tmp, " {kick_state=%s next=%p {kick_state=%s}}", - kick_state_string(pollset->root_worker->state), - pollset->root_worker->next, - kick_state_string(pollset->root_worker->next->state)); - gpr_strvec_add(&log, tmp); + log.push_back(absl::StrFormat( + " {kick_state=%s next=%p {kick_state=%s}}", + kick_state_string(pollset->root_worker->state), + pollset->root_worker->next, + kick_state_string(pollset->root_worker->next->state))); } if (specific_worker != nullptr) { - gpr_asprintf(&tmp, " worker_kick_state=%s", - kick_state_string(specific_worker->state)); - gpr_strvec_add(&log, tmp); + log.push_back(absl::StrFormat(" worker_kick_state=%s", + kick_state_string(specific_worker->state))); } - tmp = gpr_strvec_flatten(&log, nullptr); - gpr_strvec_destroy(&log); - gpr_log(GPR_DEBUG, "%s", tmp); - gpr_free(tmp); + gpr_log(GPR_DEBUG, "%s", absl::StrJoin(log, "").c_str()); } if (specific_worker == nullptr) { diff --git a/src/core/lib/security/credentials/oauth2/oauth2_credentials.cc b/src/core/lib/security/credentials/oauth2/oauth2_credentials.cc index 8191043db21..550aa0cb544 100644 --- a/src/core/lib/security/credentials/oauth2/oauth2_credentials.cc +++ b/src/core/lib/security/credentials/oauth2/oauth2_credentials.cc @@ -24,6 +24,7 @@ #include #include "absl/container/inlined_vector.h" +#include "absl/strings/str_join.h" #include #include @@ -523,12 +524,10 @@ namespace grpc_core { namespace { -void MaybeAddToBody(gpr_strvec* body_strvec, const char* field_name, - const char* field) { +void MaybeAddToBody(const char* field_name, const char* field, + std::vector* body) { if (field == nullptr || strlen(field) == 0) return; - char* new_query; - gpr_asprintf(&new_query, "&%s=%s", field_name, field); - gpr_strvec_add(body_strvec, new_query); + body->push_back(absl::StrFormat("&%s=%s", field_name, field)); } grpc_error* LoadTokenFile(const char* path, gpr_slice* token) { @@ -608,20 +607,18 @@ class StsTokenFetcherCredentials grpc_error* FillBody(char** body, size_t* body_length) { *body = nullptr; - gpr_strvec body_strvec; - gpr_strvec_init(&body_strvec); + std::vector body_parts; grpc_slice subject_token = grpc_empty_slice(); grpc_slice actor_token = grpc_empty_slice(); grpc_error* err = GRPC_ERROR_NONE; - auto cleanup = [&body, &body_length, &body_strvec, &subject_token, + auto cleanup = [&body, &body_length, &body_parts, &subject_token, &actor_token, &err]() { if (err == GRPC_ERROR_NONE) { - *body = gpr_strvec_flatten(&body_strvec, body_length); - } else { - gpr_free(*body); + std::string body_str = absl::StrJoin(body_parts, ""); + *body = gpr_strdup(body_str.c_str()); + *body_length = body_str.size(); } - gpr_strvec_destroy(&body_strvec); grpc_slice_unref_internal(subject_token); grpc_slice_unref_internal(actor_token); return err; @@ -629,23 +626,23 @@ class StsTokenFetcherCredentials err = LoadTokenFile(subject_token_path_.get(), &subject_token); if (err != GRPC_ERROR_NONE) return cleanup(); - gpr_asprintf( - body, GRPC_STS_POST_MINIMAL_BODY_FORMAT_STRING, + body_parts.push_back(absl::StrFormat( + GRPC_STS_POST_MINIMAL_BODY_FORMAT_STRING, reinterpret_cast(GRPC_SLICE_START_PTR(subject_token)), - subject_token_type_.get()); - gpr_strvec_add(&body_strvec, *body); - MaybeAddToBody(&body_strvec, "resource", resource_.get()); - MaybeAddToBody(&body_strvec, "audience", audience_.get()); - MaybeAddToBody(&body_strvec, "scope", scope_.get()); - MaybeAddToBody(&body_strvec, "requested_token_type", - requested_token_type_.get()); + subject_token_type_.get())); + MaybeAddToBody("resource", resource_.get(), &body_parts); + MaybeAddToBody("audience", audience_.get(), &body_parts); + MaybeAddToBody("scope", scope_.get(), &body_parts); + MaybeAddToBody("requested_token_type", requested_token_type_.get(), + &body_parts); if ((actor_token_path_ != nullptr) && *actor_token_path_ != '\0') { err = LoadTokenFile(actor_token_path_.get(), &actor_token); if (err != GRPC_ERROR_NONE) return cleanup(); MaybeAddToBody( - &body_strvec, "actor_token", - reinterpret_cast(GRPC_SLICE_START_PTR(actor_token))); - MaybeAddToBody(&body_strvec, "actor_token_type", actor_token_type_.get()); + "actor_token", + reinterpret_cast(GRPC_SLICE_START_PTR(actor_token)), + &body_parts); + MaybeAddToBody("actor_token_type", actor_token_type_.get(), &body_parts); } return cleanup(); } diff --git a/src/core/lib/surface/call_log_batch.cc b/src/core/lib/surface/call_log_batch.cc index 23c344a2c0e..eac55137183 100644 --- a/src/core/lib/surface/call_log_batch.cc +++ b/src/core/lib/surface/call_log_batch.cc @@ -22,98 +22,90 @@ #include +#include + +#include "absl/strings/str_format.h" +#include "absl/strings/str_join.h" + #include #include #include "src/core/lib/gpr/string.h" #include "src/core/lib/slice/slice_string_helpers.h" -static void add_metadata(gpr_strvec* b, const grpc_metadata* md, size_t count) { - size_t i; +static void add_metadata(const grpc_metadata* md, size_t count, + std::vector* b) { if (md == nullptr) { - gpr_strvec_add(b, gpr_strdup("(nil)")); + b->push_back("(nil)"); return; } - for (i = 0; i < count; i++) { - gpr_strvec_add(b, gpr_strdup("\nkey=")); - gpr_strvec_add(b, grpc_slice_to_c_string(md[i].key)); - - gpr_strvec_add(b, gpr_strdup(" value=")); - gpr_strvec_add(b, - grpc_dump_slice(md[i].value, GPR_DUMP_HEX | GPR_DUMP_ASCII)); + for (size_t i = 0; i < count; i++) { + b->push_back("\nkey="); + b->push_back(std::string(grpc_core::StringViewFromSlice(md[i].key))); + b->push_back(" value="); + char* dump = grpc_dump_slice(md[i].value, GPR_DUMP_HEX | GPR_DUMP_ASCII); + b->push_back(dump); + gpr_free(dump); } } -char* grpc_op_string(const grpc_op* op) { - char* tmp; - char* out; - - gpr_strvec b; - gpr_strvec_init(&b); - +static std::string grpc_op_string(const grpc_op* op) { + std::vector parts; switch (op->op) { case GRPC_OP_SEND_INITIAL_METADATA: - gpr_strvec_add(&b, gpr_strdup("SEND_INITIAL_METADATA")); - add_metadata(&b, op->data.send_initial_metadata.metadata, - op->data.send_initial_metadata.count); + parts.push_back("SEND_INITIAL_METADATA"); + add_metadata(op->data.send_initial_metadata.metadata, + op->data.send_initial_metadata.count, &parts); break; case GRPC_OP_SEND_MESSAGE: - gpr_asprintf(&tmp, "SEND_MESSAGE ptr=%p", - op->data.send_message.send_message); - gpr_strvec_add(&b, tmp); + parts.push_back(absl::StrFormat("SEND_MESSAGE ptr=%p", + op->data.send_message.send_message)); break; case GRPC_OP_SEND_CLOSE_FROM_CLIENT: - gpr_strvec_add(&b, gpr_strdup("SEND_CLOSE_FROM_CLIENT")); + parts.push_back("SEND_CLOSE_FROM_CLIENT"); break; case GRPC_OP_SEND_STATUS_FROM_SERVER: - gpr_asprintf(&tmp, "SEND_STATUS_FROM_SERVER status=%d details=", - op->data.send_status_from_server.status); - gpr_strvec_add(&b, tmp); + parts.push_back( + absl::StrFormat("SEND_STATUS_FROM_SERVER status=%d details=", + op->data.send_status_from_server.status)); if (op->data.send_status_from_server.status_details != nullptr) { - gpr_strvec_add(&b, grpc_dump_slice( - *op->data.send_status_from_server.status_details, - GPR_DUMP_ASCII)); + char* dump = grpc_dump_slice( + *op->data.send_status_from_server.status_details, GPR_DUMP_ASCII); + parts.push_back(dump); + gpr_free(dump); } else { - gpr_strvec_add(&b, gpr_strdup("(null)")); + parts.push_back("(null)"); } - add_metadata(&b, op->data.send_status_from_server.trailing_metadata, - op->data.send_status_from_server.trailing_metadata_count); + add_metadata(op->data.send_status_from_server.trailing_metadata, + op->data.send_status_from_server.trailing_metadata_count, + &parts); break; case GRPC_OP_RECV_INITIAL_METADATA: - gpr_asprintf(&tmp, "RECV_INITIAL_METADATA ptr=%p", - op->data.recv_initial_metadata.recv_initial_metadata); - gpr_strvec_add(&b, tmp); + parts.push_back(absl::StrFormat( + "RECV_INITIAL_METADATA ptr=%p", + op->data.recv_initial_metadata.recv_initial_metadata)); break; case GRPC_OP_RECV_MESSAGE: - gpr_asprintf(&tmp, "RECV_MESSAGE ptr=%p", - op->data.recv_message.recv_message); - gpr_strvec_add(&b, tmp); + parts.push_back(absl::StrFormat("RECV_MESSAGE ptr=%p", + op->data.recv_message.recv_message)); break; case GRPC_OP_RECV_STATUS_ON_CLIENT: - gpr_asprintf(&tmp, - "RECV_STATUS_ON_CLIENT metadata=%p status=%p details=%p", - op->data.recv_status_on_client.trailing_metadata, - op->data.recv_status_on_client.status, - op->data.recv_status_on_client.status_details); - gpr_strvec_add(&b, tmp); + parts.push_back(absl::StrFormat( + "RECV_STATUS_ON_CLIENT metadata=%p status=%p details=%p", + op->data.recv_status_on_client.trailing_metadata, + op->data.recv_status_on_client.status, + op->data.recv_status_on_client.status_details)); break; case GRPC_OP_RECV_CLOSE_ON_SERVER: - gpr_asprintf(&tmp, "RECV_CLOSE_ON_SERVER cancelled=%p", - op->data.recv_close_on_server.cancelled); - gpr_strvec_add(&b, tmp); + parts.push_back(absl::StrFormat("RECV_CLOSE_ON_SERVER cancelled=%p", + op->data.recv_close_on_server.cancelled)); } - out = gpr_strvec_flatten(&b, nullptr); - gpr_strvec_destroy(&b); - - return out; + return absl::StrJoin(parts, ""); } void grpc_call_log_batch(const char* file, int line, gpr_log_severity severity, const grpc_op* ops, size_t nops) { - char* tmp; - size_t i; - for (i = 0; i < nops; i++) { - tmp = grpc_op_string(&ops[i]); - gpr_log(file, line, severity, "ops[%" PRIuPTR "]: %s", i, tmp); - gpr_free(tmp); + for (size_t i = 0; i < nops; i++) { + gpr_log(file, line, severity, "ops[%" PRIuPTR "]: %s", i, + grpc_op_string(&ops[i]).c_str()); } } diff --git a/src/core/lib/surface/completion_queue.cc b/src/core/lib/surface/completion_queue.cc index ed9836fb106..a383f78690a 100644 --- a/src/core/lib/surface/completion_queue.cc +++ b/src/core/lib/surface/completion_queue.cc @@ -23,6 +23,11 @@ #include #include +#include + +#include "absl/strings/str_format.h" +#include "absl/strings/str_join.h" + #include #include #include @@ -427,15 +432,14 @@ static const cq_vtable g_cq_vtable[] = { grpc_core::TraceFlag grpc_cq_pluck_trace(false, "queue_pluck"); -#define GRPC_SURFACE_TRACE_RETURNED_EVENT(cq, event) \ - do { \ - if (GRPC_TRACE_FLAG_ENABLED(grpc_api_trace) && \ - (GRPC_TRACE_FLAG_ENABLED(grpc_cq_pluck_trace) || \ - (event)->type != GRPC_QUEUE_TIMEOUT)) { \ - char* _ev = grpc_event_string(event); \ - gpr_log(GPR_INFO, "RETURN_EVENT[%p]: %s", cq, _ev); \ - gpr_free(_ev); \ - } \ +#define GRPC_SURFACE_TRACE_RETURNED_EVENT(cq, event) \ + do { \ + if (GRPC_TRACE_FLAG_ENABLED(grpc_api_trace) && \ + (GRPC_TRACE_FLAG_ENABLED(grpc_cq_pluck_trace) || \ + (event)->type != GRPC_QUEUE_TIMEOUT)) { \ + gpr_log(GPR_INFO, "RETURN_EVENT[%p]: %s", cq, \ + grpc_event_string(event).c_str()); \ + } \ } while (0) static void on_pollset_shutdown_done(void* cq, grpc_error* error); @@ -947,21 +951,14 @@ class ExecCtxNext : public grpc_core::ExecCtx { #ifndef NDEBUG static void dump_pending_tags(grpc_completion_queue* cq) { if (!GRPC_TRACE_FLAG_ENABLED(grpc_trace_pending_tags)) return; - - gpr_strvec v; - gpr_strvec_init(&v); - gpr_strvec_add(&v, gpr_strdup("PENDING TAGS:")); + std::vector parts; + parts.push_back("PENDING TAGS:"); gpr_mu_lock(cq->mu); for (size_t i = 0; i < cq->outstanding_tag_count; i++) { - char* s; - gpr_asprintf(&s, " %p", cq->outstanding_tags[i]); - gpr_strvec_add(&v, s); + parts.push_back(absl::StrFormat(" %p", cq->outstanding_tags[i])); } gpr_mu_unlock(cq->mu); - char* out = gpr_strvec_flatten(&v, nullptr); - gpr_strvec_destroy(&v); - gpr_log(GPR_DEBUG, "%s", out); - gpr_free(out); + gpr_log(GPR_DEBUG, "%s", absl::StrJoin(parts, "").c_str()); } #else static void dump_pending_tags(grpc_completion_queue* /*cq*/) {} diff --git a/src/core/lib/surface/event_string.cc b/src/core/lib/surface/event_string.cc index d639baec455..32d85f7b054 100644 --- a/src/core/lib/surface/event_string.cc +++ b/src/core/lib/surface/event_string.cc @@ -22,47 +22,40 @@ #include +#include + +#include "absl/strings/str_format.h" +#include "absl/strings/str_join.h" + #include #include #include "src/core/lib/gpr/string.h" -static void addhdr(gpr_strvec* buf, grpc_event* ev) { - char* tmp; - gpr_asprintf(&tmp, "tag:%p", ev->tag); - gpr_strvec_add(buf, tmp); +static void addhdr(grpc_event* ev, std::vector* buf) { + buf->push_back(absl::StrFormat("tag:%p", ev->tag)); } static const char* errstr(int success) { return success ? "OK" : "ERROR"; } -static void adderr(gpr_strvec* buf, int success) { - char* tmp; - gpr_asprintf(&tmp, " %s", errstr(success)); - gpr_strvec_add(buf, tmp); +static void adderr(int success, std::vector* buf) { + buf->push_back(absl::StrFormat(" %s", errstr(success))); } -char* grpc_event_string(grpc_event* ev) { - char* out; - gpr_strvec buf; - - if (ev == nullptr) return gpr_strdup("null"); - - gpr_strvec_init(&buf); - +std::string grpc_event_string(grpc_event* ev) { + if (ev == nullptr) return "null"; + std::vector out; switch (ev->type) { case GRPC_QUEUE_TIMEOUT: - gpr_strvec_add(&buf, gpr_strdup("QUEUE_TIMEOUT")); + out.push_back("QUEUE_TIMEOUT"); break; case GRPC_QUEUE_SHUTDOWN: - gpr_strvec_add(&buf, gpr_strdup("QUEUE_SHUTDOWN")); + out.push_back("QUEUE_SHUTDOWN"); break; case GRPC_OP_COMPLETE: - gpr_strvec_add(&buf, gpr_strdup("OP_COMPLETE: ")); - addhdr(&buf, ev); - adderr(&buf, ev->success); + out.push_back("OP_COMPLETE: "); + addhdr(ev, &out); + adderr(ev->success, &out); break; } - - out = gpr_strvec_flatten(&buf, nullptr); - gpr_strvec_destroy(&buf); - return out; + return absl::StrJoin(out, ""); } diff --git a/src/core/lib/surface/event_string.h b/src/core/lib/surface/event_string.h index e6095705e9b..9e02a52e5cb 100644 --- a/src/core/lib/surface/event_string.h +++ b/src/core/lib/surface/event_string.h @@ -21,9 +21,11 @@ #include +#include + #include /* Returns a string describing an event. Must be later freed with gpr_free() */ -char* grpc_event_string(grpc_event* ev); +std::string grpc_event_string(grpc_event* ev); #endif /* GRPC_CORE_LIB_SURFACE_EVENT_STRING_H */ diff --git a/src/core/lib/transport/transport.h b/src/core/lib/transport/transport.h index 18374511cde..ab512896404 100644 --- a/src/core/lib/transport/transport.h +++ b/src/core/lib/transport/transport.h @@ -408,8 +408,9 @@ void grpc_transport_stream_op_batch_finish_with_failure( grpc_transport_stream_op_batch* op, grpc_error* error, grpc_core::CallCombiner* call_combiner); -char* grpc_transport_stream_op_batch_string(grpc_transport_stream_op_batch* op); -char* grpc_transport_op_string(grpc_transport_op* op); +std::string grpc_transport_stream_op_batch_string( + grpc_transport_stream_op_batch* op); +std::string grpc_transport_op_string(grpc_transport_op* op); /* Send a batch of operations on a transport diff --git a/src/core/lib/transport/transport_op_string.cc b/src/core/lib/transport/transport_op_string.cc index 34b36c3aec9..722a64ecb8d 100644 --- a/src/core/lib/transport/transport_op_string.cc +++ b/src/core/lib/transport/transport_op_string.cc @@ -25,6 +25,12 @@ #include #include +#include + +#include "absl/strings/str_cat.h" +#include "absl/strings/str_format.h" +#include "absl/strings/str_join.h" + #include #include #include "src/core/lib/gpr/string.h" @@ -34,177 +40,130 @@ /* These routines are here to facilitate debugging - they produce string representations of various transport data structures */ -static void put_metadata(gpr_strvec* b, grpc_mdelem md) { - gpr_strvec_add(b, gpr_strdup("key=")); - gpr_strvec_add( - b, grpc_dump_slice(GRPC_MDKEY(md), GPR_DUMP_HEX | GPR_DUMP_ASCII)); - - gpr_strvec_add(b, gpr_strdup(" value=")); - gpr_strvec_add( - b, grpc_dump_slice(GRPC_MDVALUE(md), GPR_DUMP_HEX | GPR_DUMP_ASCII)); +static void put_metadata(grpc_mdelem md, std::vector* out) { + out->push_back("key="); + char* dump = grpc_dump_slice(GRPC_MDKEY(md), GPR_DUMP_HEX | GPR_DUMP_ASCII); + out->push_back(dump); + gpr_free(dump); + out->push_back(" value="); + dump = grpc_dump_slice(GRPC_MDVALUE(md), GPR_DUMP_HEX | GPR_DUMP_ASCII); + out->push_back(dump); + gpr_free(dump); } -static void put_metadata_list(gpr_strvec* b, grpc_metadata_batch md) { +static void put_metadata_list(grpc_metadata_batch md, + std::vector* out) { grpc_linked_mdelem* m; for (m = md.list.head; m != nullptr; m = m->next) { - if (m != md.list.head) gpr_strvec_add(b, gpr_strdup(", ")); - put_metadata(b, m->md); + if (m != md.list.head) out->push_back(", "); + put_metadata(m->md, out); } if (md.deadline != GRPC_MILLIS_INF_FUTURE) { - char* tmp; - gpr_asprintf(&tmp, " deadline=%" PRId64, md.deadline); - gpr_strvec_add(b, tmp); + out->push_back(absl::StrFormat(" deadline=%" PRId64, md.deadline)); } } -char* grpc_transport_stream_op_batch_string( +std::string grpc_transport_stream_op_batch_string( grpc_transport_stream_op_batch* op) { - char* tmp; - char* out; - - gpr_strvec b; - gpr_strvec_init(&b); + std::vector out; if (op->send_initial_metadata) { - gpr_strvec_add(&b, gpr_strdup(" ")); - gpr_strvec_add(&b, gpr_strdup("SEND_INITIAL_METADATA{")); - put_metadata_list( - &b, *op->payload->send_initial_metadata.send_initial_metadata); - gpr_strvec_add(&b, gpr_strdup("}")); + out.push_back(" SEND_INITIAL_METADATA{"); + put_metadata_list(*op->payload->send_initial_metadata.send_initial_metadata, + &out); + out.push_back("}"); } if (op->send_message) { - gpr_strvec_add(&b, gpr_strdup(" ")); if (op->payload->send_message.send_message != nullptr) { - gpr_asprintf(&tmp, "SEND_MESSAGE:flags=0x%08x:len=%d", - op->payload->send_message.send_message->flags(), - op->payload->send_message.send_message->length()); + out.push_back( + absl::StrFormat(" SEND_MESSAGE:flags=0x%08x:len=%d", + op->payload->send_message.send_message->flags(), + op->payload->send_message.send_message->length())); } else { // This can happen when we check a batch after the transport has // processed and cleared the send_message op. - tmp = - gpr_strdup("SEND_MESSAGE(flag and length unknown, already orphaned)"); + out.push_back(" SEND_MESSAGE(flag and length unknown, already orphaned)"); } - gpr_strvec_add(&b, tmp); } if (op->send_trailing_metadata) { - gpr_strvec_add(&b, gpr_strdup(" ")); - gpr_strvec_add(&b, gpr_strdup("SEND_TRAILING_METADATA{")); + out.push_back(" SEND_TRAILING_METADATA{"); put_metadata_list( - &b, *op->payload->send_trailing_metadata.send_trailing_metadata); - gpr_strvec_add(&b, gpr_strdup("}")); + *op->payload->send_trailing_metadata.send_trailing_metadata, &out); + out.push_back("}"); } if (op->recv_initial_metadata) { - gpr_strvec_add(&b, gpr_strdup(" ")); - gpr_strvec_add(&b, gpr_strdup("RECV_INITIAL_METADATA")); + out.push_back(" RECV_INITIAL_METADATA"); } if (op->recv_message) { - gpr_strvec_add(&b, gpr_strdup(" ")); - gpr_strvec_add(&b, gpr_strdup("RECV_MESSAGE")); + out.push_back(" RECV_MESSAGE"); } if (op->recv_trailing_metadata) { - gpr_strvec_add(&b, gpr_strdup(" ")); - gpr_strvec_add(&b, gpr_strdup("RECV_TRAILING_METADATA")); + out.push_back(" RECV_TRAILING_METADATA"); } if (op->cancel_stream) { - gpr_strvec_add(&b, gpr_strdup(" ")); - const char* msg = - grpc_error_string(op->payload->cancel_stream.cancel_error); - gpr_asprintf(&tmp, "CANCEL:%s", msg); - - gpr_strvec_add(&b, tmp); + out.push_back(absl::StrCat( + " CANCEL:", + grpc_error_string(op->payload->cancel_stream.cancel_error))); } - out = gpr_strvec_flatten(&b, nullptr); - gpr_strvec_destroy(&b); - - return out; + return absl::StrJoin(out, ""); } -char* grpc_transport_op_string(grpc_transport_op* op) { - char* tmp; - char* out; - bool first = true; - - gpr_strvec b; - gpr_strvec_init(&b); +std::string grpc_transport_op_string(grpc_transport_op* op) { + std::vector out; if (op->start_connectivity_watch != nullptr) { - if (!first) gpr_strvec_add(&b, gpr_strdup(" ")); - first = false; - gpr_asprintf( - &tmp, "START_CONNECTIVITY_WATCH:watcher=%p:from=%s", + out.push_back(absl::StrFormat( + " START_CONNECTIVITY_WATCH:watcher=%p:from=%s", op->start_connectivity_watch.get(), - grpc_core::ConnectivityStateName(op->start_connectivity_watch_state)); - gpr_strvec_add(&b, tmp); + grpc_core::ConnectivityStateName(op->start_connectivity_watch_state))); } if (op->stop_connectivity_watch != nullptr) { - if (!first) gpr_strvec_add(&b, gpr_strdup(" ")); - first = false; - gpr_asprintf(&tmp, "STOP_CONNECTIVITY_WATCH:watcher=%p", - op->stop_connectivity_watch); - gpr_strvec_add(&b, tmp); + out.push_back(absl::StrFormat(" STOP_CONNECTIVITY_WATCH:watcher=%p", + op->stop_connectivity_watch)); } if (op->disconnect_with_error != GRPC_ERROR_NONE) { - if (!first) gpr_strvec_add(&b, gpr_strdup(" ")); - first = false; - const char* err = grpc_error_string(op->disconnect_with_error); - gpr_asprintf(&tmp, "DISCONNECT:%s", err); - gpr_strvec_add(&b, tmp); + out.push_back(absl::StrCat(" DISCONNECT:", + grpc_error_string(op->disconnect_with_error))); } if (op->goaway_error) { - if (!first) gpr_strvec_add(&b, gpr_strdup(" ")); - first = false; - const char* msg = grpc_error_string(op->goaway_error); - gpr_asprintf(&tmp, "SEND_GOAWAY:%s", msg); - - gpr_strvec_add(&b, tmp); + out.push_back( + absl::StrCat(" SEND_GOAWAY:%s", grpc_error_string(op->goaway_error))); } if (op->set_accept_stream) { - if (!first) gpr_strvec_add(&b, gpr_strdup(" ")); - first = false; - gpr_asprintf(&tmp, "SET_ACCEPT_STREAM:%p(%p,...)", op->set_accept_stream_fn, - op->set_accept_stream_user_data); - gpr_strvec_add(&b, tmp); + out.push_back(absl::StrFormat(" SET_ACCEPT_STREAM:%p(%p,...)", + op->set_accept_stream_fn, + op->set_accept_stream_user_data)); } if (op->bind_pollset != nullptr) { - if (!first) gpr_strvec_add(&b, gpr_strdup(" ")); - first = false; - gpr_strvec_add(&b, gpr_strdup("BIND_POLLSET")); + out.push_back(" BIND_POLLSET"); } if (op->bind_pollset_set != nullptr) { - if (!first) gpr_strvec_add(&b, gpr_strdup(" ")); - first = false; - gpr_strvec_add(&b, gpr_strdup("BIND_POLLSET_SET")); + out.push_back(" BIND_POLLSET_SET"); } if (op->send_ping.on_initiate != nullptr || op->send_ping.on_ack != nullptr) { - if (!first) gpr_strvec_add(&b, gpr_strdup(" ")); - // first = false; - gpr_strvec_add(&b, gpr_strdup("SEND_PING")); + out.push_back(" SEND_PING"); } - out = gpr_strvec_flatten(&b, nullptr); - gpr_strvec_destroy(&b); - - return out; + return absl::StrJoin(out, ""); } void grpc_call_log_op(const char* file, int line, gpr_log_severity severity, grpc_call_element* elem, grpc_transport_stream_op_batch* op) { - char* str = grpc_transport_stream_op_batch_string(op); - gpr_log(file, line, severity, "OP[%s:%p]: %s", elem->filter->name, elem, str); - gpr_free(str); + gpr_log(file, line, severity, "OP[%s:%p]: %s", elem->filter->name, elem, + grpc_transport_stream_op_batch_string(op).c_str()); } diff --git a/src/php/README.md b/src/php/README.md index 84ee8f7a2e9..9aa66896684 100644 --- a/src/php/README.md +++ b/src/php/README.md @@ -365,4 +365,30 @@ $client = new Helloworld\GreeterClient('localhost:50051', [ ]); ``` +### Compression + +You can customize the compression behavior on the client side, by specifying the following options when constructing your PHP client. + +``` +Possible values for grpc.default_compression_algorithm: +0 - No compression +1 - Compress with DEFLATE algorithm +2 - Compress with GZIP algorithm +3 - Stream compression with GZIP algorithm +``` +``` +Possible values for grpc.default_compression_level: +0 - None +1 - Low level +2 - Medium level +3 - High level +``` +Here's an example on how you can put them all together: +``` +$client = new Helloworld\GreeterClient('localhost:50051', [ + 'credentials' => Grpc\ChannelCredentials::createInsecure(), + 'grpc.default_compression_algorithm' => 2, + 'grpc.default_compression_level' => 2, +]); + [Node]:https://github.com/grpc/grpc/tree/master/src/node/examples diff --git a/src/php/lib/Grpc/AbstractCall.php b/src/php/lib/Grpc/AbstractCall.php index d1c75cb2029..d1a186add78 100644 --- a/src/php/lib/Grpc/AbstractCall.php +++ b/src/php/lib/Grpc/AbstractCall.php @@ -114,14 +114,7 @@ abstract class AbstractCall protected function _serializeMessage($data) { // Proto3 implementation - if (method_exists($data, 'encode')) { - return $data->encode(); - } elseif (method_exists($data, 'serializeToString')) { - return $data->serializeToString(); - } - - // Protobuf-PHP implementation - return $data->serialize(); + return $data->serializeToString(); } /** @@ -136,22 +129,10 @@ abstract class AbstractCall if ($value === null) { return; } - - // Proto3 implementation - if (is_array($this->deserialize)) { - list($className, $deserializeFunc) = $this->deserialize; - $obj = new $className(); - if (method_exists($obj, $deserializeFunc)) { - $obj->$deserializeFunc($value); - } else { - $obj->mergeFromString($value); - } - - return $obj; - } - - // Protobuf-PHP implementation - return call_user_func($this->deserialize, $value); + list($className, $deserializeFunc) = $this->deserialize; + $obj = new $className(); + $obj->mergeFromString($value); + return $obj; } /** diff --git a/src/python/grpcio/grpc/_cython/_cygrpc/iomgr.pxd.pxi b/src/python/grpcio/grpc/_cython/_cygrpc/iomgr.pxd.pxi index 3b64b284aa9..0c5a4e5763d 100644 --- a/src/python/grpcio/grpc/_cython/_cygrpc/iomgr.pxd.pxi +++ b/src/python/grpcio/grpc/_cython/_cygrpc/iomgr.pxd.pxi @@ -122,7 +122,7 @@ cdef extern from "src/core/lib/iomgr/iomgr_custom.h": cdef extern from "src/core/lib/iomgr/sockaddr_utils.h": int grpc_sockaddr_get_port(const grpc_resolved_address *addr); cppstring grpc_sockaddr_to_string(const grpc_resolved_address *addr, - bool_t normalize); + bool_t normalize); void grpc_string_to_sockaddr(grpc_resolved_address *out, char* addr, int port); int grpc_sockaddr_set_port(const grpc_resolved_address *resolved_addr, int port) diff --git a/src/python/grpcio/grpc/experimental/aio/__init__.py b/src/python/grpcio/grpc/experimental/aio/__init__.py index c0fc17eeb92..2933aa5a45e 100644 --- a/src/python/grpcio/grpc/experimental/aio/__init__.py +++ b/src/python/grpcio/grpc/experimental/aio/__init__.py @@ -34,7 +34,8 @@ from ._interceptor import (ClientCallDetails, ClientInterceptor, InterceptedUnaryUnaryCall, UnaryUnaryClientInterceptor, UnaryStreamClientInterceptor, - StreamUnaryClientInterceptor, ServerInterceptor) + StreamUnaryClientInterceptor, + StreamStreamClientInterceptor, ServerInterceptor) from ._server import server from ._base_server import Server, ServicerContext from ._typing import ChannelArgumentType @@ -63,6 +64,7 @@ __all__ = ( 'UnaryStreamClientInterceptor', 'UnaryUnaryClientInterceptor', 'StreamUnaryClientInterceptor', + 'StreamStreamClientInterceptor', 'InterceptedUnaryUnaryCall', 'ServerInterceptor', 'insecure_channel', diff --git a/src/python/grpcio/grpc/experimental/aio/_channel.py b/src/python/grpcio/grpc/experimental/aio/_channel.py index 97ffa833bbf..7427872e0b3 100644 --- a/src/python/grpcio/grpc/experimental/aio/_channel.py +++ b/src/python/grpcio/grpc/experimental/aio/_channel.py @@ -24,12 +24,11 @@ from grpc._cython import cygrpc from . import _base_call, _base_channel from ._call import (StreamStreamCall, StreamUnaryCall, UnaryStreamCall, UnaryUnaryCall) -from ._interceptor import (InterceptedUnaryUnaryCall, - InterceptedUnaryStreamCall, - InterceptedStreamUnaryCall, ClientInterceptor, - UnaryUnaryClientInterceptor, - UnaryStreamClientInterceptor, - StreamUnaryClientInterceptor) +from ._interceptor import ( + InterceptedUnaryUnaryCall, InterceptedUnaryStreamCall, + InterceptedStreamUnaryCall, InterceptedStreamStreamCall, ClientInterceptor, + UnaryUnaryClientInterceptor, UnaryStreamClientInterceptor, + StreamUnaryClientInterceptor, StreamStreamClientInterceptor) from ._typing import (ChannelArgumentType, DeserializingFunction, MetadataType, SerializingFunction, RequestIterableType) from ._utils import _timeout_to_deadline @@ -200,10 +199,17 @@ class StreamStreamMultiCallable(_BaseMultiCallable, deadline = _timeout_to_deadline(timeout) - call = StreamStreamCall(request_iterator, deadline, metadata, - credentials, wait_for_ready, self._channel, - self._method, self._request_serializer, - self._response_deserializer, self._loop) + if not self._interceptors: + call = StreamStreamCall(request_iterator, deadline, metadata, + credentials, wait_for_ready, self._channel, + self._method, self._request_serializer, + self._response_deserializer, self._loop) + else: + call = InterceptedStreamStreamCall( + self._interceptors, request_iterator, deadline, metadata, + credentials, wait_for_ready, self._channel, self._method, + self._request_serializer, self._response_deserializer, + self._loop) return call @@ -214,6 +220,7 @@ class Channel(_base_channel.Channel): _unary_unary_interceptors: List[UnaryUnaryClientInterceptor] _unary_stream_interceptors: List[UnaryStreamClientInterceptor] _stream_unary_interceptors: List[StreamUnaryClientInterceptor] + _stream_stream_interceptors: List[StreamStreamClientInterceptor] def __init__(self, target: str, options: ChannelArgumentType, credentials: Optional[grpc.ChannelCredentials], @@ -233,35 +240,25 @@ class Channel(_base_channel.Channel): self._unary_unary_interceptors = [] self._unary_stream_interceptors = [] self._stream_unary_interceptors = [] - - if interceptors: - attrs_and_interceptor_classes = ((self._unary_unary_interceptors, - UnaryUnaryClientInterceptor), - (self._unary_stream_interceptors, - UnaryStreamClientInterceptor), - (self._stream_unary_interceptors, - StreamUnaryClientInterceptor)) - - # pylint: disable=cell-var-from-loop - for attr, interceptor_class in attrs_and_interceptor_classes: - attr.extend([ - interceptor for interceptor in interceptors - if isinstance(interceptor, interceptor_class) - ]) - - invalid_interceptors = set(interceptors) - set( - self._unary_unary_interceptors) - set( - self._unary_stream_interceptors) - set( - self._stream_unary_interceptors) - - if invalid_interceptors: - raise ValueError( - "Interceptor must be " + - "{} or ".format(UnaryUnaryClientInterceptor.__name__) + - "{} or ".format(UnaryStreamClientInterceptor.__name__) + - "{}. ".format(StreamUnaryClientInterceptor.__name__) + - "The following are invalid: {}".format(invalid_interceptors) - ) + self._stream_stream_interceptors = [] + + if interceptors is not None: + for interceptor in interceptors: + if isinstance(interceptor, UnaryUnaryClientInterceptor): + self._unary_unary_interceptors.append(interceptor) + elif isinstance(interceptor, UnaryStreamClientInterceptor): + self._unary_stream_interceptors.append(interceptor) + elif isinstance(interceptor, StreamUnaryClientInterceptor): + self._stream_unary_interceptors.append(interceptor) + elif isinstance(interceptor, StreamStreamClientInterceptor): + self._stream_stream_interceptors.append(interceptor) + else: + raise ValueError( + "Interceptor {} must be ".format(interceptor) + + "{} or ".format(UnaryUnaryClientInterceptor.__name__) + + "{} or ".format(UnaryStreamClientInterceptor.__name__) + + "{} or ".format(StreamUnaryClientInterceptor.__name__) + + "{}. ".format(StreamStreamClientInterceptor.__name__)) self._loop = asyncio.get_event_loop() self._channel = cygrpc.AioChannel( @@ -411,7 +408,8 @@ class Channel(_base_channel.Channel): ) -> StreamStreamMultiCallable: return StreamStreamMultiCallable(self._channel, _common.encode(method), request_serializer, - response_deserializer, None, + response_deserializer, + self._stream_stream_interceptors, self._loop) diff --git a/src/python/grpcio/grpc/experimental/aio/_interceptor.py b/src/python/grpcio/grpc/experimental/aio/_interceptor.py index e4969ddb4a5..e276ae0c922 100644 --- a/src/python/grpcio/grpc/experimental/aio/_interceptor.py +++ b/src/python/grpcio/grpc/experimental/aio/_interceptor.py @@ -22,13 +22,13 @@ import grpc from grpc._cython import cygrpc from . import _base_call -from ._call import UnaryUnaryCall, UnaryStreamCall, StreamUnaryCall, AioRpcError +from ._call import UnaryUnaryCall, UnaryStreamCall, StreamUnaryCall, StreamStreamCall, AioRpcError from ._call import _RPC_ALREADY_FINISHED_DETAILS, _RPC_HALF_CLOSED_DETAILS from ._call import _API_STYLE_ERROR from ._utils import _timeout_to_deadline from ._typing import (RequestType, SerializingFunction, DeserializingFunction, MetadataType, ResponseType, DoneCallbackType, - RequestIterableType) + RequestIterableType, ResponseIterableType) _LOCAL_CANCELLATION_DETAILS = 'Locally cancelled by application!' @@ -132,7 +132,7 @@ class UnaryStreamClientInterceptor(ClientInterceptor, metaclass=ABCMeta): self, continuation: Callable[[ClientCallDetails, RequestType], UnaryStreamCall], client_call_details: ClientCallDetails, request: RequestType - ) -> Union[AsyncIterable[ResponseType], UnaryStreamCall]: + ) -> Union[ResponseIterableType, UnaryStreamCall]: """Intercepts a unary-stream invocation asynchronously. The function could return the call object or an asynchronous @@ -153,7 +153,7 @@ class UnaryStreamClientInterceptor(ClientInterceptor, metaclass=ABCMeta): request: The request value for the RPC. Returns: - The RPC Call. + The RPC Call or an asynchronous iterator. Raises: AioRpcError: Indicating that the RPC terminated with non-OK status. @@ -202,6 +202,51 @@ class StreamUnaryClientInterceptor(ClientInterceptor, metaclass=ABCMeta): """ +class StreamStreamClientInterceptor(ClientInterceptor, metaclass=ABCMeta): + """Affords intercepting stream-stream invocations.""" + + @abstractmethod + async def intercept_stream_stream( + self, + continuation: Callable[[ClientCallDetails, RequestType], + UnaryStreamCall], + client_call_details: ClientCallDetails, + request_iterator: RequestIterableType, + ) -> Union[ResponseIterableType, StreamStreamCall]: + """Intercepts a stream-stream invocation asynchronously. + + Within the interceptor the usage of the call methods like `write` or + even awaiting the call should be done carefully, since the caller + could be expecting an untouched call, for example for start writing + messages to it. + + The function could return the call object or an asynchronous + iterator, in case of being an asyncrhonous iterator this will + become the source of the reads done by the caller. + + Args: + continuation: A coroutine that proceeds with the invocation by + executing the next interceptor in the chain or invoking the + actual RPC on the underlying Channel. It is the interceptor's + responsibility to call it if it decides to move the RPC forward. + The interceptor can use + `call = await continuation(client_call_details, request_iterator)` + to continue with the RPC. `continuation` returns the call to the + RPC. + client_call_details: A ClientCallDetails object describing the + outgoing RPC. + request_iterator: The request iterator that will produce requests + for the RPC. + + Returns: + The RPC Call or an asynchronous iterator. + + Raises: + AioRpcError: Indicating that the RPC terminated with non-OK status. + asyncio.CancelledError: Indicating that the RPC was canceled. + """ + + class InterceptedCall: """Base implementation for all intecepted call arities. @@ -388,6 +433,115 @@ class _InterceptedUnaryResponseMixin: return response +class _InterceptedStreamResponseMixin: + _response_aiter: Optional[AsyncIterable[ResponseType]] + + def _init_stream_response_mixin(self) -> None: + # Is initalized later, otherwise if the iterator is not finnally + # consumed a logging warning is emmited by Asyncio. + self._response_aiter = None + + async def _wait_for_interceptor_task_response_iterator(self + ) -> ResponseType: + call = await self._interceptors_task + async for response in call: + yield response + + def __aiter__(self) -> AsyncIterable[ResponseType]: + if self._response_aiter is None: + self._response_aiter = self._wait_for_interceptor_task_response_iterator( + ) + return self._response_aiter + + async def read(self) -> ResponseType: + if self._response_aiter is None: + self._response_aiter = self._wait_for_interceptor_task_response_iterator( + ) + return await self._response_aiter.asend(None) + + +class _InterceptedStreamRequestMixin: + + _write_to_iterator_async_gen: Optional[AsyncIterable[RequestType]] + _write_to_iterator_queue: Optional[asyncio.Queue] + + _FINISH_ITERATOR_SENTINEL = object() + + def _init_stream_request_mixin( + self, request_iterator: Optional[RequestIterableType] + ) -> RequestIterableType: + + if request_iterator is None: + # We provide our own request iterator which is a proxy + # of the futures writes that will be done by the caller. + self._write_to_iterator_queue = asyncio.Queue(maxsize=1) + self._write_to_iterator_async_gen = self._proxy_writes_as_request_iterator( + ) + request_iterator = self._write_to_iterator_async_gen + else: + self._write_to_iterator_queue = None + + return request_iterator + + async def _proxy_writes_as_request_iterator(self): + await self._interceptors_task + + while True: + value = await self._write_to_iterator_queue.get() + if value is _InterceptedStreamRequestMixin._FINISH_ITERATOR_SENTINEL: + break + yield value + + async def write(self, request: RequestType) -> None: + # If no queue was created it means that requests + # should be expected through an iterators provided + # by the caller. + if self._write_to_iterator_queue is None: + raise cygrpc.UsageError(_API_STYLE_ERROR) + + try: + call = await self._interceptors_task + except (asyncio.CancelledError, AioRpcError): + raise asyncio.InvalidStateError(_RPC_ALREADY_FINISHED_DETAILS) + + if call.done(): + raise asyncio.InvalidStateError(_RPC_ALREADY_FINISHED_DETAILS) + elif call._done_writing_flag: + raise asyncio.InvalidStateError(_RPC_HALF_CLOSED_DETAILS) + + # Write might never end up since the call could abrubtly finish, + # we give up on the first awaitable object that finishes. + _, _ = await asyncio.wait( + (self._write_to_iterator_queue.put(request), call.code()), + return_when=asyncio.FIRST_COMPLETED) + + if call.done(): + raise asyncio.InvalidStateError(_RPC_ALREADY_FINISHED_DETAILS) + + async def done_writing(self) -> None: + """Signal peer that client is done writing. + + This method is idempotent. + """ + # If no queue was created it means that requests + # should be expected through an iterators provided + # by the caller. + if self._write_to_iterator_queue is None: + raise cygrpc.UsageError(_API_STYLE_ERROR) + + try: + call = await self._interceptors_task + except asyncio.CancelledError: + raise asyncio.InvalidStateError(_RPC_ALREADY_FINISHED_DETAILS) + + # Write might never end up since the call could abrubtly finish, + # we give up on the first awaitable object that finishes. + _, _ = await asyncio.wait((self._write_to_iterator_queue.put( + _InterceptedStreamRequestMixin._FINISH_ITERATOR_SENTINEL), + call.code()), + return_when=asyncio.FIRST_COMPLETED) + + class InterceptedUnaryUnaryCall(_InterceptedUnaryResponseMixin, InterceptedCall, _base_call.UnaryUnaryCall): """Used for running a `UnaryUnaryCall` wrapped by interceptors. @@ -463,12 +617,12 @@ class InterceptedUnaryUnaryCall(_InterceptedUnaryResponseMixin, InterceptedCall, raise NotImplementedError() -class InterceptedUnaryStreamCall(InterceptedCall, _base_call.UnaryStreamCall): +class InterceptedUnaryStreamCall(_InterceptedStreamResponseMixin, + InterceptedCall, _base_call.UnaryStreamCall): """Used for running a `UnaryStreamCall` wrapped by interceptors.""" _loop: asyncio.AbstractEventLoop _channel: cygrpc.AioChannel - _response_aiter: AsyncIterable[ResponseType] _last_returned_call_from_interceptors = Optional[_base_call.UnaryStreamCall] # pylint: disable=too-many-arguments @@ -482,8 +636,7 @@ class InterceptedUnaryStreamCall(InterceptedCall, _base_call.UnaryStreamCall): loop: asyncio.AbstractEventLoop) -> None: self._loop = loop self._channel = channel - self._response_aiter = self._wait_for_interceptor_task_response_iterator( - ) + self._init_stream_response_mixin() self._last_returned_call_from_interceptors = None interceptors_task = loop.create_task( self._invoke(interceptors, method, timeout, metadata, credentials, @@ -517,7 +670,7 @@ class InterceptedUnaryStreamCall(InterceptedCall, _base_call.UnaryStreamCall): continuation, client_call_details, request) if isinstance(call_or_response_iterator, - _base_call.UnaryUnaryCall): + _base_call.UnaryStreamCall): self._last_returned_call_from_interceptors = call_or_response_iterator else: self._last_returned_call_from_interceptors = UnaryStreamCallResponseIterator( @@ -540,23 +693,12 @@ class InterceptedUnaryStreamCall(InterceptedCall, _base_call.UnaryStreamCall): return await _run_interceptor(iter(interceptors), client_call_details, request) - async def _wait_for_interceptor_task_response_iterator(self - ) -> ResponseType: - call = await self._interceptors_task - async for response in call: - yield response - - def __aiter__(self) -> AsyncIterable[ResponseType]: - return self._response_aiter - - async def read(self) -> ResponseType: - return await self._response_aiter.asend(None) - def time_remaining(self) -> Optional[float]: raise NotImplementedError() class InterceptedStreamUnaryCall(_InterceptedUnaryResponseMixin, + _InterceptedStreamRequestMixin, InterceptedCall, _base_call.StreamUnaryCall): """Used for running a `StreamUnaryCall` wrapped by interceptors. @@ -566,10 +708,6 @@ class InterceptedStreamUnaryCall(_InterceptedUnaryResponseMixin, _loop: asyncio.AbstractEventLoop _channel: cygrpc.AioChannel - _write_to_iterator_async_gen: Optional[AsyncIterable[RequestType]] - _write_to_iterator_queue: Optional[asyncio.Queue] - - _FINISH_ITERATOR_SENTINEL = object() # pylint: disable=too-many-arguments def __init__(self, interceptors: Sequence[StreamUnaryClientInterceptor], @@ -582,16 +720,7 @@ class InterceptedStreamUnaryCall(_InterceptedUnaryResponseMixin, loop: asyncio.AbstractEventLoop) -> None: self._loop = loop self._channel = channel - if request_iterator is None: - # We provide our own request iterator which is a proxy - # of the futures writes that will be done by the caller. - self._write_to_iterator_queue = asyncio.Queue(maxsize=1) - self._write_to_iterator_async_gen = self._proxy_writes_as_request_iterator( - ) - request_iterator = self._write_to_iterator_async_gen - else: - self._write_to_iterator_queue = None - + request_iterator = self._init_stream_request_mixin(request_iterator) interceptors_task = loop.create_task( self._invoke(interceptors, method, timeout, metadata, credentials, wait_for_ready, request_iterator, request_serializer, @@ -641,62 +770,88 @@ class InterceptedStreamUnaryCall(_InterceptedUnaryResponseMixin, def time_remaining(self) -> Optional[float]: raise NotImplementedError() - async def _proxy_writes_as_request_iterator(self): - await self._interceptors_task - while True: - value = await self._write_to_iterator_queue.get() - if value is InterceptedStreamUnaryCall._FINISH_ITERATOR_SENTINEL: - break - yield value +class InterceptedStreamStreamCall(_InterceptedStreamResponseMixin, + _InterceptedStreamRequestMixin, + InterceptedCall, _base_call.StreamStreamCall): + """Used for running a `StreamStreamCall` wrapped by interceptors.""" - async def write(self, request: RequestType) -> None: - # If no queue was created it means that requests - # should be expected through an iterators provided - # by the caller. - if self._write_to_iterator_queue is None: - raise cygrpc.UsageError(_API_STYLE_ERROR) + _loop: asyncio.AbstractEventLoop + _channel: cygrpc.AioChannel + _last_returned_call_from_interceptors = Optional[_base_call.UnaryStreamCall] - try: - call = await self._interceptors_task - except (asyncio.CancelledError, AioRpcError): - raise asyncio.InvalidStateError(_RPC_ALREADY_FINISHED_DETAILS) + # pylint: disable=too-many-arguments + def __init__(self, interceptors: Sequence[StreamStreamClientInterceptor], + request_iterator: Optional[RequestIterableType], + timeout: Optional[float], metadata: MetadataType, + credentials: Optional[grpc.CallCredentials], + wait_for_ready: Optional[bool], channel: cygrpc.AioChannel, + method: bytes, request_serializer: SerializingFunction, + response_deserializer: DeserializingFunction, + loop: asyncio.AbstractEventLoop) -> None: + self._loop = loop + self._channel = channel + self._init_stream_response_mixin() + request_iterator = self._init_stream_request_mixin(request_iterator) + self._last_returned_call_from_interceptors = None + interceptors_task = loop.create_task( + self._invoke(interceptors, method, timeout, metadata, credentials, + wait_for_ready, request_iterator, request_serializer, + response_deserializer)) + super().__init__(interceptors_task) - if call.done(): - raise asyncio.InvalidStateError(_RPC_ALREADY_FINISHED_DETAILS) - elif call._done_writing_flag: - raise asyncio.InvalidStateError(_RPC_HALF_CLOSED_DETAILS) + # pylint: disable=too-many-arguments + async def _invoke( + self, interceptors: Sequence[StreamStreamClientInterceptor], + method: bytes, timeout: Optional[float], + metadata: Optional[MetadataType], + credentials: Optional[grpc.CallCredentials], + wait_for_ready: Optional[bool], + request_iterator: RequestIterableType, + request_serializer: SerializingFunction, + response_deserializer: DeserializingFunction) -> StreamStreamCall: + """Run the RPC call wrapped in interceptors""" - # Write might never end up since the call could abrubtly finish, - # we give up on the first awaitable object that finishes.. - _, _ = await asyncio.wait( - (self._write_to_iterator_queue.put(request), call), - return_when=asyncio.FIRST_COMPLETED) + async def _run_interceptor( + interceptors: Iterator[StreamStreamClientInterceptor], + client_call_details: ClientCallDetails, + request_iterator: RequestIterableType + ) -> _base_call.StreamStreamCall: - if call.done(): - raise asyncio.InvalidStateError(_RPC_ALREADY_FINISHED_DETAILS) + interceptor = next(interceptors, None) - async def done_writing(self) -> None: - """Signal peer that client is done writing. + if interceptor: + continuation = functools.partial(_run_interceptor, interceptors) - This method is idempotent. - """ - # If no queue was created it means that requests - # should be expected through an iterators provided - # by the caller. - if self._write_to_iterator_queue is None: - raise cygrpc.UsageError(_API_STYLE_ERROR) + call_or_response_iterator = await interceptor.intercept_stream_stream( + continuation, client_call_details, request_iterator) - try: - call = await self._interceptors_task - except asyncio.CancelledError: - raise asyncio.InvalidStateError(_RPC_ALREADY_FINISHED_DETAILS) + if isinstance(call_or_response_iterator, + _base_call.StreamStreamCall): + self._last_returned_call_from_interceptors = call_or_response_iterator + else: + self._last_returned_call_from_interceptors = StreamStreamCallResponseIterator( + self._last_returned_call_from_interceptors, + call_or_response_iterator) + return self._last_returned_call_from_interceptors + else: + self._last_returned_call_from_interceptors = StreamStreamCall( + request_iterator, + _timeout_to_deadline(client_call_details.timeout), + client_call_details.metadata, + client_call_details.credentials, + client_call_details.wait_for_ready, self._channel, + client_call_details.method, request_serializer, + response_deserializer, self._loop) + return self._last_returned_call_from_interceptors - # Write might never end up since the call could abrubtly finish, - # we give up on the first awaitable object that finishes. - _, _ = await asyncio.wait((self._write_to_iterator_queue.put( - InterceptedStreamUnaryCall._FINISH_ITERATOR_SENTINEL), call), - return_when=asyncio.FIRST_COMPLETED) + client_call_details = ClientCallDetails(method, timeout, metadata, + credentials, wait_for_ready) + return await _run_interceptor(iter(interceptors), client_call_details, + request_iterator) + + def time_remaining(self) -> Optional[float]: + raise NotImplementedError() class UnaryUnaryCallResponse(_base_call.UnaryUnaryCall): @@ -747,12 +902,13 @@ class UnaryUnaryCallResponse(_base_call.UnaryUnaryCall): pass -class UnaryStreamCallResponseIterator(_base_call.UnaryStreamCall): - """UnaryStreamCall class wich uses an alternative response iterator.""" - _call: _base_call.UnaryStreamCall +class _StreamCallResponseIterator: + + _call: Union[_base_call.UnaryStreamCall, _base_call.StreamStreamCall] _response_iterator: AsyncIterable[ResponseType] - def __init__(self, call: _base_call.UnaryStreamCall, + def __init__(self, call: Union[_base_call.UnaryStreamCall, _base_call. + StreamStreamCall], response_iterator: AsyncIterable[ResponseType]) -> None: self._response_iterator = response_iterator self._call = call @@ -793,7 +949,38 @@ class UnaryStreamCallResponseIterator(_base_call.UnaryStreamCall): async def wait_for_connection(self) -> None: return await self._call.wait_for_connection() + +class UnaryStreamCallResponseIterator(_StreamCallResponseIterator, + _base_call.UnaryStreamCall): + """UnaryStreamCall class wich uses an alternative response iterator.""" + async def read(self) -> ResponseType: # Behind the scenes everyting goes through the # async iterator. So this path should not be reached. - raise Exception() + raise NotImplementedError() + + +class StreamStreamCallResponseIterator(_StreamCallResponseIterator, + _base_call.StreamStreamCall): + """StreamStreamCall class wich uses an alternative response iterator.""" + + async def read(self) -> ResponseType: + # Behind the scenes everyting goes through the + # async iterator. So this path should not be reached. + raise NotImplementedError() + + async def write(self, request: RequestType) -> None: + # Behind the scenes everyting goes through the + # async iterator provided by the InterceptedStreamStreamCall. + # So this path should not be reached. + raise NotImplementedError() + + async def done_writing(self) -> None: + # Behind the scenes everyting goes through the + # async iterator provided by the InterceptedStreamStreamCall. + # So this path should not be reached. + raise NotImplementedError() + + @property + def _done_writing_flag(self) -> bool: + return self._call._done_writing_flag diff --git a/src/python/grpcio/grpc/experimental/aio/_typing.py b/src/python/grpcio/grpc/experimental/aio/_typing.py index 205f6dc6227..a02ec8ff803 100644 --- a/src/python/grpcio/grpc/experimental/aio/_typing.py +++ b/src/python/grpcio/grpc/experimental/aio/_typing.py @@ -28,3 +28,4 @@ ChannelArgumentType = Sequence[Tuple[str, Any]] EOFType = type(EOF) DoneCallbackType = Callable[[Any], None] RequestIterableType = Union[Iterable[Any], AsyncIterable[Any]] +ResponseIterableType = AsyncIterable[Any] diff --git a/src/python/grpcio_tests/tests_aio/tests.json b/src/python/grpcio_tests/tests_aio/tests.json index 87cfce80ae8..f01d7d0570d 100644 --- a/src/python/grpcio_tests/tests_aio/tests.json +++ b/src/python/grpcio_tests/tests_aio/tests.json @@ -16,6 +16,7 @@ "unit.channel_argument_test.TestChannelArgument", "unit.channel_ready_test.TestChannelReady", "unit.channel_test.TestChannel", + "unit.client_stream_stream_interceptor_test.TestStreamStreamClientInterceptor", "unit.client_stream_unary_interceptor_test.TestStreamUnaryClientInterceptor", "unit.client_unary_stream_interceptor_test.TestUnaryStreamClientInterceptor", "unit.client_unary_unary_interceptor_test.TestInterceptedUnaryUnaryCall", diff --git a/src/python/grpcio_tests/tests_aio/unit/_common.py b/src/python/grpcio_tests/tests_aio/unit/_common.py index 97cbe759ed0..dab9454c58d 100644 --- a/src/python/grpcio_tests/tests_aio/unit/_common.py +++ b/src/python/grpcio_tests/tests_aio/unit/_common.py @@ -14,6 +14,7 @@ import asyncio import grpc +from typing import AsyncIterable from grpc.experimental import aio from grpc.experimental.aio._typing import MetadataType, MetadatumType @@ -37,7 +38,7 @@ async def block_until_certain_state(channel: aio.Channel, state = channel.get_state() -def inject_callbacks(call): +def inject_callbacks(call: aio.Call): first_callback_ran = asyncio.Event() def first_callback(call): @@ -64,3 +65,33 @@ def inject_callbacks(call): test_constants.SHORT_TIMEOUT) return validation() + + +class CountingRequestIterator: + + def __init__(self, request_iterator): + self.request_cnt = 0 + self._request_iterator = request_iterator + + async def _forward_requests(self): + async for request in self._request_iterator: + self.request_cnt += 1 + yield request + + def __aiter__(self): + return self._forward_requests() + + +class CountingResponseIterator: + + def __init__(self, response_iterator): + self.response_cnt = 0 + self._response_iterator = response_iterator + + async def _forward_responses(self): + async for response in self._response_iterator: + self.response_cnt += 1 + yield response + + def __aiter__(self): + return self._forward_responses() diff --git a/src/python/grpcio_tests/tests_aio/unit/client_stream_stream_interceptor_test.py b/src/python/grpcio_tests/tests_aio/unit/client_stream_stream_interceptor_test.py new file mode 100644 index 00000000000..9ab54b39a6b --- /dev/null +++ b/src/python/grpcio_tests/tests_aio/unit/client_stream_stream_interceptor_test.py @@ -0,0 +1,202 @@ +# Copyright 2020 The 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 logging +import unittest + +import grpc + +from grpc.experimental import aio +from tests_aio.unit._common import CountingResponseIterator, CountingRequestIterator +from tests_aio.unit._test_server import start_test_server +from tests_aio.unit._test_base import AioTestBase +from src.proto.grpc.testing import messages_pb2, test_pb2_grpc + +_NUM_STREAM_RESPONSES = 5 +_NUM_STREAM_REQUESTS = 5 +_RESPONSE_PAYLOAD_SIZE = 7 + + +class _StreamStreamInterceptorEmpty(aio.StreamStreamClientInterceptor): + + async def intercept_stream_stream(self, continuation, client_call_details, + request_iterator): + return await continuation(client_call_details, request_iterator) + + def assert_in_final_state(self, test: unittest.TestCase): + pass + + +class _StreamStreamInterceptorWithRequestAndResponseIterator( + aio.StreamStreamClientInterceptor): + + async def intercept_stream_stream(self, continuation, client_call_details, + request_iterator): + self.request_iterator = CountingRequestIterator(request_iterator) + call = await continuation(client_call_details, self.request_iterator) + self.response_iterator = CountingResponseIterator(call) + return self.response_iterator + + def assert_in_final_state(self, test: unittest.TestCase): + test.assertEqual(_NUM_STREAM_REQUESTS, + self.request_iterator.request_cnt) + test.assertEqual(_NUM_STREAM_RESPONSES, + self.response_iterator.response_cnt) + + +class TestStreamStreamClientInterceptor(AioTestBase): + + async def setUp(self): + self._server_target, self._server = await start_test_server() + + async def tearDown(self): + await self._server.stop(None) + + async def test_intercepts(self): + + for interceptor_class in ( + _StreamStreamInterceptorEmpty, + _StreamStreamInterceptorWithRequestAndResponseIterator): + + with self.subTest(name=interceptor_class): + interceptor = interceptor_class() + channel = aio.insecure_channel(self._server_target, + interceptors=[interceptor]) + stub = test_pb2_grpc.TestServiceStub(channel) + + # Prepares the request + request = messages_pb2.StreamingOutputCallRequest() + request.response_parameters.append( + messages_pb2.ResponseParameters( + size=_RESPONSE_PAYLOAD_SIZE)) + + async def request_iterator(): + for _ in range(_NUM_STREAM_REQUESTS): + yield request + + call = stub.FullDuplexCall(request_iterator()) + + await call.wait_for_connection() + + response_cnt = 0 + async for response in call: + response_cnt += 1 + self.assertIsInstance( + response, messages_pb2.StreamingOutputCallResponse) + self.assertEqual(_RESPONSE_PAYLOAD_SIZE, + len(response.payload.body)) + + self.assertEqual(response_cnt, _NUM_STREAM_RESPONSES) + self.assertEqual(await call.code(), grpc.StatusCode.OK) + self.assertEqual(await call.initial_metadata(), ()) + self.assertEqual(await call.trailing_metadata(), ()) + self.assertEqual(await call.details(), '') + self.assertEqual(await call.debug_error_string(), '') + self.assertEqual(call.cancel(), False) + self.assertEqual(call.cancelled(), False) + self.assertEqual(call.done(), True) + + interceptor.assert_in_final_state(self) + + await channel.close() + + async def test_intercepts_using_write_and_read(self): + for interceptor_class in ( + _StreamStreamInterceptorEmpty, + _StreamStreamInterceptorWithRequestAndResponseIterator): + + with self.subTest(name=interceptor_class): + interceptor = interceptor_class() + channel = aio.insecure_channel(self._server_target, + interceptors=[interceptor]) + stub = test_pb2_grpc.TestServiceStub(channel) + + # Prepares the request + request = messages_pb2.StreamingOutputCallRequest() + request.response_parameters.append( + messages_pb2.ResponseParameters( + size=_RESPONSE_PAYLOAD_SIZE)) + + call = stub.FullDuplexCall() + + for _ in range(_NUM_STREAM_RESPONSES): + await call.write(request) + response = await call.read() + self.assertIsInstance( + response, messages_pb2.StreamingOutputCallResponse) + self.assertEqual(_RESPONSE_PAYLOAD_SIZE, + len(response.payload.body)) + + await call.done_writing() + + self.assertEqual(await call.code(), grpc.StatusCode.OK) + self.assertEqual(await call.initial_metadata(), ()) + self.assertEqual(await call.trailing_metadata(), ()) + self.assertEqual(await call.details(), '') + self.assertEqual(await call.debug_error_string(), '') + self.assertEqual(call.cancel(), False) + self.assertEqual(call.cancelled(), False) + self.assertEqual(call.done(), True) + + interceptor.assert_in_final_state(self) + + await channel.close() + + async def test_multiple_interceptors_request_iterator(self): + for interceptor_class in ( + _StreamStreamInterceptorEmpty, + _StreamStreamInterceptorWithRequestAndResponseIterator): + + with self.subTest(name=interceptor_class): + + interceptors = [interceptor_class(), interceptor_class()] + channel = aio.insecure_channel(self._server_target, + interceptors=interceptors) + stub = test_pb2_grpc.TestServiceStub(channel) + + # Prepares the request + request = messages_pb2.StreamingOutputCallRequest() + request.response_parameters.append( + messages_pb2.ResponseParameters( + size=_RESPONSE_PAYLOAD_SIZE)) + + call = stub.FullDuplexCall() + + for _ in range(_NUM_STREAM_RESPONSES): + await call.write(request) + response = await call.read() + self.assertIsInstance( + response, messages_pb2.StreamingOutputCallResponse) + self.assertEqual(_RESPONSE_PAYLOAD_SIZE, + len(response.payload.body)) + + await call.done_writing() + + self.assertEqual(await call.code(), grpc.StatusCode.OK) + self.assertEqual(await call.initial_metadata(), ()) + self.assertEqual(await call.trailing_metadata(), ()) + self.assertEqual(await call.details(), '') + self.assertEqual(await call.debug_error_string(), '') + self.assertEqual(call.cancel(), False) + self.assertEqual(call.cancelled(), False) + self.assertEqual(call.done(), True) + + for interceptor in interceptors: + interceptor.assert_in_final_state(self) + + await channel.close() + + +if __name__ == '__main__': + logging.basicConfig(level=logging.DEBUG) + unittest.main(verbosity=2) diff --git a/src/python/grpcio_tests/tests_aio/unit/client_stream_unary_interceptor_test.py b/src/python/grpcio_tests/tests_aio/unit/client_stream_unary_interceptor_test.py index 318a117ffe4..2d58cff0e41 100644 --- a/src/python/grpcio_tests/tests_aio/unit/client_stream_unary_interceptor_test.py +++ b/src/python/grpcio_tests/tests_aio/unit/client_stream_unary_interceptor_test.py @@ -21,6 +21,7 @@ import grpc from grpc.experimental import aio from tests_aio.unit._constants import UNREACHABLE_TARGET from tests_aio.unit._common import inject_callbacks +from tests_aio.unit._common import CountingRequestIterator from tests_aio.unit._test_server import start_test_server from tests_aio.unit._test_base import AioTestBase from tests.unit.framework.common import test_constants @@ -33,21 +34,6 @@ _REQUEST_PAYLOAD_SIZE = 7 _RESPONSE_INTERVAL_US = int(_SHORT_TIMEOUT_S * 1000 * 1000) -class _CountingRequestIterator: - - def __init__(self, request_iterator): - self.request_cnt = 0 - self._request_iterator = request_iterator - - async def _forward_requests(self): - async for request in self._request_iterator: - self.request_cnt += 1 - yield request - - def __aiter__(self): - return self._forward_requests() - - class _StreamUnaryInterceptorEmpty(aio.StreamUnaryClientInterceptor): async def intercept_stream_unary(self, continuation, client_call_details, @@ -63,7 +49,7 @@ class _StreamUnaryInterceptorWithRequestIterator( async def intercept_stream_unary(self, continuation, client_call_details, request_iterator): - self.request_iterator = _CountingRequestIterator(request_iterator) + self.request_iterator = CountingRequestIterator(request_iterator) call = await continuation(client_call_details, self.request_iterator) return call diff --git a/src/python/grpcio_tests/tests_aio/unit/client_unary_stream_interceptor_test.py b/src/python/grpcio_tests/tests_aio/unit/client_unary_stream_interceptor_test.py index cfb3ead226f..6137538ffca 100644 --- a/src/python/grpcio_tests/tests_aio/unit/client_unary_stream_interceptor_test.py +++ b/src/python/grpcio_tests/tests_aio/unit/client_unary_stream_interceptor_test.py @@ -21,6 +21,7 @@ import grpc from grpc.experimental import aio from tests_aio.unit._constants import UNREACHABLE_TARGET from tests_aio.unit._common import inject_callbacks +from tests_aio.unit._common import CountingResponseIterator from tests_aio.unit._test_server import start_test_server from tests_aio.unit._test_base import AioTestBase from tests.unit.framework.common import test_constants @@ -34,21 +35,6 @@ _RESPONSE_PAYLOAD_SIZE = 7 _RESPONSE_INTERVAL_US = int(_SHORT_TIMEOUT_S * 1000 * 1000) -class _CountingResponseIterator: - - def __init__(self, response_iterator): - self.response_cnt = 0 - self._response_iterator = response_iterator - - async def _forward_responses(self): - async for response in self._response_iterator: - self.response_cnt += 1 - yield response - - def __aiter__(self): - return self._forward_responses() - - class _UnaryStreamInterceptorEmpty(aio.UnaryStreamClientInterceptor): async def intercept_unary_stream(self, continuation, client_call_details, @@ -65,7 +51,7 @@ class _UnaryStreamInterceptorWithResponseIterator( async def intercept_unary_stream(self, continuation, client_call_details, request): call = await continuation(client_call_details, request) - self.response_iterator = _CountingResponseIterator(call) + self.response_iterator = CountingResponseIterator(call) return self.response_iterator def assert_in_final_state(self, test: unittest.TestCase): diff --git a/test/core/bad_client/tests/large_metadata.cc b/test/core/bad_client/tests/large_metadata.cc index 8206afa5afb..e8b153f4dd9 100644 --- a/test/core/bad_client/tests/large_metadata.cc +++ b/test/core/bad_client/tests/large_metadata.cc @@ -20,6 +20,9 @@ #include +#include "absl/strings/str_format.h" +#include "absl/strings/str_join.h" + #include #include #include "src/core/lib/gpr/string.h" @@ -144,22 +147,17 @@ int main(int argc, char** argv) { grpc::testing::TestEnvironment env(argc, argv); // Test sending more metadata than the server will accept. - gpr_strvec headers; - gpr_strvec_init(&headers); + std::vector headers; for (i = 0; i < NUM_HEADERS; ++i) { - char* str; - gpr_asprintf(&str, "%s%02d%s", - PFX_TOO_MUCH_METADATA_FROM_CLIENT_HEADER_START_STR, i, - PFX_TOO_MUCH_METADATA_FROM_CLIENT_HEADER_END_STR); - gpr_strvec_add(&headers, str); + headers.push_back(absl::StrFormat( + "%s%02d%s", PFX_TOO_MUCH_METADATA_FROM_CLIENT_HEADER_START_STR, i, + PFX_TOO_MUCH_METADATA_FROM_CLIENT_HEADER_END_STR)); } - size_t headers_len; - const char* client_headers = gpr_strvec_flatten(&headers, &headers_len); - gpr_strvec_destroy(&headers); + std::string client_headers = absl::StrJoin(headers, ""); char client_payload[TOO_MUCH_METADATA_FROM_CLIENT_REQUEST_SIZE] = PFX_TOO_MUCH_METADATA_FROM_CLIENT_REQUEST; memcpy(client_payload + sizeof(PFX_TOO_MUCH_METADATA_FROM_CLIENT_REQUEST) - 1, - client_headers, headers_len); + client_headers.data(), client_headers.size()); grpc_bad_client_arg args[2]; args[0] = connection_preface_arg; args[1].client_validator = rst_stream_client_validator; @@ -167,7 +165,6 @@ int main(int argc, char** argv) { args[1].client_payload_length = sizeof(client_payload) - 1; grpc_run_bad_client_test(server_verifier_request_call, args, 2, 0); - gpr_free((void*)client_headers); // Test sending more metadata than the client will accept. GRPC_RUN_BAD_CLIENT_TEST(server_verifier_sends_too_much_metadata, diff --git a/test/core/channel/minimal_stack_is_minimal_test.cc b/test/core/channel/minimal_stack_is_minimal_test.cc index 858fd5676a1..455d6ae3634 100644 --- a/test/core/channel/minimal_stack_is_minimal_test.cc +++ b/test/core/channel/minimal_stack_is_minimal_test.cc @@ -34,6 +34,10 @@ #include #include +#include "absl/strings/str_cat.h" +#include "absl/strings/str_format.h" +#include "absl/strings/str_join.h" + #include "src/core/lib/channel/channel_stack_builder.h" #include "src/core/lib/gpr/string.h" #include "src/core/lib/surface/channel_init.h" @@ -138,64 +142,51 @@ static int check_stack(const char* file, int line, const char* transport_name, } // build up our expectation list - gpr_strvec v; - gpr_strvec_init(&v); + std::vector parts; va_list args; va_start(args, channel_stack_type); for (;;) { char* a = va_arg(args, char*); if (a == nullptr) break; - if (v.count != 0) gpr_strvec_add(&v, gpr_strdup(", ")); - gpr_strvec_add(&v, gpr_strdup(a)); + parts.push_back(a); } va_end(args); - char* expect = gpr_strvec_flatten(&v, nullptr); - gpr_strvec_destroy(&v); + std::string expect = absl::StrJoin(parts, ", "); // build up our "got" list - gpr_strvec_init(&v); + parts.clear(); grpc_channel_stack_builder_iterator* it = grpc_channel_stack_builder_create_iterator_at_first(builder); while (grpc_channel_stack_builder_move_next(it)) { const char* name = grpc_channel_stack_builder_iterator_filter_name(it); if (name == nullptr) continue; - if (v.count != 0) gpr_strvec_add(&v, gpr_strdup(", ")); - gpr_strvec_add(&v, gpr_strdup(name)); + parts.push_back(name); } - char* got = gpr_strvec_flatten(&v, nullptr); - gpr_strvec_destroy(&v); + std::string got = absl::StrJoin(parts, ", "); grpc_channel_stack_builder_iterator_destroy(it); // figure out result, log if there's an error int result = 0; - if (0 != strcmp(got, expect)) { - gpr_strvec_init(&v); - gpr_strvec_add(&v, gpr_strdup("{")); + if (got != expect) { + parts.clear(); for (size_t i = 0; i < channel_args->num_args; i++) { - if (i > 0) gpr_strvec_add(&v, gpr_strdup(", ")); - gpr_strvec_add(&v, gpr_strdup(channel_args->args[i].key)); - gpr_strvec_add(&v, gpr_strdup("=")); + std::string value; switch (channel_args->args[i].type) { case GRPC_ARG_INTEGER: { - char* tmp; - gpr_asprintf(&tmp, "%d", channel_args->args[i].value.integer); - gpr_strvec_add(&v, tmp); + value = absl::StrCat(channel_args->args[i].value.integer); break; } case GRPC_ARG_STRING: - gpr_strvec_add(&v, gpr_strdup(channel_args->args[i].value.string)); + value = channel_args->args[i].value.string; break; case GRPC_ARG_POINTER: { - char* tmp; - gpr_asprintf(&tmp, "%p", channel_args->args[i].value.pointer.p); - gpr_strvec_add(&v, tmp); + value = absl::StrFormat("%p", channel_args->args[i].value.pointer.p); break; } } + parts.push_back(absl::StrCat(channel_args->args[i].key, "=", value)); } - gpr_strvec_add(&v, gpr_strdup("}")); - char* args_str = gpr_strvec_flatten(&v, nullptr); - gpr_strvec_destroy(&v); + std::string args_str = absl::StrCat("{", absl::StrJoin(parts, ", "), "}"); gpr_log(file, line, GPR_LOG_SEVERITY_ERROR, "**************************************************"); @@ -204,17 +195,12 @@ static int check_stack(const char* file, int line, const char* transport_name, "FAILED transport=%s; stack_type=%s; channel_args=%s:", transport_name, grpc_channel_stack_type_string( static_cast(channel_stack_type)), - args_str); - gpr_log(file, line, GPR_LOG_SEVERITY_ERROR, "EXPECTED: %s", expect); - gpr_log(file, line, GPR_LOG_SEVERITY_ERROR, "GOT: %s", got); + args_str.c_str()); + gpr_log(file, line, GPR_LOG_SEVERITY_ERROR, "EXPECTED: %s", expect.c_str()); + gpr_log(file, line, GPR_LOG_SEVERITY_ERROR, "GOT: %s", got.c_str()); result = 1; - - gpr_free(args_str); } - gpr_free(got); - gpr_free(expect); - { grpc_core::ExecCtx exec_ctx; grpc_channel_stack_builder_destroy(builder); diff --git a/test/core/end2end/cq_verifier.cc b/test/core/end2end/cq_verifier.cc index f4e7fe29cf1..6030b496226 100644 --- a/test/core/end2end/cq_verifier.cc +++ b/test/core/end2end/cq_verifier.cc @@ -23,6 +23,12 @@ #include #include +#include +#include + +#include "absl/strings/str_format.h" +#include "absl/strings/str_join.h" + #include #include #include @@ -171,21 +177,19 @@ int byte_buffer_eq_string(grpc_byte_buffer* bb, const char* str) { static bool is_probably_integer(void* p) { return ((uintptr_t)p) < 1000000; } -static void expectation_to_strvec(gpr_strvec* buf, expectation* e) { - char* tmp; +namespace { - if (is_probably_integer(e->tag)) { - gpr_asprintf(&tmp, "tag(%" PRIdPTR ") ", (intptr_t)e->tag); +std::string ExpectationString(const expectation& e) { + std::string out; + if (is_probably_integer(e.tag)) { + out = absl::StrFormat("tag(%" PRIdPTR ") ", (intptr_t)e.tag); } else { - gpr_asprintf(&tmp, "%p ", e->tag); + out = absl::StrFormat("%p ", e.tag); } - gpr_strvec_add(buf, tmp); - - switch (e->type) { + switch (e.type) { case GRPC_OP_COMPLETE: - gpr_asprintf(&tmp, "GRPC_OP_COMPLETE success=%d %s:%d", e->success, - e->file, e->line); - gpr_strvec_add(buf, tmp); + absl::StrAppendFormat(&out, "GRPC_OP_COMPLETE success=%d %s:%d", + e.success, e.file, e.line); break; case GRPC_QUEUE_TIMEOUT: case GRPC_QUEUE_SHUTDOWN: @@ -193,27 +197,22 @@ static void expectation_to_strvec(gpr_strvec* buf, expectation* e) { abort(); break; } + return out; } -static void expectations_to_strvec(gpr_strvec* buf, cq_verifier* v) { - expectation* e; - - for (e = v->first_expectation; e != nullptr; e = e->next) { - expectation_to_strvec(buf, e); - gpr_strvec_add(buf, gpr_strdup("\n")); +std::string ExpectationsString(const cq_verifier& v) { + std::vector expectations; + for (expectation* e = v.first_expectation; e != nullptr; e = e->next) { + expectations.push_back(ExpectationString(*e)); } + return absl::StrJoin(expectations, "\n"); } +} // namespace + static void fail_no_event_received(cq_verifier* v) { - gpr_strvec buf; - char* msg; - gpr_strvec_init(&buf); - gpr_strvec_add(&buf, gpr_strdup("no event received, but expected:\n")); - expectations_to_strvec(&buf, v); - msg = gpr_strvec_flatten(&buf, nullptr); - gpr_log(GPR_ERROR, "%s", msg); - gpr_strvec_destroy(&buf); - gpr_free(msg); + gpr_log(GPR_ERROR, "no event received, but expected:%s", + ExpectationsString(*v).c_str()); abort(); } @@ -222,13 +221,8 @@ static void verify_matches(expectation* e, grpc_event* ev) { switch (e->type) { case GRPC_OP_COMPLETE: if (e->check_success && e->success != ev->success) { - gpr_strvec expected; - gpr_strvec_init(&expected); - expectation_to_strvec(&expected, e); - char* s = gpr_strvec_flatten(&expected, nullptr); - gpr_strvec_destroy(&expected); - gpr_log(GPR_ERROR, "actual success does not match expected: %s", s); - gpr_free(s); + gpr_log(GPR_ERROR, "actual success does not match expected: %s", + ExpectationString(*e).c_str()); abort(); } break; @@ -264,16 +258,9 @@ void cq_verify(cq_verifier* v) { prev = e; } if (e == nullptr) { - char* s = grpc_event_string(&ev); - gpr_log(GPR_ERROR, "cq returned unexpected event: %s", s); - gpr_free(s); - gpr_strvec expectations; - gpr_strvec_init(&expectations); - expectations_to_strvec(&expectations, v); - s = gpr_strvec_flatten(&expectations, nullptr); - gpr_strvec_destroy(&expectations); - gpr_log(GPR_ERROR, "expected tags:\n%s", s); - gpr_free(s); + gpr_log(GPR_ERROR, "cq returned unexpected event: %s", + grpc_event_string(&ev).c_str()); + gpr_log(GPR_ERROR, "expected tags:\n%s", ExpectationsString(*v).c_str()); abort(); } } @@ -290,9 +277,8 @@ void cq_verify_empty_timeout(cq_verifier* v, int timeout_sec) { ev = grpc_completion_queue_next(v->cq, deadline, nullptr); if (ev.type != GRPC_QUEUE_TIMEOUT) { - char* s = grpc_event_string(&ev); - gpr_log(GPR_ERROR, "unexpected event (expected nothing): %s", s); - gpr_free(s); + gpr_log(GPR_ERROR, "unexpected event (expected nothing): %s", + grpc_event_string(&ev).c_str()); abort(); } } diff --git a/test/core/end2end/tests/server_finishes_request.cc b/test/core/end2end/tests/server_finishes_request.cc index dc489a4c7fb..9ee23c91ea0 100644 --- a/test/core/end2end/tests/server_finishes_request.cc +++ b/test/core/end2end/tests/server_finishes_request.cc @@ -151,7 +151,7 @@ static void simple_request_body(grpc_end2end_test_config /*config*/, 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; + 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; @@ -170,10 +170,10 @@ static void simple_request_body(grpc_end2end_test_config /*config*/, CQ_EXPECT_COMPLETION(cqv, tag(1), 1); cq_verify(cqv); - GPR_ASSERT(status == GRPC_STATUS_OK); + 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(was_cancelled == 0); + GPR_ASSERT(was_cancelled == 1); grpc_slice_unref(details); grpc_metadata_array_destroy(&initial_metadata_recv); diff --git a/test/core/end2end/tests/simple_request.cc b/test/core/end2end/tests/simple_request.cc index a8c57899e2c..8e0f863d847 100644 --- a/test/core/end2end/tests/simple_request.cc +++ b/test/core/end2end/tests/simple_request.cc @@ -21,6 +21,8 @@ #include #include +#include + #include #include #include @@ -245,9 +247,7 @@ static void simple_request_body(grpc_end2end_test_config config, grpc_stats_collect(after); - char* stats = grpc_stats_data_as_json(after); - gpr_log(GPR_DEBUG, "%s", stats); - gpr_free(stats); + gpr_log(GPR_DEBUG, "%s", grpc_stats_data_as_json(after).c_str()); GPR_ASSERT(after->counters[GRPC_STATS_COUNTER_CLIENT_CALLS_CREATED] - before->counters[GRPC_STATS_COUNTER_CLIENT_CALLS_CREATED] == diff --git a/test/core/gpr/arena_test.cc b/test/core/gpr/arena_test.cc index 1df1052ab95..c7aa63ab2e9 100644 --- a/test/core/gpr/arena_test.cc +++ b/test/core/gpr/arena_test.cc @@ -21,6 +21,9 @@ #include #include +#include "absl/strings/str_format.h" +#include "absl/strings/str_join.h" + #include #include #include @@ -37,20 +40,15 @@ static void test_noop(void) { Arena::Create(1)->Destroy(); } static void test(const char* name, size_t init_size, const size_t* allocs, size_t nallocs) { - gpr_strvec v; - char* s; - gpr_strvec_init(&v); - gpr_asprintf(&s, "test '%s': %" PRIdPTR " <- {", name, init_size); - gpr_strvec_add(&v, s); + std::vector parts; + parts.push_back( + absl::StrFormat("test '%s': %" PRIdPTR " <- {", name, init_size)); for (size_t i = 0; i < nallocs; i++) { - gpr_asprintf(&s, "%" PRIdPTR ",", allocs[i]); - gpr_strvec_add(&v, s); + parts.push_back(absl::StrFormat("%" PRIdPTR ",", allocs[i])); } - gpr_strvec_add(&v, gpr_strdup("}")); - s = gpr_strvec_flatten(&v, nullptr); - gpr_strvec_destroy(&v); - gpr_log(GPR_INFO, "%s", s); - gpr_free(s); + parts.push_back("}"); + std::string s = absl::StrJoin(parts, ""); + gpr_log(GPR_INFO, "%s", s.c_str()); Arena* a = Arena::Create(init_size); void** ps = static_cast(gpr_zalloc(sizeof(*ps) * nallocs)); diff --git a/test/core/security/verify_jwt.cc b/test/core/security/verify_jwt.cc index cfe285f53e6..2b480afcaa8 100644 --- a/test/core/security/verify_jwt.cc +++ b/test/core/security/verify_jwt.cc @@ -37,10 +37,9 @@ typedef struct { } synchronizer; static void print_usage_and_exit(gpr_cmdline* cl, const char* argv0) { - char* usage = gpr_cmdline_usage_string(cl, argv0); - fprintf(stderr, "%s", usage); + std::string usage = gpr_cmdline_usage_string(cl, argv0); + fprintf(stderr, "%s", usage.c_str()); fflush(stderr); - gpr_free(usage); gpr_cmdline_destroy(cl); exit(1); } diff --git a/test/core/surface/BUILD b/test/core/surface/BUILD index 37fa57e6927..7d789393c91 100644 --- a/test/core/surface/BUILD +++ b/test/core/surface/BUILD @@ -67,6 +67,7 @@ grpc_cc_test( grpc_cc_test( name = "concurrent_connectivity_test", srcs = ["concurrent_connectivity_test.cc"], + flaky = True, # TODO(b/157516542) language = "C++", deps = [ "//:gpr", diff --git a/test/core/util/cmdline.cc b/test/core/util/cmdline.cc index e76acb5e2b5..c213f84a73d 100644 --- a/test/core/util/cmdline.cc +++ b/test/core/util/cmdline.cc @@ -22,6 +22,12 @@ #include #include +#include + +#include "absl/strings/str_cat.h" +#include "absl/strings/str_format.h" +#include "absl/strings/str_join.h" + #include #include #include @@ -124,60 +130,42 @@ void gpr_cmdline_on_extra_arg( /* recursively descend argument list, adding the last element to s first - so that arguments are added in the order they were added to the list by api calls */ -static void add_args_to_usage(gpr_strvec* s, arg* a) { - char* tmp; - - if (!a) return; - add_args_to_usage(s, a->next); - +static void add_args_to_usage(arg* a, std::vector* s) { + if (a == nullptr) return; + add_args_to_usage(a->next, s); switch (a->type) { case ARGTYPE_BOOL: - gpr_asprintf(&tmp, " [--%s|--no-%s]", a->name, a->name); - gpr_strvec_add(s, tmp); + s->push_back(absl::StrFormat(" [--%s|--no-%s]", a->name, a->name)); break; case ARGTYPE_STRING: - gpr_asprintf(&tmp, " [--%s=string]", a->name); - gpr_strvec_add(s, tmp); + s->push_back(absl::StrFormat(" [--%s=string]", a->name)); break; case ARGTYPE_INT: - gpr_asprintf(&tmp, " [--%s=int]", a->name); - gpr_strvec_add(s, tmp); + s->push_back(absl::StrFormat(" [--%s=int]", a->name)); break; } } -char* gpr_cmdline_usage_string(gpr_cmdline* cl, const char* argv0) { - /* TODO(ctiller): make this prettier */ - gpr_strvec s; - char* tmp; +std::string gpr_cmdline_usage_string(gpr_cmdline* cl, const char* argv0) { const char* name = strrchr(argv0, '/'); - - if (name) { + if (name != nullptr) { name++; } else { name = argv0; } - gpr_strvec_init(&s); - - gpr_asprintf(&tmp, "Usage: %s", name); - gpr_strvec_add(&s, tmp); - add_args_to_usage(&s, cl->args); + std::vector s; + s.push_back(absl::StrCat("Usage: ", name)); + add_args_to_usage(cl->args, &s); if (cl->extra_arg) { - gpr_asprintf(&tmp, " [%s...]", cl->extra_arg_name); - gpr_strvec_add(&s, tmp); + s.push_back(absl::StrFormat(" [%s...]", cl->extra_arg_name)); } - gpr_strvec_add(&s, gpr_strdup("\n")); - - tmp = gpr_strvec_flatten(&s, nullptr); - gpr_strvec_destroy(&s); - return tmp; + s.push_back("\n"); + return absl::StrJoin(s, ""); } static int print_usage_and_die(gpr_cmdline* cl) { - char* usage = gpr_cmdline_usage_string(cl, cl->argv0); - fprintf(stderr, "%s", usage); - gpr_free(usage); + fprintf(stderr, "%s", gpr_cmdline_usage_string(cl, cl->argv0).c_str()); if (!cl->survive_failure) { exit(1); } diff --git a/test/core/util/cmdline.h b/test/core/util/cmdline.h index 3ae35d6e6af..773563dcbe9 100644 --- a/test/core/util/cmdline.h +++ b/test/core/util/cmdline.h @@ -21,6 +21,8 @@ #include +#include + /** Simple command line parser. Supports flags that can be specified as -foo, --foo, --no-foo, -no-foo, etc @@ -75,6 +77,6 @@ int gpr_cmdline_parse(gpr_cmdline* cl, int argc, char** argv); /** Destroy the parser */ void gpr_cmdline_destroy(gpr_cmdline* cl); /** Get a string describing usage */ -char* gpr_cmdline_usage_string(gpr_cmdline* cl, const char* argv0); +std::string gpr_cmdline_usage_string(gpr_cmdline* cl, const char* argv0); #endif /* GRPC_TEST_CORE_UTIL_CMDLINE_H */ diff --git a/test/core/util/cmdline_test.cc b/test/core/util/cmdline_test.cc index 59e1bbff34e..c15e5e36df9 100644 --- a/test/core/util/cmdline_test.cc +++ b/test/core/util/cmdline_test.cc @@ -307,7 +307,6 @@ static void test_extra_dashdash(void) { static void test_usage(void) { gpr_cmdline* cl; - char* usage; const char* str = nullptr; int x = 0; @@ -322,17 +321,15 @@ static void test_usage(void) { gpr_cmdline_on_extra_arg(cl, "file", "filenames to process", extra_arg_cb, nullptr); - usage = gpr_cmdline_usage_string(cl, "test"); - GPR_ASSERT(0 == strcmp(usage, - "Usage: test [--str=string] [--x=int] " - "[--flag|--no-flag] [file...]\n")); - gpr_free(usage); + std::string usage = gpr_cmdline_usage_string(cl, "test"); + GPR_ASSERT(usage == + "Usage: test [--str=string] [--x=int] " + "[--flag|--no-flag] [file...]\n"); usage = gpr_cmdline_usage_string(cl, "/foo/test"); - GPR_ASSERT(0 == strcmp(usage, - "Usage: test [--str=string] [--x=int] " - "[--flag|--no-flag] [file...]\n")); - gpr_free(usage); + GPR_ASSERT(usage == + "Usage: test [--str=string] [--x=int] " + "[--flag|--no-flag] [file...]\n"); gpr_cmdline_destroy(cl); } diff --git a/test/cpp/end2end/client_callback_end2end_test.cc b/test/cpp/end2end/client_callback_end2end_test.cc index c83d6409db0..4f8bfeba372 100644 --- a/test/cpp/end2end/client_callback_end2end_test.cc +++ b/test/cpp/end2end/client_callback_end2end_test.cc @@ -16,6 +16,12 @@ * */ +#include +#include +#include +#include +#include + #include #include #include @@ -25,14 +31,6 @@ #include #include #include -#include - -#include -#include -#include -#include -#include -#include #include "src/core/lib/gpr/env.h" #include "src/core/lib/iomgr/iomgr.h" @@ -45,6 +43,8 @@ #include "test/cpp/util/string_ref_helper.h" #include "test/cpp/util/test_credentials_provider.h" +#include + // MAYBE_SKIP_TEST is a macro to determine if this particular test configuration // should be skipped based on a decision made at SetUp time. In particular, any // callback tests can only be run if the iomgr can run in the background or if @@ -1079,8 +1079,7 @@ class BidiClient public: BidiClient(grpc::testing::EchoTestService::Stub* stub, ServerTryCancelRequestPhase server_try_cancel, - int num_msgs_to_send, bool cork_metadata, bool first_write_async, - ClientCancelInfo client_cancel = {}) + int num_msgs_to_send, ClientCancelInfo client_cancel = {}) : server_try_cancel_(server_try_cancel), msgs_to_send_{num_msgs_to_send}, client_cancel_{client_cancel} { @@ -1090,9 +1089,8 @@ class BidiClient grpc::to_string(server_try_cancel)); } request_.set_message("Hello fren "); - context_.set_initial_metadata_corked(cork_metadata); stub->experimental_async()->BidiStream(&context_, this); - MaybeAsyncWrite(first_write_async); + MaybeWrite(); StartRead(&response_); StartCall(); } @@ -1113,10 +1111,6 @@ class BidiClient } } void OnWriteDone(bool ok) override { - if (async_write_thread_.joinable()) { - async_write_thread_.join(); - RemoveHold(); - } if (server_try_cancel_ == DO_NOT_CANCEL) { EXPECT_TRUE(ok); } else if (!ok) { @@ -1181,26 +1175,6 @@ class BidiClient } private: - void MaybeAsyncWrite(bool first_write_async) { - if (first_write_async) { - // Make sure that we have a write to issue. - // TODO(vjpai): Make this work with 0 writes case as well. - assert(msgs_to_send_ >= 1); - - AddHold(); - async_write_thread_ = std::thread([this] { - std::unique_lock lock(async_write_thread_mu_); - async_write_thread_cv_.wait( - lock, [this] { return async_write_thread_start_; }); - MaybeWrite(); - }); - std::lock_guard lock(async_write_thread_mu_); - async_write_thread_start_ = true; - async_write_thread_cv_.notify_one(); - return; - } - MaybeWrite(); - } void MaybeWrite() { if (client_cancel_.cancel && writes_complete_ == client_cancel_.ops_before_cancel) { @@ -1222,57 +1196,13 @@ class BidiClient std::mutex mu_; std::condition_variable cv_; bool done_ = false; - std::thread async_write_thread_; - bool async_write_thread_start_ = false; - std::mutex async_write_thread_mu_; - std::condition_variable async_write_thread_cv_; }; TEST_P(ClientCallbackEnd2endTest, BidiStream) { MAYBE_SKIP_TEST; ResetStub(); - BidiClient test(stub_.get(), DO_NOT_CANCEL, - kServerDefaultResponseStreamsToSend, - /*cork_metadata=*/false, /*first_write_async=*/false); - test.Await(); - // Make sure that the server interceptors were not notified of a cancel - if (GetParam().use_interceptors) { - EXPECT_EQ(0, DummyInterceptor::GetNumTimesCancel()); - } -} - -TEST_P(ClientCallbackEnd2endTest, BidiStreamFirstWriteAsync) { - MAYBE_SKIP_TEST; - ResetStub(); - BidiClient test(stub_.get(), DO_NOT_CANCEL, - kServerDefaultResponseStreamsToSend, - /*cork_metadata=*/false, /*first_write_async=*/true); - test.Await(); - // Make sure that the server interceptors were not notified of a cancel - if (GetParam().use_interceptors) { - EXPECT_EQ(0, DummyInterceptor::GetNumTimesCancel()); - } -} - -TEST_P(ClientCallbackEnd2endTest, BidiStreamCorked) { - MAYBE_SKIP_TEST; - ResetStub(); - BidiClient test(stub_.get(), DO_NOT_CANCEL, - kServerDefaultResponseStreamsToSend, - /*cork_metadata=*/true, /*first_write_async=*/false); - test.Await(); - // Make sure that the server interceptors were not notified of a cancel - if (GetParam().use_interceptors) { - EXPECT_EQ(0, DummyInterceptor::GetNumTimesCancel()); - } -} - -TEST_P(ClientCallbackEnd2endTest, BidiStreamCorkedFirstWriteAsync) { - MAYBE_SKIP_TEST; - ResetStub(); - BidiClient test(stub_.get(), DO_NOT_CANCEL, - kServerDefaultResponseStreamsToSend, - /*cork_metadata=*/true, /*first_write_async=*/true); + BidiClient test{stub_.get(), DO_NOT_CANCEL, + kServerDefaultResponseStreamsToSend}; test.Await(); // Make sure that the server interceptors were not notified of a cancel if (GetParam().use_interceptors) { @@ -1283,10 +1213,8 @@ TEST_P(ClientCallbackEnd2endTest, BidiStreamCorkedFirstWriteAsync) { TEST_P(ClientCallbackEnd2endTest, ClientCancelsBidiStream) { MAYBE_SKIP_TEST; ResetStub(); - BidiClient test(stub_.get(), DO_NOT_CANCEL, - kServerDefaultResponseStreamsToSend, - /*cork_metadata=*/false, /*first_write_async=*/false, - ClientCancelInfo(2)); + BidiClient test{stub_.get(), DO_NOT_CANCEL, + kServerDefaultResponseStreamsToSend, ClientCancelInfo{2}}; test.Await(); // Make sure that the server interceptors were notified of a cancel if (GetParam().use_interceptors) { @@ -1298,8 +1226,7 @@ TEST_P(ClientCallbackEnd2endTest, ClientCancelsBidiStream) { TEST_P(ClientCallbackEnd2endTest, BidiStreamServerCancelBefore) { MAYBE_SKIP_TEST; ResetStub(); - BidiClient test(stub_.get(), CANCEL_BEFORE_PROCESSING, /*num_msgs_to_send=*/2, - /*cork_metadata=*/false, /*first_write_async=*/false); + BidiClient test{stub_.get(), CANCEL_BEFORE_PROCESSING, 2}; test.Await(); // Make sure that the server interceptors were notified if (GetParam().use_interceptors) { @@ -1312,9 +1239,7 @@ TEST_P(ClientCallbackEnd2endTest, BidiStreamServerCancelBefore) { TEST_P(ClientCallbackEnd2endTest, BidiStreamServerCancelDuring) { MAYBE_SKIP_TEST; ResetStub(); - BidiClient test(stub_.get(), CANCEL_DURING_PROCESSING, - /*num_msgs_to_send=*/10, /*cork_metadata=*/false, - /*first_write_async=*/false); + BidiClient test{stub_.get(), CANCEL_DURING_PROCESSING, 10}; test.Await(); // Make sure that the server interceptors were notified if (GetParam().use_interceptors) { @@ -1327,8 +1252,7 @@ TEST_P(ClientCallbackEnd2endTest, BidiStreamServerCancelDuring) { TEST_P(ClientCallbackEnd2endTest, BidiStreamServerCancelAfter) { MAYBE_SKIP_TEST; ResetStub(); - BidiClient test(stub_.get(), CANCEL_AFTER_PROCESSING, /*num_msgs_to_send=*/5, - /*cork_metadata=*/false, /*first_write_async=*/false); + BidiClient test{stub_.get(), CANCEL_AFTER_PROCESSING, 5}; test.Await(); // Make sure that the server interceptors were notified if (GetParam().use_interceptors) { diff --git a/test/cpp/end2end/xds_end2end_test.cc b/test/cpp/end2end/xds_end2end_test.cc index 050c6909c5e..271ddae52f3 100644 --- a/test/cpp/end2end/xds_end2end_test.cc +++ b/test/cpp/end2end/xds_end2end_test.cc @@ -922,7 +922,10 @@ class AdsServiceImpl : public AggregatedDiscoveryService::Service, const SubscriptionNameMap& subscription_name_map, const std::set& resources_added_to_response, DiscoveryResponse* response) { - resource_type_response_state_[resource_type].state = ResponseState::SENT; + auto& response_state = resource_type_response_state_[resource_type]; + if (response_state.state == ResponseState::NOT_SENT) { + response_state.state = ResponseState::SENT; + } response->set_type_url(resource_type); response->set_version_info(absl::StrCat(version)); response->set_nonce(absl::StrCat(version)); @@ -1201,8 +1204,7 @@ class XdsEnd2endTest : public ::testing::TestWithParam { if (!expected_targets.empty()) { args.SetString(GRPC_ARG_FAKE_SECURITY_EXPECTED_TARGETS, expected_targets); } - grpc::string scheme = - GetParam().use_xds_resolver() ? "xds-experimental" : "fake"; + grpc::string scheme = GetParam().use_xds_resolver() ? "xds" : "fake"; std::ostringstream uri; uri << scheme << ":///" << kApplicationTargetName_; // TODO(dgq): templatize tests to run everything using both secure and @@ -2986,11 +2988,16 @@ TEST_P(LdsRdsTest, XdsRoutingWeightedCluster) { (1 - kErrorTolerance)), ::testing::Le(kNumEcho1Rpcs * kWeight75 / 100 * (1 + kErrorTolerance)))); + // TODO: (@donnadionne) Reduce tolerance: increased the tolerance to keep the + // test from flaking while debugging potential root cause. + const double kErrorToleranceSmallLoad = 0.3; + gpr_log(GPR_INFO, "target_75 received %d rpcs and target_25 received %d rpcs", + weight_75_request_count, weight_25_request_count); EXPECT_THAT(weight_25_request_count, ::testing::AllOf(::testing::Ge(kNumEcho1Rpcs * kWeight25 / 100 * - (1 - kErrorTolerance)), + (1 - kErrorToleranceSmallLoad)), ::testing::Le(kNumEcho1Rpcs * kWeight25 / 100 * - (1 + kErrorTolerance)))); + (1 + kErrorToleranceSmallLoad)))); gpr_unsetenv("GRPC_XDS_EXPERIMENTAL_ROUTING"); } @@ -3057,11 +3064,16 @@ TEST_P(LdsRdsTest, RouteActionWeightedTargetDefaultRoute) { (1 - kErrorTolerance)), ::testing::Le(kNumEchoRpcs * kWeight75 / 100 * (1 + kErrorTolerance)))); + // TODO: (@donnadionne) Reduce tolerance: increased the tolerance to keep the + // test from flaking while debugging potential root cause. + const double kErrorToleranceSmallLoad = 0.3; + gpr_log(GPR_INFO, "target_75 received %d rpcs and target_25 received %d rpcs", + weight_75_request_count, weight_25_request_count); EXPECT_THAT(weight_25_request_count, ::testing::AllOf(::testing::Ge(kNumEchoRpcs * kWeight25 / 100 * - (1 - kErrorTolerance)), + (1 - kErrorToleranceSmallLoad)), ::testing::Le(kNumEchoRpcs * kWeight25 / 100 * - (1 + kErrorTolerance)))); + (1 + kErrorToleranceSmallLoad)))); } TEST_P(LdsRdsTest, XdsRoutingWeightedClusterUpdateWeights) { @@ -3150,11 +3162,16 @@ TEST_P(LdsRdsTest, XdsRoutingWeightedClusterUpdateWeights) { (1 - kErrorTolerance)), ::testing::Le(kNumEcho1Rpcs * kWeight75 / 100 * (1 + kErrorTolerance)))); + // TODO: (@donnadionne) Reduce tolerance: increased the tolerance to keep the + // test from flaking while debugging potential root cause. + const double kErrorToleranceSmallLoad = 0.3; + gpr_log(GPR_INFO, "target_75 received %d rpcs and target_25 received %d rpcs", + weight_75_request_count, weight_25_request_count); EXPECT_THAT(weight_25_request_count, ::testing::AllOf(::testing::Ge(kNumEcho1Rpcs * kWeight25 / 100 * - (1 - kErrorTolerance)), + (1 - kErrorToleranceSmallLoad)), ::testing::Le(kNumEcho1Rpcs * kWeight25 / 100 * - (1 + kErrorTolerance)))); + (1 + kErrorToleranceSmallLoad)))); // Change Route Configurations: same clusters different weights. weighted_cluster1->mutable_weight()->set_value(kWeight50); weighted_cluster2->mutable_weight()->set_value(kWeight50); @@ -3275,11 +3292,16 @@ TEST_P(LdsRdsTest, XdsRoutingWeightedClusterUpdateClusters) { (1 - kErrorTolerance)), ::testing::Le(kNumEcho1Rpcs * kWeight75 / 100 * (1 + kErrorTolerance)))); + // TODO: (@donnadionne) Reduce tolerance: increased the tolerance to keep the + // test from flaking while debugging potential root cause. + const double kErrorToleranceSmallLoad = 0.3; + gpr_log(GPR_INFO, "target_75 received %d rpcs and target_25 received %d rpcs", + weight_75_request_count, weight_25_request_count); EXPECT_THAT(weight_25_request_count, ::testing::AllOf(::testing::Ge(kNumEcho1Rpcs * kWeight25 / 100 * - (1 - kErrorTolerance)), + (1 - kErrorToleranceSmallLoad)), ::testing::Le(kNumEcho1Rpcs * kWeight25 / 100 * - (1 + kErrorTolerance)))); + (1 + kErrorToleranceSmallLoad)))); // Change Route Configurations: new set of clusters with different weights. weighted_cluster1->mutable_weight()->set_value(kWeight50); weighted_cluster2->set_name(kNewCluster2Name); @@ -3333,11 +3355,15 @@ TEST_P(LdsRdsTest, XdsRoutingWeightedClusterUpdateClusters) { (1 - kErrorTolerance)), ::testing::Le(kNumEcho1Rpcs * kWeight75 / 100 * (1 + kErrorTolerance)))); + // TODO: (@donnadionne) Reduce tolerance: increased the tolerance to keep the + // test from flaking while debugging potential root cause. + gpr_log(GPR_INFO, "target_75 received %d rpcs and target_25 received %d rpcs", + weight_75_request_count, weight_25_request_count); EXPECT_THAT(weight_25_request_count, ::testing::AllOf(::testing::Ge(kNumEcho1Rpcs * kWeight25 / 100 * - (1 - kErrorTolerance)), + (1 - kErrorToleranceSmallLoad)), ::testing::Le(kNumEcho1Rpcs * kWeight25 / 100 * - (1 + kErrorTolerance)))); + (1 + kErrorToleranceSmallLoad)))); gpr_unsetenv("GRPC_XDS_EXPERIMENTAL_ROUTING"); } diff --git a/test/cpp/naming/BUILD b/test/cpp/naming/BUILD index b35c37c0349..dd2dac7ee12 100644 --- a/test/cpp/naming/BUILD +++ b/test/cpp/naming/BUILD @@ -38,6 +38,7 @@ grpc_cc_test( name = "cancel_ares_query_test", srcs = ["cancel_ares_query_test.cc"], external_deps = ["gtest"], + flaky = True, # TODO(b/157516105) deps = [ ":dns_test_util", "//:gpr", diff --git a/test/cpp/naming/gen_build_yaml.py b/test/cpp/naming/gen_build_yaml.py index 9829116c2e6..59d12be92a9 100755 --- a/test/cpp/naming/gen_build_yaml.py +++ b/test/cpp/naming/gen_build_yaml.py @@ -44,6 +44,8 @@ def _resolver_test_cases(resolver_component_data): target_name, 'arg_names_and_values': [ ('target_name', target_name), + ('do_ordered_address_comparison', + test_case['do_ordered_address_comparison']), ('expected_addrs', _build_expected_addrs_cmd_arg(test_case['expected_addrs'])), ('expected_chosen_service_config', diff --git a/test/cpp/naming/resolver_component_test.cc b/test/cpp/naming/resolver_component_test.cc index 350806d6aa7..cb065a3cb75 100644 --- a/test/cpp/naming/resolver_component_test.cc +++ b/test/cpp/naming/resolver_component_test.cc @@ -87,6 +87,13 @@ using namespace google; using namespace gflags; DEFINE_string(target_name, "", "Target name to resolve."); +DEFINE_string(do_ordered_address_comparison, "", + "Whether or not to compare resolved addresses to expected " + "addresses using an ordered comparison. This is useful for " + "testing certain behaviors that involve sorting of resolved " + "addresses. Note it would be better if this argument was a " + "bool flag, but it's a string for ease of invocation from " + "the generated python test runner."); DEFINE_string(expected_addrs, "", "List of expected backend or balancer addresses in the form " "',;,;...'. " @@ -484,8 +491,18 @@ class CheckingResultHandler : public ResultHandler { found_lb_addrs.size(), args->expected_addrs.size()); abort(); } - EXPECT_THAT(args->expected_addrs, - UnorderedElementsAreArray(found_lb_addrs)); + if (FLAGS_do_ordered_address_comparison == "True") { + EXPECT_EQ(args->expected_addrs, found_lb_addrs); + } else if (FLAGS_do_ordered_address_comparison == "False") { + EXPECT_THAT(args->expected_addrs, + UnorderedElementsAreArray(found_lb_addrs)); + } else { + gpr_log(GPR_ERROR, + "Invalid for setting for --do_ordered_address_comparison. " + "Have %s, want True or False", + FLAGS_do_ordered_address_comparison.c_str()); + GPR_ASSERT(0); + } const char* service_config_json = result.service_config == nullptr ? nullptr diff --git a/test/cpp/naming/resolver_component_tests_runner.py b/test/cpp/naming/resolver_component_tests_runner.py index e5ff9cd294a..bb544533320 100755 --- a/test/cpp/naming/resolver_component_tests_runner.py +++ b/test/cpp/naming/resolver_component_tests_runner.py @@ -122,6 +122,7 @@ test_runner_log('Run test with target: %s' % 'no-srv-ipv4-single-target.resolver current_test_subprocess = subprocess.Popen([ args.test_bin_path, '--target_name', 'no-srv-ipv4-single-target.resolver-tests-version-4.grpctestingexp.', + '--do_ordered_address_comparison', 'False', '--expected_addrs', '5.5.5.5:443,False', '--expected_chosen_service_config', '', '--expected_service_config_error', '', @@ -138,6 +139,7 @@ test_runner_log('Run test with target: %s' % 'srv-ipv4-single-target.resolver-te current_test_subprocess = subprocess.Popen([ args.test_bin_path, '--target_name', 'srv-ipv4-single-target.resolver-tests-version-4.grpctestingexp.', + '--do_ordered_address_comparison', 'False', '--expected_addrs', '1.2.3.4:1234,True', '--expected_chosen_service_config', '', '--expected_service_config_error', '', @@ -154,6 +156,7 @@ test_runner_log('Run test with target: %s' % 'srv-ipv4-multi-target.resolver-tes current_test_subprocess = subprocess.Popen([ args.test_bin_path, '--target_name', 'srv-ipv4-multi-target.resolver-tests-version-4.grpctestingexp.', + '--do_ordered_address_comparison', 'False', '--expected_addrs', '1.2.3.5:1234,True;1.2.3.6:1234,True;1.2.3.7:1234,True', '--expected_chosen_service_config', '', '--expected_service_config_error', '', @@ -170,6 +173,7 @@ test_runner_log('Run test with target: %s' % 'srv-ipv6-single-target.resolver-te current_test_subprocess = subprocess.Popen([ args.test_bin_path, '--target_name', 'srv-ipv6-single-target.resolver-tests-version-4.grpctestingexp.', + '--do_ordered_address_comparison', 'False', '--expected_addrs', '[2607:f8b0:400a:801::1001]:1234,True', '--expected_chosen_service_config', '', '--expected_service_config_error', '', @@ -186,6 +190,7 @@ test_runner_log('Run test with target: %s' % 'srv-ipv6-multi-target.resolver-tes current_test_subprocess = subprocess.Popen([ args.test_bin_path, '--target_name', 'srv-ipv6-multi-target.resolver-tests-version-4.grpctestingexp.', + '--do_ordered_address_comparison', 'False', '--expected_addrs', '[2607:f8b0:400a:801::1002]:1234,True;[2607:f8b0:400a:801::1003]:1234,True;[2607:f8b0:400a:801::1004]:1234,True', '--expected_chosen_service_config', '', '--expected_service_config_error', '', @@ -202,6 +207,7 @@ test_runner_log('Run test with target: %s' % 'srv-ipv4-simple-service-config.res current_test_subprocess = subprocess.Popen([ args.test_bin_path, '--target_name', 'srv-ipv4-simple-service-config.resolver-tests-version-4.grpctestingexp.', + '--do_ordered_address_comparison', 'False', '--expected_addrs', '1.2.3.4:1234,True', '--expected_chosen_service_config', '{"loadBalancingPolicy":"round_robin","methodConfig":[{"name":[{"method":"Foo","service":"SimpleService"}],"waitForReady":true}]}', '--expected_service_config_error', '', @@ -218,6 +224,7 @@ test_runner_log('Run test with target: %s' % 'ipv4-no-srv-simple-service-config. current_test_subprocess = subprocess.Popen([ args.test_bin_path, '--target_name', 'ipv4-no-srv-simple-service-config.resolver-tests-version-4.grpctestingexp.', + '--do_ordered_address_comparison', 'False', '--expected_addrs', '1.2.3.4:443,False', '--expected_chosen_service_config', '{"loadBalancingPolicy":"round_robin","methodConfig":[{"name":[{"method":"Foo","service":"NoSrvSimpleService"}],"waitForReady":true}]}', '--expected_service_config_error', '', @@ -234,6 +241,7 @@ test_runner_log('Run test with target: %s' % 'ipv4-no-config-for-cpp.resolver-te current_test_subprocess = subprocess.Popen([ args.test_bin_path, '--target_name', 'ipv4-no-config-for-cpp.resolver-tests-version-4.grpctestingexp.', + '--do_ordered_address_comparison', 'False', '--expected_addrs', '1.2.3.4:443,False', '--expected_chosen_service_config', '', '--expected_service_config_error', '', @@ -250,6 +258,7 @@ test_runner_log('Run test with target: %s' % 'ipv4-cpp-config-has-zero-percentag current_test_subprocess = subprocess.Popen([ args.test_bin_path, '--target_name', 'ipv4-cpp-config-has-zero-percentage.resolver-tests-version-4.grpctestingexp.', + '--do_ordered_address_comparison', 'False', '--expected_addrs', '1.2.3.4:443,False', '--expected_chosen_service_config', '', '--expected_service_config_error', '', @@ -266,6 +275,7 @@ test_runner_log('Run test with target: %s' % 'ipv4-second-language-is-cpp.resolv current_test_subprocess = subprocess.Popen([ args.test_bin_path, '--target_name', 'ipv4-second-language-is-cpp.resolver-tests-version-4.grpctestingexp.', + '--do_ordered_address_comparison', 'False', '--expected_addrs', '1.2.3.4:443,False', '--expected_chosen_service_config', '{"loadBalancingPolicy":"round_robin","methodConfig":[{"name":[{"method":"Foo","service":"CppService"}],"waitForReady":true}]}', '--expected_service_config_error', '', @@ -282,6 +292,7 @@ test_runner_log('Run test with target: %s' % 'ipv4-config-with-percentages.resol current_test_subprocess = subprocess.Popen([ args.test_bin_path, '--target_name', 'ipv4-config-with-percentages.resolver-tests-version-4.grpctestingexp.', + '--do_ordered_address_comparison', 'False', '--expected_addrs', '1.2.3.4:443,False', '--expected_chosen_service_config', '{"loadBalancingPolicy":"round_robin","methodConfig":[{"name":[{"method":"Foo","service":"AlwaysPickedService"}],"waitForReady":true}]}', '--expected_service_config_error', '', @@ -298,6 +309,7 @@ test_runner_log('Run test with target: %s' % 'srv-ipv4-target-has-backend-and-ba current_test_subprocess = subprocess.Popen([ args.test_bin_path, '--target_name', 'srv-ipv4-target-has-backend-and-balancer.resolver-tests-version-4.grpctestingexp.', + '--do_ordered_address_comparison', 'False', '--expected_addrs', '1.2.3.4:1234,True;1.2.3.4:443,False', '--expected_chosen_service_config', '', '--expected_service_config_error', '', @@ -314,6 +326,7 @@ test_runner_log('Run test with target: %s' % 'srv-ipv6-target-has-backend-and-ba current_test_subprocess = subprocess.Popen([ args.test_bin_path, '--target_name', 'srv-ipv6-target-has-backend-and-balancer.resolver-tests-version-4.grpctestingexp.', + '--do_ordered_address_comparison', 'False', '--expected_addrs', '[2607:f8b0:400a:801::1002]:1234,True;[2607:f8b0:400a:801::1002]:443,False', '--expected_chosen_service_config', '', '--expected_service_config_error', '', @@ -330,6 +343,7 @@ test_runner_log('Run test with target: %s' % 'ipv4-config-causing-fallback-to-tc current_test_subprocess = subprocess.Popen([ args.test_bin_path, '--target_name', 'ipv4-config-causing-fallback-to-tcp.resolver-tests-version-4.grpctestingexp.', + '--do_ordered_address_comparison', 'False', '--expected_addrs', '1.2.3.4:443,False', '--expected_chosen_service_config', '{"loadBalancingPolicy":"round_robin","methodConfig":[{"name":[{"method":"Foo","service":"SimpleService"}],"waitForReady":true},{"name":[{"method":"FooTwo","service":"SimpleService"}],"waitForReady":true},{"name":[{"method":"FooThree","service":"SimpleService"}],"waitForReady":true},{"name":[{"method":"FooFour","service":"SimpleService"}],"waitForReady":true},{"name":[{"method":"FooFive","service":"SimpleService"}],"waitForReady":true},{"name":[{"method":"FooSix","service":"SimpleService"}],"waitForReady":true},{"name":[{"method":"FooSeven","service":"SimpleService"}],"waitForReady":true},{"name":[{"method":"FooEight","service":"SimpleService"}],"waitForReady":true},{"name":[{"method":"FooNine","service":"SimpleService"}],"waitForReady":true},{"name":[{"method":"FooTen","service":"SimpleService"}],"waitForReady":true},{"name":[{"method":"FooEleven","service":"SimpleService"}],"waitForReady":true},{"name":[{"method":"FooTwelve","service":"SimpleService"}],"waitForReady":true},{"name":[{"method":"FooThirteen","service":"SimpleService"}],"waitForReady":true},{"name":[{"method":"FooFourteen","service":"SimpleService"}],"waitForReady":true},{"name":[{"method":"FooFifteen","service":"SimpleService"}],"waitForReady":true}]}', '--expected_service_config_error', '', @@ -346,6 +360,7 @@ test_runner_log('Run test with target: %s' % 'srv-ipv4-single-target-srv-disable current_test_subprocess = subprocess.Popen([ args.test_bin_path, '--target_name', 'srv-ipv4-single-target-srv-disabled.resolver-tests-version-4.grpctestingexp.', + '--do_ordered_address_comparison', 'False', '--expected_addrs', '2.3.4.5:443,False', '--expected_chosen_service_config', '', '--expected_service_config_error', '', @@ -362,6 +377,7 @@ test_runner_log('Run test with target: %s' % 'srv-ipv4-multi-target-srv-disabled current_test_subprocess = subprocess.Popen([ args.test_bin_path, '--target_name', 'srv-ipv4-multi-target-srv-disabled.resolver-tests-version-4.grpctestingexp.', + '--do_ordered_address_comparison', 'False', '--expected_addrs', '9.2.3.5:443,False;9.2.3.6:443,False;9.2.3.7:443,False', '--expected_chosen_service_config', '', '--expected_service_config_error', '', @@ -378,6 +394,7 @@ test_runner_log('Run test with target: %s' % 'srv-ipv6-single-target-srv-disable current_test_subprocess = subprocess.Popen([ args.test_bin_path, '--target_name', 'srv-ipv6-single-target-srv-disabled.resolver-tests-version-4.grpctestingexp.', + '--do_ordered_address_comparison', 'False', '--expected_addrs', '[2600::1001]:443,False', '--expected_chosen_service_config', '', '--expected_service_config_error', '', @@ -394,6 +411,7 @@ test_runner_log('Run test with target: %s' % 'srv-ipv6-multi-target-srv-disabled current_test_subprocess = subprocess.Popen([ args.test_bin_path, '--target_name', 'srv-ipv6-multi-target-srv-disabled.resolver-tests-version-4.grpctestingexp.', + '--do_ordered_address_comparison', 'False', '--expected_addrs', '[2600::1002]:443,False;[2600::1003]:443,False;[2600::1004]:443,False', '--expected_chosen_service_config', '', '--expected_service_config_error', '', @@ -410,6 +428,7 @@ test_runner_log('Run test with target: %s' % 'srv-ipv4-simple-service-config-srv current_test_subprocess = subprocess.Popen([ args.test_bin_path, '--target_name', 'srv-ipv4-simple-service-config-srv-disabled.resolver-tests-version-4.grpctestingexp.', + '--do_ordered_address_comparison', 'False', '--expected_addrs', '5.5.3.4:443,False', '--expected_chosen_service_config', '{"loadBalancingPolicy":"round_robin","methodConfig":[{"name":[{"method":"Foo","service":"SimpleService"}],"waitForReady":true}]}', '--expected_service_config_error', '', @@ -426,6 +445,7 @@ test_runner_log('Run test with target: %s' % 'srv-ipv4-simple-service-config-txt current_test_subprocess = subprocess.Popen([ args.test_bin_path, '--target_name', 'srv-ipv4-simple-service-config-txt-disabled.resolver-tests-version-4.grpctestingexp.', + '--do_ordered_address_comparison', 'False', '--expected_addrs', '1.2.3.4:1234,True', '--expected_chosen_service_config', '', '--expected_service_config_error', '', @@ -442,6 +462,7 @@ test_runner_log('Run test with target: %s' % 'ipv4-cpp-config-has-zero-percentag current_test_subprocess = subprocess.Popen([ args.test_bin_path, '--target_name', 'ipv4-cpp-config-has-zero-percentage-txt-disabled.resolver-tests-version-4.grpctestingexp.', + '--do_ordered_address_comparison', 'False', '--expected_addrs', '1.2.3.4:443,False', '--expected_chosen_service_config', '', '--expected_service_config_error', '', @@ -458,6 +479,7 @@ test_runner_log('Run test with target: %s' % 'ipv4-second-language-is-cpp-txt-di current_test_subprocess = subprocess.Popen([ args.test_bin_path, '--target_name', 'ipv4-second-language-is-cpp-txt-disabled.resolver-tests-version-4.grpctestingexp.', + '--do_ordered_address_comparison', 'False', '--expected_addrs', '1.2.3.4:443,False', '--expected_chosen_service_config', '', '--expected_service_config_error', '', @@ -474,6 +496,7 @@ test_runner_log('Run test with target: %s' % 'ipv4-svc_cfg_bad_json.resolver-tes current_test_subprocess = subprocess.Popen([ args.test_bin_path, '--target_name', 'ipv4-svc_cfg_bad_json.resolver-tests-version-4.grpctestingexp.', + '--do_ordered_address_comparison', 'False', '--expected_addrs', '1.2.3.4:443,False', '--expected_chosen_service_config', '', '--expected_service_config_error', 'JSON parse error', @@ -490,6 +513,7 @@ test_runner_log('Run test with target: %s' % 'ipv4-svc_cfg_bad_client_language.r current_test_subprocess = subprocess.Popen([ args.test_bin_path, '--target_name', 'ipv4-svc_cfg_bad_client_language.resolver-tests-version-4.grpctestingexp.', + '--do_ordered_address_comparison', 'False', '--expected_addrs', '1.2.3.4:443,False', '--expected_chosen_service_config', '', '--expected_service_config_error', 'field:clientLanguage error:should be of type array', @@ -506,6 +530,7 @@ test_runner_log('Run test with target: %s' % 'ipv4-svc_cfg_bad_percentage.resolv current_test_subprocess = subprocess.Popen([ args.test_bin_path, '--target_name', 'ipv4-svc_cfg_bad_percentage.resolver-tests-version-4.grpctestingexp.', + '--do_ordered_address_comparison', 'False', '--expected_addrs', '1.2.3.4:443,False', '--expected_chosen_service_config', '', '--expected_service_config_error', 'field:percentage error:should be of type number', @@ -522,6 +547,7 @@ test_runner_log('Run test with target: %s' % 'ipv4-svc_cfg_bad_wait_for_ready.re current_test_subprocess = subprocess.Popen([ args.test_bin_path, '--target_name', 'ipv4-svc_cfg_bad_wait_for_ready.resolver-tests-version-4.grpctestingexp.', + '--do_ordered_address_comparison', 'False', '--expected_addrs', '1.2.3.4:443,False', '--expected_chosen_service_config', '', '--expected_service_config_error', 'field:waitForReady error:Type should be true/false', @@ -538,6 +564,7 @@ test_runner_log('Run test with target: %s' % 'no-srv-ipv4-single-target-inject-b current_test_subprocess = subprocess.Popen([ args.test_bin_path, '--target_name', 'no-srv-ipv4-single-target-inject-broken-nameservers.resolver-tests-version-4.grpctestingexp.', + '--do_ordered_address_comparison', 'False', '--expected_addrs', '5.5.5.5:443,False', '--expected_chosen_service_config', '', '--expected_service_config_error', '', @@ -554,6 +581,7 @@ test_runner_log('Run test with target: %s' % 'ipv4-config-causing-fallback-to-tc current_test_subprocess = subprocess.Popen([ args.test_bin_path, '--target_name', 'ipv4-config-causing-fallback-to-tcp-inject-broken-nameservers.resolver-tests-version-4.grpctestingexp.', + '--do_ordered_address_comparison', 'False', '--expected_addrs', '1.2.3.4:443,False', '--expected_chosen_service_config', '{"loadBalancingPolicy":["round_robin"]}', '--expected_service_config_error', 'field:loadBalancingPolicy error:type should be string', @@ -566,6 +594,23 @@ current_test_subprocess.communicate() if current_test_subprocess.returncode != 0: num_test_failures += 1 +test_runner_log('Run test with target: %s' % 'load-balanced-name-with-dualstack-balancer.resolver-tests-version-4.grpctestingexp.') +current_test_subprocess = subprocess.Popen([ + args.test_bin_path, + '--target_name', 'load-balanced-name-with-dualstack-balancer.resolver-tests-version-4.grpctestingexp.', + '--do_ordered_address_comparison', 'True', + '--expected_addrs', '[::1]:1234,True;[2002::1111]:1234,True', + '--expected_chosen_service_config', '', + '--expected_service_config_error', '', + '--expected_lb_policy', '', + '--enable_srv_queries', 'True', + '--enable_txt_queries', 'True', + '--inject_broken_nameserver_list', 'False', + '--local_dns_server_address', '127.0.0.1:%d' % args.dns_server_port]) +current_test_subprocess.communicate() +if current_test_subprocess.returncode != 0: + num_test_failures += 1 + test_runner_log('now kill DNS server') dns_server_subprocess.kill() dns_server_subprocess.wait() diff --git a/test/cpp/naming/resolver_test_record_groups.yaml b/test/cpp/naming/resolver_test_record_groups.yaml index c44c39faecc..af89fb73637 100644 --- a/test/cpp/naming/resolver_test_record_groups.yaml +++ b/test/cpp/naming/resolver_test_record_groups.yaml @@ -3,6 +3,7 @@ resolver_component_tests: # Tests for which we enable SRV queries - expected_addrs: - {address: '5.5.5.5:443', is_balancer: false} + do_ordered_address_comparison: false expected_chosen_service_config: null expected_service_config_error: null expected_lb_policy: null @@ -15,6 +16,7 @@ resolver_component_tests: - {TTL: '2100', data: 5.5.5.5, type: A} - expected_addrs: - {address: '1.2.3.4:1234', is_balancer: true} + do_ordered_address_comparison: false expected_chosen_service_config: null expected_service_config_error: null expected_lb_policy: null @@ -31,6 +33,7 @@ resolver_component_tests: - {address: '1.2.3.5:1234', is_balancer: true} - {address: '1.2.3.6:1234', is_balancer: true} - {address: '1.2.3.7:1234', is_balancer: true} + do_ordered_address_comparison: false expected_chosen_service_config: null expected_service_config_error: null expected_lb_policy: null @@ -47,6 +50,7 @@ resolver_component_tests: - {TTL: '2100', data: 1.2.3.7, type: A} - expected_addrs: - {address: '[2607:f8b0:400a:801::1001]:1234', is_balancer: true} + do_ordered_address_comparison: false expected_chosen_service_config: null expected_service_config_error: null expected_lb_policy: null @@ -63,6 +67,7 @@ resolver_component_tests: - {address: '[2607:f8b0:400a:801::1002]:1234', is_balancer: true} - {address: '[2607:f8b0:400a:801::1003]:1234', is_balancer: true} - {address: '[2607:f8b0:400a:801::1004]:1234', is_balancer: true} + do_ordered_address_comparison: false expected_chosen_service_config: null expected_service_config_error: null expected_lb_policy: null @@ -79,6 +84,7 @@ resolver_component_tests: - {TTL: '2100', data: '2607:f8b0:400a:801::1004', type: AAAA} - expected_addrs: - {address: '1.2.3.4:1234', is_balancer: true} + do_ordered_address_comparison: false expected_chosen_service_config: '{"loadBalancingPolicy":"round_robin","methodConfig":[{"name":[{"method":"Foo","service":"SimpleService"}],"waitForReady":true}]}' expected_service_config_error: null expected_lb_policy: round_robin @@ -96,6 +102,7 @@ resolver_component_tests: type: TXT} - expected_addrs: - {address: '1.2.3.4:443', is_balancer: false} + do_ordered_address_comparison: false expected_chosen_service_config: '{"loadBalancingPolicy":"round_robin","methodConfig":[{"name":[{"method":"Foo","service":"NoSrvSimpleService"}],"waitForReady":true}]}' expected_service_config_error: null expected_lb_policy: round_robin @@ -111,6 +118,7 @@ resolver_component_tests: type: TXT} - expected_addrs: - {address: '1.2.3.4:443', is_balancer: false} + do_ordered_address_comparison: false expected_chosen_service_config: null expected_service_config_error: null expected_lb_policy: null @@ -126,6 +134,7 @@ resolver_component_tests: type: TXT} - expected_addrs: - {address: '1.2.3.4:443', is_balancer: false} + do_ordered_address_comparison: false expected_chosen_service_config: null expected_service_config_error: null expected_lb_policy: null @@ -141,6 +150,7 @@ resolver_component_tests: type: TXT} - expected_addrs: - {address: '1.2.3.4:443', is_balancer: false} + do_ordered_address_comparison: false expected_chosen_service_config: '{"loadBalancingPolicy":"round_robin","methodConfig":[{"name":[{"method":"Foo","service":"CppService"}],"waitForReady":true}]}' expected_service_config_error: null expected_lb_policy: round_robin @@ -156,6 +166,7 @@ resolver_component_tests: type: TXT} - expected_addrs: - {address: '1.2.3.4:443', is_balancer: false} + do_ordered_address_comparison: false expected_chosen_service_config: '{"loadBalancingPolicy":"round_robin","methodConfig":[{"name":[{"method":"Foo","service":"AlwaysPickedService"}],"waitForReady":true}]}' expected_service_config_error: null expected_lb_policy: round_robin @@ -172,6 +183,7 @@ resolver_component_tests: - expected_addrs: - {address: '1.2.3.4:1234', is_balancer: true} - {address: '1.2.3.4:443', is_balancer: false} + do_ordered_address_comparison: false expected_chosen_service_config: null expected_service_config_error: null expected_lb_policy: null @@ -189,6 +201,7 @@ resolver_component_tests: - expected_addrs: - {address: '[2607:f8b0:400a:801::1002]:1234', is_balancer: true} - {address: '[2607:f8b0:400a:801::1002]:443', is_balancer: false} + do_ordered_address_comparison: false expected_chosen_service_config: null expected_service_config_error: null expected_lb_policy: null @@ -205,6 +218,7 @@ resolver_component_tests: - {TTL: '2100', data: '2607:f8b0:400a:801::1002', type: AAAA} - expected_addrs: - {address: '1.2.3.4:443', is_balancer: false} + do_ordered_address_comparison: false expected_chosen_service_config: '{"loadBalancingPolicy":"round_robin","methodConfig":[{"name":[{"method":"Foo","service":"SimpleService"}],"waitForReady":true},{"name":[{"method":"FooTwo","service":"SimpleService"}],"waitForReady":true},{"name":[{"method":"FooThree","service":"SimpleService"}],"waitForReady":true},{"name":[{"method":"FooFour","service":"SimpleService"}],"waitForReady":true},{"name":[{"method":"FooFive","service":"SimpleService"}],"waitForReady":true},{"name":[{"method":"FooSix","service":"SimpleService"}],"waitForReady":true},{"name":[{"method":"FooSeven","service":"SimpleService"}],"waitForReady":true},{"name":[{"method":"FooEight","service":"SimpleService"}],"waitForReady":true},{"name":[{"method":"FooNine","service":"SimpleService"}],"waitForReady":true},{"name":[{"method":"FooTen","service":"SimpleService"}],"waitForReady":true},{"name":[{"method":"FooEleven","service":"SimpleService"}],"waitForReady":true},{"name":[{"method":"FooTwelve","service":"SimpleService"}],"waitForReady":true},{"name":[{"method":"FooThirteen","service":"SimpleService"}],"waitForReady":true},{"name":[{"method":"FooFourteen","service":"SimpleService"}],"waitForReady":true},{"name":[{"method":"FooFifteen","service":"SimpleService"}],"waitForReady":true}]}' expected_service_config_error: null expected_lb_policy: null @@ -221,6 +235,7 @@ resolver_component_tests: # Tests for which we don't enable SRV queries - expected_addrs: - {address: '2.3.4.5:443', is_balancer: false} + do_ordered_address_comparison: false expected_chosen_service_config: null expected_service_config_error: null expected_lb_policy: null @@ -239,6 +254,7 @@ resolver_component_tests: - {address: '9.2.3.5:443', is_balancer: false} - {address: '9.2.3.6:443', is_balancer: false} - {address: '9.2.3.7:443', is_balancer: false} + do_ordered_address_comparison: false expected_chosen_service_config: null expected_service_config_error: null expected_lb_policy: null @@ -259,6 +275,7 @@ resolver_component_tests: - {TTL: '2100', data: 9.2.3.7, type: A} - expected_addrs: - {address: '[2600::1001]:443', is_balancer: false} + do_ordered_address_comparison: false expected_chosen_service_config: null expected_service_config_error: null expected_lb_policy: null @@ -277,6 +294,7 @@ resolver_component_tests: - {address: '[2600::1002]:443', is_balancer: false} - {address: '[2600::1003]:443', is_balancer: false} - {address: '[2600::1004]:443', is_balancer: false} + do_ordered_address_comparison: false expected_chosen_service_config: null expected_service_config_error: null expected_lb_policy: null @@ -297,6 +315,7 @@ resolver_component_tests: - {TTL: '2100', data: '2600::1004', type: AAAA} - expected_addrs: - {address: '5.5.3.4:443', is_balancer: false} + do_ordered_address_comparison: false expected_chosen_service_config: '{"loadBalancingPolicy":"round_robin","methodConfig":[{"name":[{"method":"Foo","service":"SimpleService"}],"waitForReady":true}]}' expected_service_config_error: null expected_lb_policy: round_robin @@ -316,6 +335,7 @@ resolver_component_tests: type: TXT} - expected_addrs: - {address: '1.2.3.4:1234', is_balancer: true} + do_ordered_address_comparison: false expected_chosen_service_config: null expected_service_config_error: null expected_lb_policy: null @@ -333,6 +353,7 @@ resolver_component_tests: type: TXT} - expected_addrs: - {address: '1.2.3.4:443', is_balancer: false} + do_ordered_address_comparison: false expected_chosen_service_config: null expected_service_config_error: null expected_lb_policy: null @@ -348,6 +369,7 @@ resolver_component_tests: type: TXT} - expected_addrs: - {address: '1.2.3.4:443', is_balancer: false} + do_ordered_address_comparison: false expected_chosen_service_config: null expected_service_config_error: null expected_lb_policy: null @@ -363,6 +385,7 @@ resolver_component_tests: type: TXT} - expected_addrs: - {address: '1.2.3.4:443', is_balancer: false} + do_ordered_address_comparison: false expected_chosen_service_config: null expected_service_config_error: 'JSON parse error' expected_lb_policy: null @@ -378,6 +401,7 @@ resolver_component_tests: type: TXT} - expected_addrs: - {address: '1.2.3.4:443', is_balancer: false} + do_ordered_address_comparison: false expected_chosen_service_config: null expected_service_config_error: 'field:clientLanguage error:should be of type array' expected_lb_policy: null @@ -393,6 +417,7 @@ resolver_component_tests: type: TXT} - expected_addrs: - {address: '1.2.3.4:443', is_balancer: false} + do_ordered_address_comparison: false expected_chosen_service_config: null expected_service_config_error: 'field:percentage error:should be of type number' expected_lb_policy: null @@ -408,6 +433,7 @@ resolver_component_tests: type: TXT} - expected_addrs: - {address: '1.2.3.4:443', is_balancer: false} + do_ordered_address_comparison: false expected_chosen_service_config: null expected_service_config_error: 'field:waitForReady error:Type should be true/false' expected_lb_policy: null @@ -420,10 +446,11 @@ resolver_component_tests: - {TTL: '2100', data: 1.2.3.4, type: A} _grpc_config.ipv4-svc_cfg_bad_wait_for_ready: - {TTL: '2100', data: 'grpc_config=[{"serviceConfig":{"methodConfig":[{"name":[{"method":"Foo","service":"CppService"}],"waitForReady":"true"}]}}]', - type: TXT} + type: TXT} # Tests for which we also exercise the resolver's ability to skip past a broken DNS server in its nameserver list - expected_addrs: - {address: '5.5.5.5:443', is_balancer: false} + do_ordered_address_comparison: false expected_chosen_service_config: null expected_service_config_error: null expected_lb_policy: null @@ -436,6 +463,7 @@ resolver_component_tests: - {TTL: '2100', data: 5.5.5.5, type: A} - expected_addrs: - {address: '1.2.3.4:443', is_balancer: false} + do_ordered_address_comparison: false expected_chosen_service_config: '{"loadBalancingPolicy":["round_robin"]}' expected_service_config_error: 'field:loadBalancingPolicy error:type should be string' expected_lb_policy: null @@ -449,3 +477,26 @@ resolver_component_tests: _grpc_config.ipv4-config-causing-fallback-to-tcp-inject-broken-nameservers: - {TTL: '2100', data: 'grpc_config=[{"serviceConfig":{"loadBalancingPolicy":["round_robin"]}}]', type: TXT} +# This tests that gRPCLB addresses are sorted properly per RFC 6724. Note +# that the only assumption that this makes is that the machine that the +# test runs on has a functioning IPv6 loopback (which by the RFC should +# always be preferred). Note too that the ordering of the AAAA records +# listed under the dualstack-balancer name is important in order to +# actually test this sorting. +- expected_addrs: + - {address: '[::1]:1234', is_balancer: true} + - {address: '[2002::1111]:1234', is_balancer: true} + do_ordered_address_comparison: true + expected_chosen_service_config: null + expected_service_config_error: null + expected_lb_policy: null + enable_srv_queries: true + enable_txt_queries: true + inject_broken_nameserver_list: false + record_to_resolve: load-balanced-name-with-dualstack-balancer + records: + _grpclb._tcp.load-balanced-name-with-dualstack-balancer: + - {TTL: '2100', data: 0 0 1234 dualstack-balancer, type: SRV} + dualstack-balancer: + - {TTL: '2100', data: '2002::1111', type: AAAA} + - {TTL: '2100', data: '::1', type: AAAA} diff --git a/test/cpp/util/cli_credentials.cc b/test/cpp/util/cli_credentials.cc index 1f7af81a331..207bef13825 100644 --- a/test/cpp/util/cli_credentials.cc +++ b/test/cpp/util/cli_credentials.cc @@ -47,10 +47,14 @@ DEFINE_string( ssl_client_key, "", "If not empty, load this PEM formatted private key. Requires use of " "--ssl_client_cert"); +DEFINE_string( + local_connect_type, "local_tcp", + "The type of local connections for which local channel credentials will " + "be applied. Should be local_tcp or uds."); DEFINE_string( channel_creds_type, "", - "The channel creds type: insecure, ssl, gdc (Google Default Credentials) " - "or alts."); + "The channel creds type: insecure, ssl, gdc (Google Default Credentials), " + "alts, or local."); DEFINE_string( call_creds, "", "Call credentials to use: none (default), or access_token=. If " @@ -138,10 +142,20 @@ CliCredentials::GetChannelCredentials() const { } else if (FLAGS_channel_creds_type.compare("alts") == 0) { return grpc::experimental::AltsCredentials( grpc::experimental::AltsCredentialsOptions()); + } else if (FLAGS_channel_creds_type.compare("local") == 0) { + if (FLAGS_local_connect_type.compare("local_tcp") == 0) { + return grpc::experimental::LocalCredentials(LOCAL_TCP); + } else if (FLAGS_local_connect_type.compare("uds") == 0) { + return grpc::experimental::LocalCredentials(UDS); + } else { + fprintf(stderr, + "--local_connect_type=%s invalid; must be local_tcp or uds.\n", + FLAGS_local_connect_type.c_str()); + } } fprintf(stderr, - "--channel_creds_type=%s invalid; must be insecure, ssl, gdc or " - "alts.\n", + "--channel_creds_type=%s invalid; must be insecure, ssl, gdc, " + "alts, or local.\n", FLAGS_channel_creds_type.c_str()); return std::shared_ptr(); } @@ -203,7 +217,8 @@ std::shared_ptr CliCredentials::GetCredentials() } const grpc::string CliCredentials::GetCredentialUsage() const { - return " --enable_ssl ; Set whether to use ssl (deprecated)\n" + return " --enable_ssl ; Set whether to use ssl " + "(deprecated)\n" " --use_auth ; Set whether to create default google" " credentials\n" " ; (deprecated)\n" @@ -213,7 +228,9 @@ const grpc::string CliCredentials::GetCredentialUsage() const { " --ssl_target ; Set server host for ssl validation\n" " --ssl_client_cert ; Client cert for ssl\n" " --ssl_client_key ; Client private key for ssl\n" - " --channel_creds_type ; Set to insecure, ssl, gdc, or alts\n" + " --local_connect_type ; Set to local_tcp or uds\n" + " --channel_creds_type ; Set to insecure, ssl, gdc, alts, or " + "local\n" " --call_creds ; Set to none, or" " access_token=\n"; } diff --git a/test/cpp/util/grpc_cli.cc b/test/cpp/util/grpc_cli.cc index a4035c57758..663b44b9f56 100644 --- a/test/cpp/util/grpc_cli.cc +++ b/test/cpp/util/grpc_cli.cc @@ -51,6 +51,9 @@ --decode=grpc.testing.SimpleResponse \ src/proto/grpc/testing/messages.proto \ < output.bin > output.txt + 10. --default_service_config, optional default service config to use + on the channel. Note that this may be ignored if the name resolver + returns a service config. */ #include diff --git a/test/cpp/util/grpc_tool.cc b/test/cpp/util/grpc_tool.cc index 053fd3862c0..a7bdb2b54ff 100644 --- a/test/cpp/util/grpc_tool.cc +++ b/test/cpp/util/grpc_tool.cc @@ -57,6 +57,11 @@ DEFINE_string(proto_path, ".", "Path to look for the proto file."); DEFINE_string(protofiles, "", "Name of the proto file."); DEFINE_bool(binary_input, false, "Input in binary format"); DEFINE_bool(binary_output, false, "Output in binary format"); +DEFINE_string( + default_service_config, "", + "Default service config to use on the channel, if non-empty. Note " + "that this will be ignored if the name resolver returns a service " + "config."); DEFINE_bool(json_input, false, "Input in json format"); DEFINE_bool(json_output, false, "Output in json format"); DEFINE_string(infile, "", "Input file (default is stdin)"); @@ -217,6 +222,10 @@ std::shared_ptr CreateCliChannel( if (!cred.GetSslTargetNameOverride().empty()) { args.SetSslTargetNameOverride(cred.GetSslTargetNameOverride()); } + if (!FLAGS_default_service_config.empty()) { + args.SetString(GRPC_ARG_SERVICE_CONFIG, + FLAGS_default_service_config.c_str()); + } return ::grpc::CreateCustomChannel(server_address, cred.GetCredentials(), args); } diff --git a/test/cpp/util/grpc_tool_test.cc b/test/cpp/util/grpc_tool_test.cc index 175f16c911c..5d15e8c183a 100644 --- a/test/cpp/util/grpc_tool_test.cc +++ b/test/cpp/util/grpc_tool_test.cc @@ -118,6 +118,7 @@ DECLARE_bool(batch); DECLARE_string(metadata); DECLARE_string(protofiles); DECLARE_string(proto_path); +DECLARE_string(default_service_config); namespace { @@ -1192,6 +1193,28 @@ TEST_F(GrpcToolTest, ListCommand_OverrideSslHostName) { ShutdownServer(); } +TEST_F(GrpcToolTest, ConfiguringDefaultServiceConfig) { + // Test input "grpc_cli list localhost: + // --default_service_config={\"loadBalancingConfig\":[{\"pick_first\":{}}]}" + std::stringstream output_stream; + const grpc::string server_address = SetUpServer(); + const char* argv[] = {"grpc_cli", "ls", server_address.c_str()}; + // Just check that the tool is still operational when --default_service_config + // is configured. This particular service config is in reality redundant with + // the channel's default configuration. + FLAGS_l = false; + FLAGS_default_service_config = + "{\"loadBalancingConfig\":[{\"pick_first\":{}}]}"; + EXPECT_TRUE(0 == GrpcToolMainLib(ArraySize(argv), argv, TestCliCredentials(), + std::bind(PrintStream, &output_stream, + std::placeholders::_1))); + FLAGS_default_service_config = ""; + EXPECT_TRUE(0 == strcmp(output_stream.str().c_str(), + "grpc.testing.EchoTestService\n" + "grpc.reflection.v1alpha.ServerReflection\n")); + ShutdownServer(); +} + } // namespace testing } // namespace grpc diff --git a/tools/distrib/python/grpc_prefixed/.gitignore b/tools/distrib/python/grpc_prefixed/.gitignore new file mode 100644 index 00000000000..9d0b71a3c79 --- /dev/null +++ b/tools/distrib/python/grpc_prefixed/.gitignore @@ -0,0 +1,2 @@ +build +dist diff --git a/tools/distrib/python/grpc_prefixed/generate.py b/tools/distrib/python/grpc_prefixed/generate.py new file mode 100644 index 00000000000..864971f5c16 --- /dev/null +++ b/tools/distrib/python/grpc_prefixed/generate.py @@ -0,0 +1,140 @@ +# Copyright 2020 The 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. +"""Generates grpc-prefixed packages using template renderer. + +To use this script, please use 3.7+ interpreter. This script is work-directory +agnostic. A quick executable command: + + python3 tools/distrib/python/grpc_prefixed/generate.py +""" + +import dataclasses +import datetime +import logging +import os +import shutil +import subprocess +import sys + +import jinja2 + +WORK_PATH = os.path.realpath(os.path.dirname(__file__)) +LICENSE = os.path.join(WORK_PATH, '../../../../LICENSE') +BUILD_PATH = os.path.join(WORK_PATH, 'build') +DIST_PATH = os.path.join(WORK_PATH, 'dist') + +env = jinja2.Environment( + loader=jinja2.FileSystemLoader(os.path.join(WORK_PATH, 'templates'))) + +LOGGER = logging.getLogger(__name__) +POPEN_TIMEOUT_S = datetime.timedelta(minutes=1).total_seconds() + + +@dataclasses.dataclass +class PackageMeta: + """Meta-info of a PyPI package.""" + name: str + name_long: str + destination_package: str + version: str = '1.0.0' + + +def clean() -> None: + try: + shutil.rmtree(BUILD_PATH) + except FileNotFoundError: + pass + + try: + shutil.rmtree(DIST_PATH) + except FileNotFoundError: + pass + + +def generate_package(meta: PackageMeta) -> None: + # Makes package directory + package_path = os.path.join(BUILD_PATH, meta.name) + os.makedirs(package_path, exist_ok=True) + + # Copy license + shutil.copyfile(LICENSE, os.path.join(package_path, 'LICENSE')) + + # Generates source code + for template_name in env.list_templates(): + template = env.get_template(template_name) + with open( + os.path.join(package_path, + template_name.replace('.template', '')), 'w') as f: + f.write(template.render(dataclasses.asdict(meta))) + + # Creates wheel + job = subprocess.Popen([ + sys.executable, + os.path.join(package_path, 'setup.py'), 'sdist', '--dist-dir', DIST_PATH + ], + cwd=package_path, + stdout=subprocess.PIPE, + stderr=subprocess.STDOUT) + outs, _ = job.communicate(timeout=POPEN_TIMEOUT_S) + + # Logs result + if job.returncode != 0: + LOGGER.error('Wheel creation failed with %d', job.returncode) + LOGGER.error(outs) + else: + LOGGER.info('Package <%s> generated', meta.name) + + +def main(): + clean() + + generate_package( + PackageMeta(name='grpc', + name_long='gRPC Python', + destination_package='grpcio')) + + generate_package( + PackageMeta(name='grpc-status', + name_long='gRPC Rich Error Status', + destination_package='grpcio-status')) + + generate_package( + PackageMeta(name='grpc-channelz', + name_long='gRPC Channel Tracing', + destination_package='grpcio-channelz')) + + generate_package( + PackageMeta(name='grpc-tools', + name_long='ProtoBuf Code Generator', + destination_package='grpcio-tools')) + + generate_package( + PackageMeta(name='grpc-reflection', + name_long='gRPC Reflection', + destination_package='grpcio-reflection')) + + generate_package( + PackageMeta(name='grpc-testing', + name_long='gRPC Testing Utility', + destination_package='grpcio-testing')) + + generate_package( + PackageMeta(name='grpc-health-checking', + name_long='gRPC Health Checking', + destination_package='grpcio-health-checking')) + + +if __name__ == "__main__": + logging.basicConfig(level=logging.INFO) + main() diff --git a/tools/distrib/python/grpc_prefixed/templates/MANIFEST.in.template b/tools/distrib/python/grpc_prefixed/templates/MANIFEST.in.template new file mode 100644 index 00000000000..5840309d09c --- /dev/null +++ b/tools/distrib/python/grpc_prefixed/templates/MANIFEST.in.template @@ -0,0 +1,4 @@ +include setup.py +include README.rst +include LICENSE +global-exclude *.pyc diff --git a/tools/distrib/python/grpc_prefixed/templates/README.rst.template b/tools/distrib/python/grpc_prefixed/templates/README.rst.template new file mode 100644 index 00000000000..299ba9f59a2 --- /dev/null +++ b/tools/distrib/python/grpc_prefixed/templates/README.rst.template @@ -0,0 +1,2 @@ +The official package of {{ name_long }} is `{{ destination_package }} `_. +Please download that package instead. diff --git a/tools/distrib/python/grpc_prefixed/templates/setup.py.template b/tools/distrib/python/grpc_prefixed/templates/setup.py.template new file mode 100644 index 00000000000..2d62270d04d --- /dev/null +++ b/tools/distrib/python/grpc_prefixed/templates/setup.py.template @@ -0,0 +1,45 @@ +# Copyright 2020 The 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. +"""Setup module for the {{ name_long }} package.""" + +import sys + +import setuptools + +CLASSIFIERS = [ + 'Development Status :: 7 - Inactive', + 'Programming Language :: Python', + 'Programming Language :: Python :: 2', + 'Programming Language :: Python :: 3', + 'License :: OSI Approved :: Apache Software License', +] + + +HINT = 'Please install the official package with: pip install {{ destination_package }}' + + +if 'sdist' not in sys.argv: + raise RuntimeError(HINT) + + +setuptools.setup( + name='{{ name }}', + version='{{ version }}', + description=HINT, + author='The gRPC Authors', + author_email='grpc-io@googlegroups.com', + url='https://grpc.io', + license='Apache License 2.0', + classifiers=CLASSIFIERS +) diff --git a/tools/internal_ci/linux/grpc_xds_bazel_python_test_in_docker.sh b/tools/internal_ci/linux/grpc_xds_bazel_python_test_in_docker.sh index a9a74eef377..12729543bdb 100755 --- a/tools/internal_ci/linux/grpc_xds_bazel_python_test_in_docker.sh +++ b/tools/internal_ci/linux/grpc_xds_bazel_python_test_in_docker.sh @@ -56,4 +56,4 @@ GRPC_VERBOSITY=debug GRPC_TRACE=xds_client,xds_resolver,cds_lb,eds_lb,priority_l --path_to_server_binary=/java_server/grpc-java/interop-testing/build/install/grpc-interop-testing/bin/xds-test-server \ --gcp_suffix=$(date '+%s') \ --verbose \ - --client_cmd='bazel run //src/python/grpcio_tests/tests_py3_only/interop:xds_interop_client -- --server=xds-experimental:///{server_uri} --stats_port={stats_port} --qps={qps} --verbose' + --client_cmd='bazel run //src/python/grpcio_tests/tests_py3_only/interop:xds_interop_client -- --server=xds:///{server_uri} --stats_port={stats_port} --qps={qps} --verbose' diff --git a/tools/internal_ci/linux/grpc_xds_bazel_test_in_docker.sh b/tools/internal_ci/linux/grpc_xds_bazel_test_in_docker.sh index 54ade769e75..667dc7c7e36 100755 --- a/tools/internal_ci/linux/grpc_xds_bazel_test_in_docker.sh +++ b/tools/internal_ci/linux/grpc_xds_bazel_test_in_docker.sh @@ -56,4 +56,4 @@ GRPC_VERBOSITY=debug GRPC_TRACE=xds_client,xds_resolver,cds_lb,eds_lb,priority_l --path_to_server_binary=/java_server/grpc-java/interop-testing/build/install/grpc-interop-testing/bin/xds-test-server \ --gcp_suffix=$(date '+%s') \ --verbose \ - --client_cmd='bazel-bin/test/cpp/interop/xds_interop_client --server=xds-experimental:///{server_uri} --stats_port={stats_port} --qps={qps} {fail_on_failed_rpc}' + --client_cmd='bazel-bin/test/cpp/interop/xds_interop_client --server=xds:///{server_uri} --stats_port={stats_port} --qps={qps} {fail_on_failed_rpc}' diff --git a/tools/internal_ci/linux/grpc_xds_csharp_test_in_docker.sh b/tools/internal_ci/linux/grpc_xds_csharp_test_in_docker.sh index c5eef3be572..03ecbc4b32c 100755 --- a/tools/internal_ci/linux/grpc_xds_csharp_test_in_docker.sh +++ b/tools/internal_ci/linux/grpc_xds_csharp_test_in_docker.sh @@ -56,4 +56,4 @@ GRPC_VERBOSITY=debug GRPC_TRACE=xds_client,xds_resolver,cds_lb,eds_lb,priority_l --path_to_server_binary=/java_server/grpc-java/interop-testing/build/install/grpc-interop-testing/bin/xds-test-server \ --gcp_suffix=$(date '+%s') \ --verbose \ - --client_cmd='dotnet exec src/csharp/Grpc.IntegrationTesting.XdsClient/bin/Release/netcoreapp2.1/Grpc.IntegrationTesting.XdsClient.dll -- --server=xds-experimental:///{server_uri} --stats_port={stats_port} --qps={qps}' + --client_cmd='dotnet exec src/csharp/Grpc.IntegrationTesting.XdsClient/bin/Release/netcoreapp2.1/Grpc.IntegrationTesting.XdsClient.dll -- --server=xds:///{server_uri} --stats_port={stats_port} --qps={qps}' diff --git a/tools/internal_ci/linux/grpc_xds_php_test_in_docker.sh b/tools/internal_ci/linux/grpc_xds_php_test_in_docker.sh index 0d971d7b69b..c0bbcea535b 100755 --- a/tools/internal_ci/linux/grpc_xds_php_test_in_docker.sh +++ b/tools/internal_ci/linux/grpc_xds_php_test_in_docker.sh @@ -70,4 +70,4 @@ GRPC_VERBOSITY=debug GRPC_TRACE=xds_client,xds_resolver,cds_lb,eds_lb,priority_l --gcp_suffix=$(date '+%s') \ --only_stable_gcp_apis \ --verbose \ - --client_cmd='php -d extension=grpc.so -d extension=pthreads.so src/php/tests/interop/xds_client.php --server=xds-experimental:///{server_uri} --stats_port={stats_port} --qps={qps}' + --client_cmd='php -d extension=grpc.so -d extension=pthreads.so src/php/tests/interop/xds_client.php --server=xds:///{server_uri} --stats_port={stats_port} --qps={qps}' diff --git a/tools/internal_ci/linux/grpc_xds_ruby_test_in_docker.sh b/tools/internal_ci/linux/grpc_xds_ruby_test_in_docker.sh index 2cf3d46d0d3..8d643e3ad25 100644 --- a/tools/internal_ci/linux/grpc_xds_ruby_test_in_docker.sh +++ b/tools/internal_ci/linux/grpc_xds_ruby_test_in_docker.sh @@ -57,4 +57,4 @@ GRPC_VERBOSITY=debug GRPC_TRACE=xds_client,xds_resolver,cds_lb,eds_lb,priority_l --gcp_suffix=$(date '+%s') \ --only_stable_gcp_apis \ --verbose \ - --client_cmd='ruby src/ruby/pb/test/xds_client.rb --server=xds-experimental:///{server_uri} --stats_port={stats_port} --qps={qps}' + --client_cmd='ruby src/ruby/pb/test/xds_client.rb --server=xds:///{server_uri} --stats_port={stats_port} --qps={qps}' diff --git a/tools/run_tests/generated/tests.json b/tools/run_tests/generated/tests.json index b395729d7c7..d07c55a4df3 100644 --- a/tools/run_tests/generated/tests.json +++ b/tools/run_tests/generated/tests.json @@ -809,30 +809,6 @@ ], "uses_polling": false }, - { - "args": [], - "benchmark": false, - "ci_platforms": [ - "linux", - "mac", - "posix", - "windows" - ], - "cpu_cost": 1.0, - "exclude_configs": [], - "exclude_iomgrs": [], - "flaky": false, - "gtest": false, - "language": "c", - "name": "concurrent_connectivity_test", - "platforms": [ - "linux", - "mac", - "posix", - "windows" - ], - "uses_polling": true - }, { "args": [], "benchmark": false, @@ -3905,30 +3881,6 @@ ], "uses_polling": false }, - { - "args": [], - "benchmark": false, - "ci_platforms": [ - "linux", - "mac", - "posix", - "windows" - ], - "cpu_cost": 1.0, - "exclude_configs": [], - "exclude_iomgrs": [], - "flaky": false, - "gtest": true, - "language": "c++", - "name": "cancel_ares_query_test", - "platforms": [ - "linux", - "mac", - "posix", - "windows" - ], - "uses_polling": true - }, { "args": [], "benchmark": false, diff --git a/tools/run_tests/run_xds_tests.py b/tools/run_tests/run_xds_tests.py index 7459104f986..cd23a233417 100755 --- a/tools/run_tests/run_xds_tests.py +++ b/tools/run_tests/run_xds_tests.py @@ -52,7 +52,7 @@ _TEST_CASES = [ 'round_robin', 'secondary_locality_gets_no_requests_on_partial_primary_failure', 'secondary_locality_gets_requests_on_primary_failure', - 'traffic_splitting', + # 'traffic_splitting', ] @@ -1087,11 +1087,9 @@ def patch_url_map_backend_service(gcp, 'backendService': service.url, 'weight': w, } for service, w in services_with_weights.items()] - } if services_withWeights else None + } if services_with_weights else None config = { - 'defaultService': - backend_service.url, 'pathMatchers': [{ 'name': _PATH_MATCHER_NAME, 'defaultService': default_service,