From abe3cf51f02d48b28fd67d78e7a3060d763148b5 Mon Sep 17 00:00:00 2001 From: Guantao Liu Date: Wed, 12 Jul 2017 17:36:30 -0700 Subject: [PATCH 01/16] Add a new metric 'Queries/CPU-sec'. Enable internal credential types. --- src/proto/grpc/testing/control.proto | 5 ++++ test/cpp/qps/BUILD | 2 ++ test/cpp/qps/client.h | 14 ++++++++-- test/cpp/qps/driver.cc | 36 ++++++++++++++++++++----- test/cpp/qps/driver.h | 5 ++-- test/cpp/qps/qps_json_driver.cc | 12 ++++++--- test/cpp/qps/qps_worker.cc | 8 ++++-- test/cpp/qps/qps_worker.h | 3 ++- test/cpp/qps/report.cc | 21 +++++++++++++++ test/cpp/qps/report.h | 7 +++++ test/cpp/qps/server.h | 15 ++++++----- test/cpp/qps/worker.cc | 5 +++- test/cpp/util/create_test_channel.cc | 40 +++++++++++++++++++++------- test/cpp/util/create_test_channel.h | 6 +++++ 14 files changed, 146 insertions(+), 33 deletions(-) diff --git a/src/proto/grpc/testing/control.proto b/src/proto/grpc/testing/control.proto index 48016642f74..2ff2e4e8a28 100644 --- a/src/proto/grpc/testing/control.proto +++ b/src/proto/grpc/testing/control.proto @@ -64,6 +64,7 @@ message LoadParams { message SecurityParams { bool use_test_ca = 1; string server_host_override = 2; + string cred_type = 3; } message ChannelArg { @@ -240,6 +241,10 @@ message ScenarioResultSummary // Number of polls called inside completion queue per request double client_polls_per_request = 15; double server_polls_per_request = 16; + + // Queries per CPU-sec over all servers or clients + double server_queries_per_cpu_sec = 17; + double client_queries_per_cpu_sec = 18; } // Results of a single benchmark scenario. diff --git a/test/cpp/qps/BUILD b/test/cpp/qps/BUILD index 4d8b3b40e3c..a3f70cf8b5b 100644 --- a/test/cpp/qps/BUILD +++ b/test/cpp/qps/BUILD @@ -77,6 +77,7 @@ grpc_cc_library( "//src/proto/grpc/testing:services_proto", "//test/core/util:gpr_test_util", "//test/core/util:grpc_test_util", + "//test/cpp/util:test_util", ], ) @@ -141,6 +142,7 @@ grpc_cc_binary( ":benchmark_config", ":driver_impl", "//:grpc++", + "//test/cpp/util:test_config", ], external_deps = [ "gflags", diff --git a/test/cpp/qps/client.h b/test/cpp/qps/client.h index e759446c1ae..f47f378a984 100644 --- a/test/cpp/qps/client.h +++ b/test/cpp/qps/client.h @@ -39,6 +39,7 @@ #include "test/cpp/qps/interarrival.h" #include "test/cpp/qps/usage_timer.h" #include "test/cpp/util/create_test_channel.h" +#include "test/cpp/util/test_credentials_provider.h" namespace grpc { namespace testing { @@ -407,9 +408,18 @@ class ClientImpl : public Client { ChannelArguments args; args.SetInt("shard_to_ensure_no_subchannel_merges", shard); set_channel_args(config, &args); + + grpc::string type; + if (config.has_security_params() && + config.security_params().cred_type().empty()) { + type = kTlsCredentialsType; + } else { + type = config.security_params().cred_type(); + } + channel_ = CreateTestChannel( - target, config.security_params().server_host_override(), - config.has_security_params(), !config.security_params().use_test_ca(), + target, type, config.security_params().server_host_override(), + !config.security_params().use_test_ca(), std::shared_ptr(), args); gpr_log(GPR_INFO, "Connecting to %s", target.c_str()); GPR_ASSERT(channel_->WaitForConnected( diff --git a/test/cpp/qps/driver.cc b/test/cpp/qps/driver.cc index fbd8d1b1e76..bb4e7536e84 100644 --- a/test/cpp/qps/driver.cc +++ b/test/cpp/qps/driver.cc @@ -40,6 +40,7 @@ #include "test/cpp/qps/histogram.h" #include "test/cpp/qps/qps_worker.h" #include "test/cpp/qps/stats.h" +#include "test/cpp/util/test_credentials_provider.h" using std::list; using std::thread; @@ -172,13 +173,27 @@ static void postprocess_scenario_result(ScenarioResult* result) { sum(result->client_stats(), CliPollCount) / histogram.Count()); result->mutable_summary()->set_server_polls_per_request( sum(result->server_stats(), SvrPollCount) / histogram.Count()); + + auto server_queries_per_cpu_sec = + histogram.Count() / + (sum(result->server_stats(), ServerSystemTime) + + sum(result->server_stats(), ServerUserTime)); + auto client_queries_per_cpu_sec = + histogram.Count() / + (sum(result->client_stats(), SystemTime) + + sum(result->client_stats(), UserTime)); + + result->mutable_summary()->set_server_queries_per_cpu_sec( + server_queries_per_cpu_sec); + result->mutable_summary()->set_client_queries_per_cpu_sec( + client_queries_per_cpu_sec); } std::unique_ptr RunScenario( const ClientConfig& initial_client_config, size_t num_clients, const ServerConfig& initial_server_config, size_t num_servers, int warmup_seconds, int benchmark_seconds, int spawn_local_worker_count, - const char* qps_server_target_override) { + const char* qps_server_target_override, const char* credential_type) { // Log everything from the driver gpr_set_log_verbosity(GPR_LOG_SEVERITY_DEBUG); @@ -214,7 +229,7 @@ std::unique_ptr RunScenario( } int driver_port = grpc_pick_unused_port_or_die(); - local_workers.emplace_back(new QpsWorker(driver_port)); + local_workers.emplace_back(new QpsWorker(driver_port, 0, credential_type)); char addr[256]; sprintf(addr, "localhost:%d", driver_port); if (spawn_local_worker_count < 0) { @@ -246,12 +261,15 @@ std::unique_ptr RunScenario( }; std::vector servers(num_servers); std::unordered_map> hosts_cores; + ChannelArguments channel_args; for (size_t i = 0; i < num_servers; i++) { gpr_log(GPR_INFO, "Starting server on %s (worker #%" PRIuPTR ")", workers[i].c_str(), i); servers[i].stub = WorkerService::NewStub( - CreateChannel(workers[i], InsecureChannelCredentials())); + CreateChannel(workers[i], + GetCredentialsProvider()->GetChannelCredentials( + credential_type, &channel_args))); ServerConfig server_config = initial_server_config; if (server_config.core_limit() != 0) { @@ -298,7 +316,9 @@ std::unique_ptr RunScenario( gpr_log(GPR_INFO, "Starting client on %s (worker #%" PRIuPTR ")", worker.c_str(), i + num_servers); clients[i].stub = WorkerService::NewStub( - CreateChannel(worker, InsecureChannelCredentials())); + CreateChannel(worker, + GetCredentialsProvider()->GetChannelCredentials( + credential_type, &channel_args))); ClientConfig per_client_config = client_config; if (initial_client_config.core_limit() != 0) { @@ -483,16 +503,20 @@ std::unique_ptr RunScenario( return result; } -bool RunQuit() { +bool RunQuit(const char* credential_type) { // Get client, server lists bool result = true; auto workers = get_workers("QPS_WORKERS"); if (workers.size() == 0) { return false; } + + ChannelArguments channel_args; for (size_t i = 0; i < workers.size(); i++) { auto stub = WorkerService::NewStub( - CreateChannel(workers[i], InsecureChannelCredentials())); + CreateChannel(workers[i], + GetCredentialsProvider()->GetChannelCredentials( + credential_type, &channel_args))); Void dummy; grpc::ClientContext ctx; ctx.set_wait_for_ready(true); diff --git a/test/cpp/qps/driver.h b/test/cpp/qps/driver.h index def32c6f0ef..ddf75ce5872 100644 --- a/test/cpp/qps/driver.h +++ b/test/cpp/qps/driver.h @@ -31,9 +31,10 @@ std::unique_ptr RunScenario( const grpc::testing::ClientConfig& client_config, size_t num_clients, const grpc::testing::ServerConfig& server_config, size_t num_servers, int warmup_seconds, int benchmark_seconds, int spawn_local_worker_count, - const char* qps_server_target_override = ""); + const char* qps_server_target_override = "", + const char* credential_type = "INSECURE_CREDENTIALS"); -bool RunQuit(); +bool RunQuit(const char* credential_type = "INSECURE_CREDENTIALS"); } // namespace testing } // namespace grpc diff --git a/test/cpp/qps/qps_json_driver.cc b/test/cpp/qps/qps_json_driver.cc index 590c22ec29f..d4698a7dc2f 100644 --- a/test/cpp/qps/qps_json_driver.cc +++ b/test/cpp/qps/qps_json_driver.cc @@ -30,6 +30,7 @@ #include "test/cpp/qps/driver.h" #include "test/cpp/qps/parse_json.h" #include "test/cpp/qps/report.h" +#include "test/cpp/util/test_config.h" DEFINE_string(scenarios_file, "", "JSON file containing an array of Scenario objects"); @@ -60,6 +61,9 @@ DEFINE_string(qps_server_target_override, "", DEFINE_string(json_file_out, "", "File to write the JSON output to."); +DEFINE_string(credential_type, "INSECURE_CREDENTIALS", + "Credential type for communication with workers"); + namespace grpc { namespace testing { @@ -71,7 +75,8 @@ static std::unique_ptr RunAndReport(const Scenario& scenario, scenario.server_config(), scenario.num_servers(), scenario.warmup_seconds(), scenario.benchmark_seconds(), scenario.spawn_local_worker_count(), - FLAGS_qps_server_target_override.c_str()); + FLAGS_qps_server_target_override.c_str(), + FLAGS_credential_type.c_str()); // Amend the result with scenario config. Eventually we should adjust // RunScenario contract so we don't need to touch the result here. @@ -83,6 +88,7 @@ static std::unique_ptr RunAndReport(const Scenario& scenario, GetReporter()->ReportTimes(*result); GetReporter()->ReportCpuUsage(*result); GetReporter()->ReportPollCount(*result); + GetReporter()->ReportQueriesPerCpuSec(*result); for (int i = 0; *success && i < result->client_success_size(); i++) { *success = result->client_success(i); @@ -184,7 +190,7 @@ static bool QpsDriver() { } else if (scjson) { json = FLAGS_scenarios_json.c_str(); } else if (FLAGS_quit) { - return RunQuit(); + return RunQuit(FLAGS_credential_type.c_str()); } // Parse into an array of scenarios @@ -219,7 +225,7 @@ static bool QpsDriver() { } // namespace grpc int main(int argc, char** argv) { - grpc::testing::InitBenchmark(&argc, &argv, true); + grpc::testing::InitTest(&argc, &argv, true); bool ok = grpc::testing::QpsDriver(); diff --git a/test/cpp/qps/qps_worker.cc b/test/cpp/qps/qps_worker.cc index 10bc5422e13..4db4c868241 100644 --- a/test/cpp/qps/qps_worker.cc +++ b/test/cpp/qps/qps_worker.cc @@ -41,6 +41,7 @@ #include "test/cpp/qps/client.h" #include "test/cpp/qps/server.h" #include "test/cpp/util/create_test_channel.h" +#include "test/cpp/util/test_credentials_provider.h" namespace grpc { namespace testing { @@ -263,7 +264,8 @@ class WorkerServiceImpl final : public WorkerService::Service { QpsWorker* worker_; }; -QpsWorker::QpsWorker(int driver_port, int server_port) { +QpsWorker::QpsWorker(int driver_port, int server_port, + const char* credential_type) { impl_.reset(new WorkerServiceImpl(server_port, this)); gpr_atm_rel_store(&done_, static_cast(0)); @@ -271,7 +273,9 @@ QpsWorker::QpsWorker(int driver_port, int server_port) { gpr_join_host_port(&server_address, "::", driver_port); ServerBuilder builder; - builder.AddListeningPort(server_address, InsecureServerCredentials()); + builder.AddListeningPort(server_address, + GetCredentialsProvider()->GetServerCredentials( + credential_type)); builder.RegisterService(impl_.get()); gpr_free(server_address); diff --git a/test/cpp/qps/qps_worker.h b/test/cpp/qps/qps_worker.h index c8a7be93605..ab5e26a10e4 100644 --- a/test/cpp/qps/qps_worker.h +++ b/test/cpp/qps/qps_worker.h @@ -33,7 +33,8 @@ class WorkerServiceImpl; class QpsWorker { public: - explicit QpsWorker(int driver_port, int server_port = 0); + explicit QpsWorker(int driver_port, int server_port = 0, + const char* credential_type = "INSECURE_CREDENTIALS"); ~QpsWorker(); bool Done() const; diff --git a/test/cpp/qps/report.cc b/test/cpp/qps/report.cc index 809c5630442..a45b10bcb81 100644 --- a/test/cpp/qps/report.cc +++ b/test/cpp/qps/report.cc @@ -71,6 +71,12 @@ void CompositeReporter::ReportPollCount(const ScenarioResult& result) { } } +void CompositeReporter::ReportQueriesPerCpuSec(const ScenarioResult& result) { + for (size_t i = 0; i < reporters_.size(); ++i) { + reporters_[i]->ReportQueriesPerCpuSec(result); + } +} + void GprLogReporter::ReportQPS(const ScenarioResult& result) { gpr_log(GPR_INFO, "QPS: %.1f", result.summary().qps()); if (result.summary().failed_requests_per_second() > 0) { @@ -119,6 +125,13 @@ void GprLogReporter::ReportPollCount(const ScenarioResult& result) { result.summary().server_polls_per_request()); } +void GprLogReporter::ReportQueriesPerCpuSec(const ScenarioResult& result) { + gpr_log(GPR_INFO, "Server Queries/CPU-sec: %.2f", + result.summary().server_queries_per_cpu_sec()); + gpr_log(GPR_INFO, "Client Queries/CPU-sec: %.2f", + result.summary().client_queries_per_cpu_sec()); +} + void JsonReporter::ReportQPS(const ScenarioResult& result) { grpc::string json_string = SerializeJson(result, "type.googleapis.com/grpc.testing.ScenarioResult"); @@ -147,6 +160,10 @@ void JsonReporter::ReportPollCount(const ScenarioResult& result) { // NOP - all reporting is handled by ReportQPS. } +void JsonReporter::ReportQueriesPerCpuSec(const ScenarioResult& result) { + // NOP - all reporting is handled by ReportQPS. +} + void RpcReporter::ReportQPS(const ScenarioResult& result) { grpc::ClientContext context; grpc::Status status; @@ -183,5 +200,9 @@ void RpcReporter::ReportPollCount(const ScenarioResult& result) { // NOP - all reporting is handled by ReportQPS. } +void RpcReporter::ReportQueriesPerCpuSec(const ScenarioResult& result) { + // NOP - all reporting is handled by ReportQPS. +} + } // namespace testing } // namespace grpc diff --git a/test/cpp/qps/report.h b/test/cpp/qps/report.h index 0bd398fd2a2..321be2a97fa 100644 --- a/test/cpp/qps/report.h +++ b/test/cpp/qps/report.h @@ -64,6 +64,9 @@ class Reporter { /** Reports client and server poll usage inside completion queue. */ virtual void ReportPollCount(const ScenarioResult& result) = 0; + /** Reports queries per cpu-sec. */ + virtual void ReportQueriesPerCpuSec(const ScenarioResult& result) = 0; + private: const string name_; }; @@ -82,6 +85,7 @@ class CompositeReporter : public Reporter { void ReportTimes(const ScenarioResult& result) override; void ReportCpuUsage(const ScenarioResult& result) override; void ReportPollCount(const ScenarioResult& result) override; + void ReportQueriesPerCpuSec(const ScenarioResult& result) override; private: std::vector > reporters_; @@ -99,6 +103,7 @@ class GprLogReporter : public Reporter { void ReportTimes(const ScenarioResult& result) override; void ReportCpuUsage(const ScenarioResult& result) override; void ReportPollCount(const ScenarioResult& result) override; + void ReportQueriesPerCpuSec(const ScenarioResult& result) override; }; /** Dumps the report to a JSON file. */ @@ -114,6 +119,7 @@ class JsonReporter : public Reporter { void ReportTimes(const ScenarioResult& result) override; void ReportCpuUsage(const ScenarioResult& result) override; void ReportPollCount(const ScenarioResult& result) override; + void ReportQueriesPerCpuSec(const ScenarioResult& result) override; const string report_file_; }; @@ -130,6 +136,7 @@ class RpcReporter : public Reporter { void ReportTimes(const ScenarioResult& result) override; void ReportCpuUsage(const ScenarioResult& result) override; void ReportPollCount(const ScenarioResult& result) override; + void ReportQueriesPerCpuSec(const ScenarioResult& result) override; std::unique_ptr stub_; }; diff --git a/test/cpp/qps/server.h b/test/cpp/qps/server.h index 4b699e07080..c0dac96d8b8 100644 --- a/test/cpp/qps/server.h +++ b/test/cpp/qps/server.h @@ -32,6 +32,7 @@ #include "test/core/end2end/data/ssl_test_data.h" #include "test/core/util/port.h" #include "test/cpp/qps/usage_timer.h" +#include "test/cpp/util/test_credentials_provider.h" namespace grpc { namespace testing { @@ -89,12 +90,14 @@ class Server { static std::shared_ptr CreateServerCredentials( const ServerConfig& config) { if (config.has_security_params()) { - SslServerCredentialsOptions::PemKeyCertPair pkcp = {test_server1_key, - test_server1_cert}; - SslServerCredentialsOptions ssl_opts; - ssl_opts.pem_root_certs = ""; - ssl_opts.pem_key_cert_pairs.push_back(pkcp); - return SslServerCredentials(ssl_opts); + grpc::string type; + if (config.security_params().cred_type().empty()) { + type = kTlsCredentialsType; + } else { + type = config.security_params().cred_type(); + } + + return GetCredentialsProvider()->GetServerCredentials(type); } else { return InsecureServerCredentials(); } diff --git a/test/cpp/qps/worker.cc b/test/cpp/qps/worker.cc index fd51d32db0d..7a8036327f6 100644 --- a/test/cpp/qps/worker.cc +++ b/test/cpp/qps/worker.cc @@ -30,6 +30,8 @@ DEFINE_int32(driver_port, 0, "Port for communication with driver"); DEFINE_int32(server_port, 0, "Port for operation as a server"); +DEFINE_string(credential_type, "INSECURE_CREDENTIALS", + "Credential type for communication with driver"); static bool got_sigint = false; @@ -39,7 +41,8 @@ namespace grpc { namespace testing { static void RunServer() { - QpsWorker worker(FLAGS_driver_port, FLAGS_server_port); + QpsWorker worker(FLAGS_driver_port, FLAGS_server_port, + FLAGS_credential_type.c_str()); while (!got_sigint && !worker.Done()) { gpr_sleep_until(gpr_time_add(gpr_now(GPR_CLOCK_REALTIME), diff --git a/test/cpp/util/create_test_channel.cc b/test/cpp/util/create_test_channel.cc index 68c6fe4622e..34b6d60d014 100644 --- a/test/cpp/util/create_test_channel.cc +++ b/test/cpp/util/create_test_channel.cc @@ -51,29 +51,31 @@ void AddProdSslType() { } // namespace -// When ssl is enabled, if server is empty, override_hostname is used to +// When cred_type is 'ssl', if server is empty, override_hostname is used to // create channel. Otherwise, connect to server and override hostname if // override_hostname is provided. -// When ssl is not enabled, override_hostname is ignored. +// When cred_type is not 'ssl', override_hostname is ignored. // Set use_prod_root to true to use the SSL root for connecting to google. // In this case, path to the roots pem file must be set via environment variable // GRPC_DEFAULT_SSL_ROOTS_FILE_PATH. // Otherwise, root for test SSL cert will be used. -// creds will be used to create a channel when enable_ssl is true. +// creds will be used to create a channel when cred_type is 'ssl'. // Use examples: // CreateTestChannel( -// "1.1.1.1:12345", "override.hostname.com", true, false, creds); -// CreateTestChannel("test.google.com:443", "", true, true, creds); +// "1.1.1.1:12345", "ssl", "override.hostname.com", false, creds); +// CreateTestChannel("test.google.com:443", "ssl", "", true, creds); // same as above -// CreateTestChannel("", "test.google.com:443", true, true, creds); +// CreateTestChannel("", "ssl", "test.google.com:443", true, creds); std::shared_ptr CreateTestChannel( - const grpc::string& server, const grpc::string& override_hostname, - bool enable_ssl, bool use_prod_roots, + const grpc::string& server, const grpc::string& cred_type, + const grpc::string& override_hostname, bool use_prod_roots, const std::shared_ptr& creds, const ChannelArguments& args) { ChannelArguments channel_args(args); std::shared_ptr channel_creds; - if (enable_ssl) { + if (cred_type.empty()) { + return CreateChannel(server, InsecureChannelCredentials()); + } else if (cred_type == testing::kTlsCredentialsType) { // cred_type == "ssl" if (use_prod_roots) { gpr_once_init(&g_once_init_add_prod_ssl_provider, &AddProdSslType); channel_creds = testing::GetCredentialsProvider()->GetChannelCredentials( @@ -95,10 +97,28 @@ std::shared_ptr CreateTestChannel( } return CreateCustomChannel(connect_to, channel_creds, channel_args); } else { - return CreateChannel(server, InsecureChannelCredentials()); + channel_creds = testing::GetCredentialsProvider()->GetChannelCredentials( + cred_type, &channel_args); + GPR_ASSERT(channel_creds != nullptr); + + return CreateChannel(server, channel_creds); } } +std::shared_ptr CreateTestChannel( + const grpc::string& server, const grpc::string& override_hostname, + bool enable_ssl, bool use_prod_roots, + const std::shared_ptr& creds, + const ChannelArguments& args) { + grpc::string type; + if (enable_ssl) { + type = testing::kTlsCredentialsType; + } + + return CreateTestChannel(server, type, override_hostname, use_prod_roots, + creds, args); +} + std::shared_ptr CreateTestChannel( const grpc::string& server, const grpc::string& override_hostname, bool enable_ssl, bool use_prod_roots, diff --git a/test/cpp/util/create_test_channel.h b/test/cpp/util/create_test_channel.h index 9b4b09171ef..e2ca8f99b40 100644 --- a/test/cpp/util/create_test_channel.h +++ b/test/cpp/util/create_test_channel.h @@ -44,6 +44,12 @@ std::shared_ptr CreateTestChannel( const std::shared_ptr& creds, const ChannelArguments& args); +std::shared_ptr CreateTestChannel( + const grpc::string& server, const grpc::string& cred_type, + const grpc::string& override_hostname, bool use_prod_roots, + const std::shared_ptr& creds, + const ChannelArguments& args); + std::shared_ptr CreateTestChannel( const grpc::string& server, const grpc::string& credential_type, const std::shared_ptr& creds); From 5b48dea50e93d865e6dbf0147e82648d79e72d8a Mon Sep 17 00:00:00 2001 From: Yuchen Zeng Date: Wed, 12 Jul 2017 19:00:35 -0700 Subject: [PATCH 02/16] Fix fd_orphan in ev_epollsig_linux --- src/core/lib/iomgr/ev_epollsig_linux.c | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/src/core/lib/iomgr/ev_epollsig_linux.c b/src/core/lib/iomgr/ev_epollsig_linux.c index 3c4ca9c7c59..0d969dccce9 100644 --- a/src/core/lib/iomgr/ev_epollsig_linux.c +++ b/src/core/lib/iomgr/ev_epollsig_linux.c @@ -855,24 +855,12 @@ static int fd_wrapped_fd(grpc_fd *fd) { static void fd_orphan(grpc_exec_ctx *exec_ctx, grpc_fd *fd, grpc_closure *on_done, int *release_fd, const char *reason) { - bool is_fd_closed = false; grpc_error *error = GRPC_ERROR_NONE; polling_island *unref_pi = NULL; gpr_mu_lock(&fd->po.mu); fd->on_done_closure = on_done; - /* If release_fd is not NULL, we should be relinquishing control of the file - descriptor fd->fd (but we still own the grpc_fd structure). */ - if (release_fd != NULL) { - *release_fd = fd->fd; - } else { - close(fd->fd); - is_fd_closed = true; - } - - fd->orphaned = true; - /* Remove the active status but keep referenced. We want this grpc_fd struct to be alive (and not added to freelist) until the end of this function */ REF_BY(fd, 1, reason); @@ -887,13 +875,24 @@ static void fd_orphan(grpc_exec_ctx *exec_ctx, grpc_fd *fd, before doing this.) */ if (fd->po.pi != NULL) { polling_island *pi_latest = polling_island_lock(fd->po.pi); - polling_island_remove_fd_locked(pi_latest, fd, is_fd_closed, &error); + polling_island_remove_fd_locked(pi_latest, fd, false /* is_fd_closed */, + &error); gpr_mu_unlock(&pi_latest->mu); unref_pi = fd->po.pi; fd->po.pi = NULL; } + /* If release_fd is not NULL, we should be relinquishing control of the file + descriptor fd->fd (but we still own the grpc_fd structure). */ + if (release_fd != NULL) { + *release_fd = fd->fd; + } else { + close(fd->fd); + } + + fd->orphaned = true; + GRPC_CLOSURE_SCHED(exec_ctx, fd->on_done_closure, GRPC_ERROR_REF(error)); gpr_mu_unlock(&fd->po.mu); From 35e9d41b0266b8949120ea47d15bfae3e87d9b85 Mon Sep 17 00:00:00 2001 From: Yuchen Zeng Date: Wed, 12 Jul 2017 19:29:35 -0700 Subject: [PATCH 03/16] Also fix ev_epoll_limited_pollers_linux --- .../iomgr/ev_epoll_limited_pollers_linux.c | 25 +++++++++---------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/src/core/lib/iomgr/ev_epoll_limited_pollers_linux.c b/src/core/lib/iomgr/ev_epoll_limited_pollers_linux.c index 9f82c480bc4..27b4892d1da 100644 --- a/src/core/lib/iomgr/ev_epoll_limited_pollers_linux.c +++ b/src/core/lib/iomgr/ev_epoll_limited_pollers_linux.c @@ -932,24 +932,12 @@ static int fd_wrapped_fd(grpc_fd *fd) { static void fd_orphan(grpc_exec_ctx *exec_ctx, grpc_fd *fd, grpc_closure *on_done, int *release_fd, const char *reason) { - bool is_fd_closed = false; grpc_error *error = GRPC_ERROR_NONE; polling_island *unref_pi = NULL; gpr_mu_lock(&fd->po.mu); fd->on_done_closure = on_done; - /* If release_fd is not NULL, we should be relinquishing control of the file - descriptor fd->fd (but we still own the grpc_fd structure). */ - if (release_fd != NULL) { - *release_fd = fd->fd; - } else { - close(fd->fd); - is_fd_closed = true; - } - - fd->orphaned = true; - /* Remove the active status but keep referenced. We want this grpc_fd struct to be alive (and not added to freelist) until the end of this function */ REF_BY(fd, 1, reason); @@ -964,13 +952,24 @@ static void fd_orphan(grpc_exec_ctx *exec_ctx, grpc_fd *fd, before doing this.) */ if (fd->po.pi != NULL) { polling_island *pi_latest = polling_island_lock(fd->po.pi); - polling_island_remove_fd_locked(pi_latest, fd, is_fd_closed, &error); + polling_island_remove_fd_locked(pi_latest, fd, false /* is_fd_closed */, + &error); gpr_mu_unlock(&pi_latest->mu); unref_pi = fd->po.pi; fd->po.pi = NULL; } + /* If release_fd is not NULL, we should be relinquishing control of the file + descriptor fd->fd (but we still own the grpc_fd structure). */ + if (release_fd != NULL) { + *release_fd = fd->fd; + } else { + close(fd->fd); + } + + fd->orphaned = true; + GRPC_CLOSURE_SCHED(exec_ctx, fd->on_done_closure, GRPC_ERROR_REF(error)); gpr_mu_unlock(&fd->po.mu); From f47c063d021e8850e9cb393641981dfe3abdc1a1 Mon Sep 17 00:00:00 2001 From: Ken Payson Date: Thu, 13 Jul 2017 18:26:34 -0700 Subject: [PATCH 04/16] Restore interop_client_main This target is used internally. --- test/cpp/interop/BUILD | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/test/cpp/interop/BUILD b/test/cpp/interop/BUILD index 0de5a6f4da4..9123bd929ec 100644 --- a/test/cpp/interop/BUILD +++ b/test/cpp/interop/BUILD @@ -88,13 +88,22 @@ grpc_cc_library( ], ) -grpc_cc_binary( - name = "interop_client", +grpc_cc_library( + name = "interop_client_main", srcs = [ "client.cc", ], + language = "C++", deps = [ ":client_helper_lib", + ], +) + +grpc_cc_binary( + name = "interop_client", + language = "C++", + deps = [ + ":interop_client_main", "//:grpc++", ], ) From 0696611fb574e4d186ac8824ddeb37e8fd7838cf Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Fri, 14 Jul 2017 07:18:39 -0700 Subject: [PATCH 05/16] Do not return calls on server when request proto fails to deserialize. --- CMakeLists.txt | 57 +++++ Makefile | 55 +++++ build.yaml | 15 ++ include/grpc++/impl/codegen/core_codegen.h | 4 + .../impl/codegen/core_codegen_interface.h | 4 + .../grpc++/impl/codegen/server_interface.h | 41 +++- src/cpp/common/core_codegen.cc | 5 + test/cpp/server/BUILD | 43 ++++ test/cpp/server/server_request_call_test.cc | 162 +++++++++++++ .../generated/sources_and_headers.json | 26 ++ tools/run_tests/generated/tests.json | 22 ++ .../server_request_call_test.vcxproj | 223 ++++++++++++++++++ .../server_request_call_test.vcxproj.filters | 39 +++ 13 files changed, 689 insertions(+), 7 deletions(-) create mode 100644 test/cpp/server/BUILD create mode 100644 test/cpp/server/server_request_call_test.cc create mode 100644 vsprojects/vcxproj/test/server_request_call_test/server_request_call_test.vcxproj create mode 100644 vsprojects/vcxproj/test/server_request_call_test/server_request_call_test.vcxproj.filters diff --git a/CMakeLists.txt b/CMakeLists.txt index 3dd8ec45037..28aec791ee1 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -748,6 +748,7 @@ if(_gRPC_PLATFORM_LINUX OR _gRPC_PLATFORM_MAC OR _gRPC_PLATFORM_POSIX) add_dependencies(buildtests_cxx server_crash_test) endif() add_dependencies(buildtests_cxx server_crash_test_client) +add_dependencies(buildtests_cxx server_request_call_test) add_dependencies(buildtests_cxx shutdown_test) add_dependencies(buildtests_cxx status_test) if(_gRPC_PLATFORM_LINUX OR _gRPC_PLATFORM_MAC OR _gRPC_PLATFORM_POSIX) @@ -11990,6 +11991,62 @@ target_link_libraries(server_crash_test_client endif (gRPC_BUILD_TESTS) if (gRPC_BUILD_TESTS) +add_executable(server_request_call_test + ${_gRPC_PROTO_GENS_DIR}/src/proto/grpc/testing/echo_messages.pb.cc + ${_gRPC_PROTO_GENS_DIR}/src/proto/grpc/testing/echo_messages.grpc.pb.cc + ${_gRPC_PROTO_GENS_DIR}/src/proto/grpc/testing/echo_messages.pb.h + ${_gRPC_PROTO_GENS_DIR}/src/proto/grpc/testing/echo_messages.grpc.pb.h + ${_gRPC_PROTO_GENS_DIR}/src/proto/grpc/testing/echo.pb.cc + ${_gRPC_PROTO_GENS_DIR}/src/proto/grpc/testing/echo.grpc.pb.cc + ${_gRPC_PROTO_GENS_DIR}/src/proto/grpc/testing/echo.pb.h + ${_gRPC_PROTO_GENS_DIR}/src/proto/grpc/testing/echo.grpc.pb.h + test/cpp/server/server_request_call_test.cc + third_party/googletest/googletest/src/gtest-all.cc + third_party/googletest/googlemock/src/gmock-all.cc +) + +protobuf_generate_grpc_cpp( + src/proto/grpc/testing/echo_messages.proto +) +protobuf_generate_grpc_cpp( + src/proto/grpc/testing/echo.proto +) + +target_include_directories(server_request_call_test + PRIVATE ${CMAKE_CURRENT_SOURCE_DIR} + PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/include + PRIVATE ${BORINGSSL_ROOT_DIR}/include + PRIVATE ${PROTOBUF_ROOT_DIR}/src + PRIVATE ${BENCHMARK_ROOT_DIR}/include + PRIVATE ${ZLIB_ROOT_DIR} + PRIVATE ${CMAKE_CURRENT_BINARY_DIR}/third_party/zlib + PRIVATE ${CARES_BUILD_INCLUDE_DIR} + PRIVATE ${CARES_INCLUDE_DIR} + PRIVATE ${CARES_PLATFORM_INCLUDE_DIR} + PRIVATE ${CMAKE_CURRENT_BINARY_DIR}/third_party/cares/cares + PRIVATE ${CMAKE_CURRENT_BINARY_DIR}/third_party/gflags/include + PRIVATE third_party/googletest/googletest/include + PRIVATE third_party/googletest/googletest + PRIVATE third_party/googletest/googlemock/include + PRIVATE third_party/googletest/googlemock + PRIVATE ${_gRPC_PROTO_GENS_DIR} +) + +target_link_libraries(server_request_call_test + ${_gRPC_PROTOBUF_LIBRARIES} + ${_gRPC_ALLTARGETS_LIBRARIES} + grpc++_test_util + grpc_test_util + gpr_test_util + grpc++ + grpc + gpr + ${_gRPC_GFLAGS_LIBRARIES} +) + +endif (gRPC_BUILD_TESTS) +if (gRPC_BUILD_TESTS) + add_executable(shutdown_test test/cpp/end2end/shutdown_test.cc third_party/googletest/googletest/src/gtest-all.cc diff --git a/Makefile b/Makefile index f58a02df9ae..ad9a93f2812 100644 --- a/Makefile +++ b/Makefile @@ -1166,6 +1166,7 @@ server_builder_test: $(BINDIR)/$(CONFIG)/server_builder_test server_context_test_spouse_test: $(BINDIR)/$(CONFIG)/server_context_test_spouse_test server_crash_test: $(BINDIR)/$(CONFIG)/server_crash_test server_crash_test_client: $(BINDIR)/$(CONFIG)/server_crash_test_client +server_request_call_test: $(BINDIR)/$(CONFIG)/server_request_call_test shutdown_test: $(BINDIR)/$(CONFIG)/shutdown_test status_test: $(BINDIR)/$(CONFIG)/status_test streaming_throughput_test: $(BINDIR)/$(CONFIG)/streaming_throughput_test @@ -1589,6 +1590,7 @@ buildtests_cxx: privatelibs_cxx \ $(BINDIR)/$(CONFIG)/server_context_test_spouse_test \ $(BINDIR)/$(CONFIG)/server_crash_test \ $(BINDIR)/$(CONFIG)/server_crash_test_client \ + $(BINDIR)/$(CONFIG)/server_request_call_test \ $(BINDIR)/$(CONFIG)/shutdown_test \ $(BINDIR)/$(CONFIG)/status_test \ $(BINDIR)/$(CONFIG)/streaming_throughput_test \ @@ -1703,6 +1705,7 @@ buildtests_cxx: privatelibs_cxx \ $(BINDIR)/$(CONFIG)/server_context_test_spouse_test \ $(BINDIR)/$(CONFIG)/server_crash_test \ $(BINDIR)/$(CONFIG)/server_crash_test_client \ + $(BINDIR)/$(CONFIG)/server_request_call_test \ $(BINDIR)/$(CONFIG)/shutdown_test \ $(BINDIR)/$(CONFIG)/status_test \ $(BINDIR)/$(CONFIG)/streaming_throughput_test \ @@ -2095,6 +2098,8 @@ test_cxx: buildtests_cxx $(Q) $(BINDIR)/$(CONFIG)/server_context_test_spouse_test || ( echo test server_context_test_spouse_test failed ; exit 1 ) $(E) "[RUN] Testing server_crash_test" $(Q) $(BINDIR)/$(CONFIG)/server_crash_test || ( echo test server_crash_test failed ; exit 1 ) + $(E) "[RUN] Testing server_request_call_test" + $(Q) $(BINDIR)/$(CONFIG)/server_request_call_test || ( echo test server_request_call_test failed ; exit 1 ) $(E) "[RUN] Testing shutdown_test" $(Q) $(BINDIR)/$(CONFIG)/shutdown_test || ( echo test shutdown_test failed ; exit 1 ) $(E) "[RUN] Testing status_test" @@ -15961,6 +15966,56 @@ endif endif +SERVER_REQUEST_CALL_TEST_SRC = \ + $(GENDIR)/src/proto/grpc/testing/echo_messages.pb.cc $(GENDIR)/src/proto/grpc/testing/echo_messages.grpc.pb.cc \ + $(GENDIR)/src/proto/grpc/testing/echo.pb.cc $(GENDIR)/src/proto/grpc/testing/echo.grpc.pb.cc \ + test/cpp/server/server_request_call_test.cc \ + +SERVER_REQUEST_CALL_TEST_OBJS = $(addprefix $(OBJDIR)/$(CONFIG)/, $(addsuffix .o, $(basename $(SERVER_REQUEST_CALL_TEST_SRC)))) +ifeq ($(NO_SECURE),true) + +# You can't build secure targets if you don't have OpenSSL. + +$(BINDIR)/$(CONFIG)/server_request_call_test: openssl_dep_error + +else + + + + +ifeq ($(NO_PROTOBUF),true) + +# You can't build the protoc plugins or protobuf-enabled targets if you don't have protobuf 3.0.0+. + +$(BINDIR)/$(CONFIG)/server_request_call_test: protobuf_dep_error + +else + +$(BINDIR)/$(CONFIG)/server_request_call_test: $(PROTOBUF_DEP) $(SERVER_REQUEST_CALL_TEST_OBJS) $(LIBDIR)/$(CONFIG)/libgrpc++_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc_test_util.a $(LIBDIR)/$(CONFIG)/libgpr_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc++.a $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr.a + $(E) "[LD] Linking $@" + $(Q) mkdir -p `dirname $@` + $(Q) $(LDXX) $(LDFLAGS) $(SERVER_REQUEST_CALL_TEST_OBJS) $(LIBDIR)/$(CONFIG)/libgrpc++_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc_test_util.a $(LIBDIR)/$(CONFIG)/libgpr_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc++.a $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr.a $(LDLIBSXX) $(LDLIBS_PROTOBUF) $(LDLIBS) $(LDLIBS_SECURE) $(GTEST_LIB) -o $(BINDIR)/$(CONFIG)/server_request_call_test + +endif + +endif + +$(OBJDIR)/$(CONFIG)/src/proto/grpc/testing/echo_messages.o: $(LIBDIR)/$(CONFIG)/libgrpc++_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc_test_util.a $(LIBDIR)/$(CONFIG)/libgpr_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc++.a $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr.a + +$(OBJDIR)/$(CONFIG)/src/proto/grpc/testing/echo.o: $(LIBDIR)/$(CONFIG)/libgrpc++_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc_test_util.a $(LIBDIR)/$(CONFIG)/libgpr_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc++.a $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr.a + +$(OBJDIR)/$(CONFIG)/test/cpp/server/server_request_call_test.o: $(LIBDIR)/$(CONFIG)/libgrpc++_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc_test_util.a $(LIBDIR)/$(CONFIG)/libgpr_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc++.a $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr.a + +deps_server_request_call_test: $(SERVER_REQUEST_CALL_TEST_OBJS:.o=.dep) + +ifneq ($(NO_SECURE),true) +ifneq ($(NO_DEPS),true) +-include $(SERVER_REQUEST_CALL_TEST_OBJS:.o=.dep) +endif +endif +$(OBJDIR)/$(CONFIG)/test/cpp/server/server_request_call_test.o: $(GENDIR)/src/proto/grpc/testing/echo_messages.pb.cc $(GENDIR)/src/proto/grpc/testing/echo_messages.grpc.pb.cc $(GENDIR)/src/proto/grpc/testing/echo.pb.cc $(GENDIR)/src/proto/grpc/testing/echo.grpc.pb.cc + + SHUTDOWN_TEST_SRC = \ test/cpp/end2end/shutdown_test.cc \ diff --git a/build.yaml b/build.yaml index 8a2fbe19077..375d6e1f6b2 100644 --- a/build.yaml +++ b/build.yaml @@ -4359,6 +4359,21 @@ targets: - grpc - gpr_test_util - gpr +- name: server_request_call_test + gtest: true + build: test + language: c++ + src: + - src/proto/grpc/testing/echo_messages.proto + - src/proto/grpc/testing/echo.proto + - test/cpp/server/server_request_call_test.cc + deps: + - grpc++_test_util + - grpc_test_util + - gpr_test_util + - grpc++ + - grpc + - gpr - name: shutdown_test gtest: true build: test diff --git a/include/grpc++/impl/codegen/core_codegen.h b/include/grpc++/impl/codegen/core_codegen.h index 504c79cff94..2b15a018455 100644 --- a/include/grpc++/impl/codegen/core_codegen.h +++ b/include/grpc++/impl/codegen/core_codegen.h @@ -60,6 +60,10 @@ class CoreCodegen final : public CoreCodegenInterface { void gpr_cv_signal(gpr_cv* cv) override; void gpr_cv_broadcast(gpr_cv* cv) override; + grpc_call_error grpc_call_cancel_with_status(grpc_call* call, + grpc_status_code status, + const char* description, + void* reserved) override; void grpc_call_ref(grpc_call* call) override; void grpc_call_unref(grpc_call* call) override; virtual void* grpc_call_arena_alloc(grpc_call* call, size_t length) override; diff --git a/include/grpc++/impl/codegen/core_codegen_interface.h b/include/grpc++/impl/codegen/core_codegen_interface.h index 930b116ddac..b4c771ac93a 100644 --- a/include/grpc++/impl/codegen/core_codegen_interface.h +++ b/include/grpc++/impl/codegen/core_codegen_interface.h @@ -89,6 +89,10 @@ class CoreCodegenInterface { virtual grpc_slice grpc_slice_new_with_user_data(void* p, size_t len, void (*destroy)(void*), void* user_data) = 0; + virtual grpc_call_error grpc_call_cancel_with_status(grpc_call* call, + grpc_status_code status, + const char* description, + void* reserved) = 0; virtual void grpc_call_ref(grpc_call* call) = 0; virtual void grpc_call_unref(grpc_call* call) = 0; virtual void* grpc_call_arena_alloc(grpc_call* call, size_t length) = 0; diff --git a/include/grpc++/impl/codegen/server_interface.h b/include/grpc++/impl/codegen/server_interface.h index 9d120031ca3..3e013a32b1c 100644 --- a/include/grpc++/impl/codegen/server_interface.h +++ b/include/grpc++/impl/codegen/server_interface.h @@ -176,22 +176,49 @@ class ServerInterface : public internal::CallHook { ServerCompletionQueue* notification_cq, void* tag, Message* request) : RegisteredAsyncRequest(server, context, stream, call_cq, tag), + registered_method_(registered_method), + server_(server), + context_(context), + stream_(stream), + call_cq_(call_cq), + notification_cq_(notification_cq), + tag_(tag), request_(request) { IssueRequest(registered_method, &payload_, notification_cq); } bool FinalizeResult(void** tag, bool* status) override { - bool serialization_status = - *status && payload_ && - SerializationTraits::Deserialize(payload_, request_).ok(); - bool ret = RegisteredAsyncRequest::FinalizeResult(tag, status); - *status = serialization_status && *status; - return ret; + if (*status) { + if (payload_ == nullptr || + !SerializationTraits::Deserialize(payload_, request_) + .ok()) { + // If deserialization fails, we cancel the call and instantiate + // a new instance of ourselves to request another call. We then + // return false, which prevents the call from being returned to + // the application. + g_core_codegen_interface->grpc_call_cancel_with_status( + call_, GRPC_STATUS_INTERNAL, "Unable to parse request", nullptr); + g_core_codegen_interface->grpc_call_unref(call_); + new PayloadAsyncRequest(registered_method_, server_, context_, + stream_, call_cq_, notification_cq_, tag_, + request_); + delete this; + return false; + } + } + return RegisteredAsyncRequest::FinalizeResult(tag, status); } private: - grpc_byte_buffer* payload_; + void* const registered_method_; + ServerInterface* const server_; + ServerContext* const context_; + internal::ServerAsyncStreamingInterface* const stream_; + CompletionQueue* const call_cq_; + ServerCompletionQueue* const notification_cq_; + void* const tag_; Message* const request_; + grpc_byte_buffer* payload_; }; class GenericAsyncRequest : public BaseAsyncRequest { diff --git a/src/cpp/common/core_codegen.cc b/src/cpp/common/core_codegen.cc index 71017407dea..c7c6b6b13b4 100644 --- a/src/cpp/common/core_codegen.cc +++ b/src/cpp/common/core_codegen.cc @@ -93,6 +93,11 @@ void CoreCodegen::grpc_byte_buffer_destroy(grpc_byte_buffer* bb) { ::grpc_byte_buffer_destroy(bb); } +grpc_call_error CoreCodegen::grpc_call_cancel_with_status( + grpc_call* call, grpc_status_code status, const char* description, + void* reserved) { + return ::grpc_call_cancel_with_status(call, status, description, reserved); +} void CoreCodegen::grpc_call_ref(grpc_call* call) { ::grpc_call_ref(call); } void CoreCodegen::grpc_call_unref(grpc_call* call) { ::grpc_call_unref(call); } void* CoreCodegen::grpc_call_arena_alloc(grpc_call* call, size_t length) { diff --git a/test/cpp/server/BUILD b/test/cpp/server/BUILD new file mode 100644 index 00000000000..512241e3504 --- /dev/null +++ b/test/cpp/server/BUILD @@ -0,0 +1,43 @@ +# Copyright 2017 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. + +licenses(["notice"]) # Apache v2 + +load("//bazel:grpc_build_system.bzl", "grpc_cc_test", "grpc_cc_library", "grpc_cc_binary") + +grpc_cc_test( + name = "server_builder_test", + srcs = ["server_builder_test.cc"], + deps = [ + "//:grpc++", + "//src/proto/grpc/testing:echo_proto", + "//test/core/util:grpc_test_util", + ], + external_deps = [ + "gtest", + ], +) + +grpc_cc_test( + name = "server_request_call_test", + srcs = ["server_request_call_test.cc"], + deps = [ + "//:grpc++", + "//src/proto/grpc/testing:echo_proto", + "//test/core/util:grpc_test_util", + ], + external_deps = [ + "gtest", + ], +) diff --git a/test/cpp/server/server_request_call_test.cc b/test/cpp/server/server_request_call_test.cc new file mode 100644 index 00000000000..1c64ddfc67c --- /dev/null +++ b/test/cpp/server/server_request_call_test.cc @@ -0,0 +1,162 @@ +/* + * + * Copyright 2017 gRPC authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +#include + +#include +#include + +#include +#include + +#include +#include + +#include + +#include "src/proto/grpc/testing/echo.grpc.pb.h" +#include "test/core/util/port.h" + +namespace grpc { +namespace { + +TEST(ServerRequestCallTest, ShortDeadlineDoesNotCauseOkayFalse) { + std::mutex mu; + bool shutting_down = false; + + // grpc server config. + std::ostringstream s; + int p = grpc_pick_unused_port_or_die(); + s << "[::1]:" << p; + const string address = s.str(); + testing::EchoTestService::AsyncService service; + ServerBuilder builder; + builder.AddListeningPort(address, InsecureServerCredentials()); + auto cq = builder.AddCompletionQueue(); + builder.RegisterService(&service); + auto server = builder.BuildAndStart(); + + // server thread. + std::thread t([address, &service, &cq, &mu, &shutting_down] { + for (int n = 0; true; n++) { + ServerContext ctx; + testing::EchoRequest req; + ServerAsyncResponseWriter responder(&ctx); + + // if shutting down, don't enqueue a new request. + { + std::lock_guard lock(mu); + if (!shutting_down) { + service.RequestEcho(&ctx, &req, &responder, cq.get(), cq.get(), + (void*)1); + } + } + + bool ok; + void* tag; + if (!cq->Next(&tag, &ok)) { + break; + } + + EXPECT_EQ((void*)1, tag); + // If not shutting down, ok must be true for new requests. + { + std::lock_guard lock(mu); + if (!shutting_down && !ok) { + gpr_log(GPR_INFO, "!ok on request %d", n); + abort(); + } + if (shutting_down && !ok) { + // Failed connection due to shutdown, continue flushing the CQ. + continue; + } + } + + // Send a simple response after a small delay that would ensure the client + // deadline is exceeded. + gpr_log(GPR_INFO, "Got request %d", n); + testing::EchoResponse response; + response.set_message("foobar"); + // A bit of sleep to make sure the deadline elapses. + gpr_sleep_until(gpr_time_add(gpr_now(GPR_CLOCK_MONOTONIC), + gpr_time_from_millis(50, GPR_TIMESPAN))); + { + std::lock_guard lock(mu); + if (shutting_down) { + gpr_log(GPR_INFO, + "shut down while processing call, not calling Finish()"); + // Continue flushing the CQ. + continue; + } + gpr_log(GPR_INFO, "Finishing request %d", n); + responder.Finish(response, grpc::Status::OK, (void*)2); + if (!cq->Next(&tag, &ok)) { + break; + } + EXPECT_EQ((void*)2, tag); + } + } + }); + + auto stub = testing::EchoTestService::NewStub( + CreateChannel(address, InsecureChannelCredentials())); + + for (int i = 0; i < 100; i++) { + gpr_log(GPR_INFO, "Sending %d.", i); + testing::EchoRequest request; + + ///////// + // Comment out the following line to get ok=false due to invalid request. + // Otherwise, ok=false due to deadline being exceeded. + ///////// + request.set_message("foobar"); + + // A simple request with a short deadline. The server will always exceed the + // deadline, whether due to the sleep or because the server was unable to + // even fetch the request from the CQ before the deadline elapsed. + testing::EchoResponse response; + ::grpc::ClientContext ctx; + ctx.set_fail_fast(false); + ctx.set_deadline(std::chrono::system_clock::now() + + std::chrono::milliseconds(1)); + grpc::Status status = stub->Echo(&ctx, request, &response); + EXPECT_EQ(DEADLINE_EXCEEDED, status.error_code()); + gpr_log(GPR_INFO, "Success."); + } + gpr_log(GPR_INFO, "Done sending RPCs."); + + // Shut down everything properly. + gpr_log(GPR_INFO, "Shutting down."); + { + std::lock_guard lock(mu); + shutting_down = true; + } + server->Shutdown(); + cq->Shutdown(); + server->Wait(); + + t.join(); +} + +} // namespace +} // namespace grpc + +int main(int argc, char** argv) { + ::testing::InitGoogleTest(&argc, argv); + return RUN_ALL_TESTS(); +} diff --git a/tools/run_tests/generated/sources_and_headers.json b/tools/run_tests/generated/sources_and_headers.json index 02562bfae4d..57ce5aa9fa4 100644 --- a/tools/run_tests/generated/sources_and_headers.json +++ b/tools/run_tests/generated/sources_and_headers.json @@ -3933,6 +3933,32 @@ "third_party": false, "type": "target" }, + { + "deps": [ + "gpr", + "gpr_test_util", + "grpc", + "grpc++", + "grpc++_test_util", + "grpc_test_util" + ], + "headers": [ + "src/proto/grpc/testing/echo.grpc.pb.h", + "src/proto/grpc/testing/echo.pb.h", + "src/proto/grpc/testing/echo_messages.grpc.pb.h", + "src/proto/grpc/testing/echo_messages.pb.h", + "src/proto/grpc/testing/echo_messages_mock.grpc.pb.h", + "src/proto/grpc/testing/echo_mock.grpc.pb.h" + ], + "is_filegroup": false, + "language": "c++", + "name": "server_request_call_test", + "src": [ + "test/cpp/server/server_request_call_test.cc" + ], + "third_party": false, + "type": "target" + }, { "deps": [ "gpr", diff --git a/tools/run_tests/generated/tests.json b/tools/run_tests/generated/tests.json index e44bf0e06d7..0036e23e4d0 100644 --- a/tools/run_tests/generated/tests.json +++ b/tools/run_tests/generated/tests.json @@ -3841,6 +3841,28 @@ "posix" ] }, + { + "args": [], + "ci_platforms": [ + "linux", + "mac", + "posix", + "windows" + ], + "cpu_cost": 1.0, + "exclude_configs": [], + "exclude_iomgrs": [], + "flaky": false, + "gtest": true, + "language": "c++", + "name": "server_request_call_test", + "platforms": [ + "linux", + "mac", + "posix", + "windows" + ] + }, { "args": [], "ci_platforms": [ diff --git a/vsprojects/vcxproj/test/server_request_call_test/server_request_call_test.vcxproj b/vsprojects/vcxproj/test/server_request_call_test/server_request_call_test.vcxproj new file mode 100644 index 00000000000..55511759353 --- /dev/null +++ b/vsprojects/vcxproj/test/server_request_call_test/server_request_call_test.vcxproj @@ -0,0 +1,223 @@ + + + + + + Debug + Win32 + + + Debug + x64 + + + Release + Win32 + + + Release + x64 + + + + {F33164EE-1406-7E49-E894-7E795146B882} + true + $(SolutionDir)IntDir\$(MSBuildProjectName)\ + + + + v100 + + + v110 + + + v120 + + + v140 + + + Application + true + Unicode + + + Application + false + true + Unicode + + + + + + + + + + + + + + + + server_request_call_test + static + Debug + static + Debug + + + server_request_call_test + static + Release + static + Release + + + + NotUsing + Level3 + Disabled + WIN32;_DEBUG;_LIB;%(PreprocessorDefinitions) + true + MultiThreadedDebug + true + None + false + + + Console + true + false + + + + + + NotUsing + Level3 + Disabled + WIN32;_DEBUG;_LIB;%(PreprocessorDefinitions) + true + MultiThreadedDebug + true + None + false + + + Console + true + false + + + + + + NotUsing + Level3 + MaxSpeed + WIN32;NDEBUG;_LIB;%(PreprocessorDefinitions) + true + true + true + MultiThreaded + true + None + false + + + Console + true + false + true + true + + + + + + NotUsing + Level3 + MaxSpeed + WIN32;NDEBUG;_LIB;%(PreprocessorDefinitions) + true + true + true + MultiThreaded + true + None + false + + + Console + true + false + true + true + + + + + + + + + + + + + + + + + + + + + + + + + + {0BE77741-552A-929B-A497-4EF7ECE17A64} + + + {17BCAFC0-5FDC-4C94-AEB9-95F3E220614B} + + + {EAB0A629-17A9-44DB-B5FF-E91A721FE037} + + + {C187A093-A0FE-489D-A40A-6E33DE0F9FEB} + + + {29D16885-7228-4C31-81ED-5F9187C7F2A9} + + + {B23D3D1A-9438-4EDA-BEB6-9A0A03D17792} + + + + + + + + + + + + + + + This project references NuGet package(s) that are missing on this computer. Enable NuGet Package Restore to download them. For more information, see http://go.microsoft.com/fwlink/?LinkID=322105. The missing file is {0}. + + + + + + + + + diff --git a/vsprojects/vcxproj/test/server_request_call_test/server_request_call_test.vcxproj.filters b/vsprojects/vcxproj/test/server_request_call_test/server_request_call_test.vcxproj.filters new file mode 100644 index 00000000000..a82cfb16d5d --- /dev/null +++ b/vsprojects/vcxproj/test/server_request_call_test/server_request_call_test.vcxproj.filters @@ -0,0 +1,39 @@ + + + + + src\proto\grpc\testing + + + src\proto\grpc\testing + + + test\cpp\server + + + + + + {48583c1d-014b-ecf7-ece7-98145537c913} + + + {f931e5e2-7d0e-a8d9-b072-1ed8095387a3} + + + {55f7d797-a139-d9c5-8cc3-7fde09c1d28b} + + + {1b39e313-5219-fbc1-9d88-9154b406e4ba} + + + {613e68e0-6bd4-3936-f8c2-34e255688225} + + + {871d6909-5ab9-336d-41ea-380563fcd3b3} + + + {4939f87e-df84-7c23-cd10-c23c06c72683} + + + + From ac9fdfbc40507a6c5453b171561537ee9cafe61a Mon Sep 17 00:00:00 2001 From: Guantao Liu Date: Fri, 14 Jul 2017 17:33:19 -0700 Subject: [PATCH 06/16] Removed unnecessary default values and fixed sanity. --- test/cpp/qps/BUILD | 3 ++ test/cpp/qps/driver.cc | 34 ++++++++----------- test/cpp/qps/driver.h | 6 ++-- test/cpp/qps/qps_json_driver.cc | 8 ++--- test/cpp/qps/qps_openloop_test.cc | 5 +-- test/cpp/qps/qps_worker.cc | 8 ++--- test/cpp/qps/qps_worker.h | 4 +-- .../qps/secure_sync_unary_ping_pong_test.cc | 5 +-- test/cpp/qps/worker.cc | 6 ++-- 9 files changed, 40 insertions(+), 39 deletions(-) diff --git a/test/cpp/qps/BUILD b/test/cpp/qps/BUILD index e24aa0cd0a0..b3348b76fab 100644 --- a/test/cpp/qps/BUILD +++ b/test/cpp/qps/BUILD @@ -149,6 +149,7 @@ grpc_cc_binary( ":driver_impl", "//:grpc++", "//test/cpp/util:test_config", + "//test/cpp/util:test_util", ], external_deps = [ "gflags", @@ -163,6 +164,7 @@ grpc_cc_test( ":driver_impl", ":qps_worker_impl", "//test/cpp/util:test_config", + "//test/cpp/util:test_util", ], ) @@ -174,6 +176,7 @@ grpc_cc_test( ":driver_impl", "//:grpc++", "//test/cpp/util:test_config", + "//test/cpp/util:test_util", ], ) diff --git a/test/cpp/qps/driver.cc b/test/cpp/qps/driver.cc index bb4e7536e84..88d4d52abc0 100644 --- a/test/cpp/qps/driver.cc +++ b/test/cpp/qps/driver.cc @@ -175,13 +175,11 @@ static void postprocess_scenario_result(ScenarioResult* result) { sum(result->server_stats(), SvrPollCount) / histogram.Count()); auto server_queries_per_cpu_sec = - histogram.Count() / - (sum(result->server_stats(), ServerSystemTime) + - sum(result->server_stats(), ServerUserTime)); + histogram.Count() / (sum(result->server_stats(), ServerSystemTime) + + sum(result->server_stats(), ServerUserTime)); auto client_queries_per_cpu_sec = - histogram.Count() / - (sum(result->client_stats(), SystemTime) + - sum(result->client_stats(), UserTime)); + histogram.Count() / (sum(result->client_stats(), SystemTime) + + sum(result->client_stats(), UserTime)); result->mutable_summary()->set_server_queries_per_cpu_sec( server_queries_per_cpu_sec); @@ -193,7 +191,8 @@ std::unique_ptr RunScenario( const ClientConfig& initial_client_config, size_t num_clients, const ServerConfig& initial_server_config, size_t num_servers, int warmup_seconds, int benchmark_seconds, int spawn_local_worker_count, - const char* qps_server_target_override, const char* credential_type) { + const grpc::string& qps_server_target_override, + const grpc::string& credential_type) { // Log everything from the driver gpr_set_log_verbosity(GPR_LOG_SEVERITY_DEBUG); @@ -266,10 +265,9 @@ std::unique_ptr RunScenario( for (size_t i = 0; i < num_servers; i++) { gpr_log(GPR_INFO, "Starting server on %s (worker #%" PRIuPTR ")", workers[i].c_str(), i); - servers[i].stub = WorkerService::NewStub( - CreateChannel(workers[i], - GetCredentialsProvider()->GetChannelCredentials( - credential_type, &channel_args))); + servers[i].stub = WorkerService::NewStub(CreateChannel( + workers[i], GetCredentialsProvider()->GetChannelCredentials( + credential_type, &channel_args))); ServerConfig server_config = initial_server_config; if (server_config.core_limit() != 0) { @@ -316,9 +314,8 @@ std::unique_ptr RunScenario( gpr_log(GPR_INFO, "Starting client on %s (worker #%" PRIuPTR ")", worker.c_str(), i + num_servers); clients[i].stub = WorkerService::NewStub( - CreateChannel(worker, - GetCredentialsProvider()->GetChannelCredentials( - credential_type, &channel_args))); + CreateChannel(worker, GetCredentialsProvider()->GetChannelCredentials( + credential_type, &channel_args))); ClientConfig per_client_config = client_config; if (initial_client_config.core_limit() != 0) { @@ -503,7 +500,7 @@ std::unique_ptr RunScenario( return result; } -bool RunQuit(const char* credential_type) { +bool RunQuit(const grpc::string& credential_type) { // Get client, server lists bool result = true; auto workers = get_workers("QPS_WORKERS"); @@ -513,10 +510,9 @@ bool RunQuit(const char* credential_type) { ChannelArguments channel_args; for (size_t i = 0; i < workers.size(); i++) { - auto stub = WorkerService::NewStub( - CreateChannel(workers[i], - GetCredentialsProvider()->GetChannelCredentials( - credential_type, &channel_args))); + auto stub = WorkerService::NewStub(CreateChannel( + workers[i], GetCredentialsProvider()->GetChannelCredentials( + credential_type, &channel_args))); Void dummy; grpc::ClientContext ctx; ctx.set_wait_for_ready(true); diff --git a/test/cpp/qps/driver.h b/test/cpp/qps/driver.h index ddf75ce5872..29f2776d791 100644 --- a/test/cpp/qps/driver.h +++ b/test/cpp/qps/driver.h @@ -31,10 +31,10 @@ std::unique_ptr RunScenario( const grpc::testing::ClientConfig& client_config, size_t num_clients, const grpc::testing::ServerConfig& server_config, size_t num_servers, int warmup_seconds, int benchmark_seconds, int spawn_local_worker_count, - const char* qps_server_target_override = "", - const char* credential_type = "INSECURE_CREDENTIALS"); + const grpc::string& qps_server_target_override, + const grpc::string& credential_type); -bool RunQuit(const char* credential_type = "INSECURE_CREDENTIALS"); +bool RunQuit(const grpc::string& credential_type); } // namespace testing } // namespace grpc diff --git a/test/cpp/qps/qps_json_driver.cc b/test/cpp/qps/qps_json_driver.cc index d4698a7dc2f..cca59f64d8c 100644 --- a/test/cpp/qps/qps_json_driver.cc +++ b/test/cpp/qps/qps_json_driver.cc @@ -31,6 +31,7 @@ #include "test/cpp/qps/parse_json.h" #include "test/cpp/qps/report.h" #include "test/cpp/util/test_config.h" +#include "test/cpp/util/test_credentials_provider.h" DEFINE_string(scenarios_file, "", "JSON file containing an array of Scenario objects"); @@ -61,7 +62,7 @@ DEFINE_string(qps_server_target_override, "", DEFINE_string(json_file_out, "", "File to write the JSON output to."); -DEFINE_string(credential_type, "INSECURE_CREDENTIALS", +DEFINE_string(credential_type, grpc::testing::kInsecureCredentialsType, "Credential type for communication with workers"); namespace grpc { @@ -75,8 +76,7 @@ static std::unique_ptr RunAndReport(const Scenario& scenario, scenario.server_config(), scenario.num_servers(), scenario.warmup_seconds(), scenario.benchmark_seconds(), scenario.spawn_local_worker_count(), - FLAGS_qps_server_target_override.c_str(), - FLAGS_credential_type.c_str()); + FLAGS_qps_server_target_override, FLAGS_credential_type); // Amend the result with scenario config. Eventually we should adjust // RunScenario contract so we don't need to touch the result here. @@ -190,7 +190,7 @@ static bool QpsDriver() { } else if (scjson) { json = FLAGS_scenarios_json.c_str(); } else if (FLAGS_quit) { - return RunQuit(FLAGS_credential_type.c_str()); + return RunQuit(FLAGS_credential_type); } // Parse into an array of scenarios diff --git a/test/cpp/qps/qps_openloop_test.cc b/test/cpp/qps/qps_openloop_test.cc index 2f8a3d75f0c..069b3fa0768 100644 --- a/test/cpp/qps/qps_openloop_test.cc +++ b/test/cpp/qps/qps_openloop_test.cc @@ -25,6 +25,7 @@ #include "test/cpp/qps/driver.h" #include "test/cpp/qps/report.h" #include "test/cpp/util/test_config.h" +#include "test/cpp/util/test_credentials_provider.h" namespace grpc { namespace testing { @@ -48,8 +49,8 @@ static void RunQPS() { server_config.set_server_type(ASYNC_SERVER); server_config.set_async_server_threads(8); - const auto result = - RunScenario(client_config, 1, server_config, 1, WARMUP, BENCHMARK, -2); + const auto result = RunScenario(client_config, 1, server_config, 1, WARMUP, + BENCHMARK, -2, "", kInsecureCredentialsType); GetReporter()->ReportQPSPerCore(*result); GetReporter()->ReportLatency(*result); diff --git a/test/cpp/qps/qps_worker.cc b/test/cpp/qps/qps_worker.cc index 4db4c868241..d20bc1b074e 100644 --- a/test/cpp/qps/qps_worker.cc +++ b/test/cpp/qps/qps_worker.cc @@ -265,7 +265,7 @@ class WorkerServiceImpl final : public WorkerService::Service { }; QpsWorker::QpsWorker(int driver_port, int server_port, - const char* credential_type) { + const grpc::string& credential_type) { impl_.reset(new WorkerServiceImpl(server_port, this)); gpr_atm_rel_store(&done_, static_cast(0)); @@ -273,9 +273,9 @@ QpsWorker::QpsWorker(int driver_port, int server_port, gpr_join_host_port(&server_address, "::", driver_port); ServerBuilder builder; - builder.AddListeningPort(server_address, - GetCredentialsProvider()->GetServerCredentials( - credential_type)); + builder.AddListeningPort( + server_address, + GetCredentialsProvider()->GetServerCredentials(credential_type)); builder.RegisterService(impl_.get()); gpr_free(server_address); diff --git a/test/cpp/qps/qps_worker.h b/test/cpp/qps/qps_worker.h index ab5e26a10e4..32ff98da9b7 100644 --- a/test/cpp/qps/qps_worker.h +++ b/test/cpp/qps/qps_worker.h @@ -33,8 +33,8 @@ class WorkerServiceImpl; class QpsWorker { public: - explicit QpsWorker(int driver_port, int server_port = 0, - const char* credential_type = "INSECURE_CREDENTIALS"); + explicit QpsWorker(int driver_port, int server_port, + const grpc::string& credential_type); ~QpsWorker(); bool Done() const; diff --git a/test/cpp/qps/secure_sync_unary_ping_pong_test.cc b/test/cpp/qps/secure_sync_unary_ping_pong_test.cc index 1ee6e374749..137b33ee25f 100644 --- a/test/cpp/qps/secure_sync_unary_ping_pong_test.cc +++ b/test/cpp/qps/secure_sync_unary_ping_pong_test.cc @@ -24,6 +24,7 @@ #include "test/cpp/qps/driver.h" #include "test/cpp/qps/report.h" #include "test/cpp/util/test_config.h" +#include "test/cpp/util/test_credentials_provider.h" namespace grpc { namespace testing { @@ -51,8 +52,8 @@ static void RunSynchronousUnaryPingPong() { client_config.mutable_security_params()->CopyFrom(security); server_config.mutable_security_params()->CopyFrom(security); - const auto result = - RunScenario(client_config, 1, server_config, 1, WARMUP, BENCHMARK, -2); + const auto result = RunScenario(client_config, 1, server_config, 1, WARMUP, + BENCHMARK, -2, "", kInsecureCredentialsType); GetReporter()->ReportQPS(*result); GetReporter()->ReportLatency(*result); diff --git a/test/cpp/qps/worker.cc b/test/cpp/qps/worker.cc index 7a8036327f6..27010b73152 100644 --- a/test/cpp/qps/worker.cc +++ b/test/cpp/qps/worker.cc @@ -27,10 +27,11 @@ #include "test/cpp/qps/qps_worker.h" #include "test/cpp/util/test_config.h" +#include "test/cpp/util/test_credentials_provider.h" DEFINE_int32(driver_port, 0, "Port for communication with driver"); DEFINE_int32(server_port, 0, "Port for operation as a server"); -DEFINE_string(credential_type, "INSECURE_CREDENTIALS", +DEFINE_string(credential_type, grpc::testing::kInsecureCredentialsType, "Credential type for communication with driver"); static bool got_sigint = false; @@ -41,8 +42,7 @@ namespace grpc { namespace testing { static void RunServer() { - QpsWorker worker(FLAGS_driver_port, FLAGS_server_port, - FLAGS_credential_type.c_str()); + QpsWorker worker(FLAGS_driver_port, FLAGS_server_port, FLAGS_credential_type); while (!got_sigint && !worker.Done()) { gpr_sleep_until(gpr_time_add(gpr_now(GPR_CLOCK_REALTIME), From 91d8f0b5a846553ed11d0908129aaad116f63be4 Mon Sep 17 00:00:00 2001 From: Guantao Liu Date: Fri, 14 Jul 2017 18:33:30 -0700 Subject: [PATCH 07/16] Added the missing header file and fixed a syntax error. --- test/cpp/qps/driver.cc | 3 +-- test/cpp/qps/qps_worker.h | 1 + 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/test/cpp/qps/driver.cc b/test/cpp/qps/driver.cc index 88d4d52abc0..4458e389e7e 100644 --- a/test/cpp/qps/driver.cc +++ b/test/cpp/qps/driver.cc @@ -285,8 +285,7 @@ std::unique_ptr RunScenario( if (!servers[i].stream->Read(&init_status)) { gpr_log(GPR_ERROR, "Server %zu did not yield initial status", i); } - if (qps_server_target_override != NULL && - strlen(qps_server_target_override) > 0) { + if (qps_server_target_override.length() > 0) { // overriding the qps server target only works if there is 1 server GPR_ASSERT(num_servers == 1); client_config.add_server_targets(qps_server_target_override); diff --git a/test/cpp/qps/qps_worker.h b/test/cpp/qps/qps_worker.h index 32ff98da9b7..5c1d1a8ae64 100644 --- a/test/cpp/qps/qps_worker.h +++ b/test/cpp/qps/qps_worker.h @@ -22,6 +22,7 @@ #include #include +#include namespace grpc { From 29a7050c357e64639c0f1bf699489659e34778ee Mon Sep 17 00:00:00 2001 From: Guantao Liu Date: Fri, 14 Jul 2017 19:24:26 -0700 Subject: [PATCH 08/16] Fixed sanity. --- test/cpp/qps/qps_worker.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/cpp/qps/qps_worker.h b/test/cpp/qps/qps_worker.h index 5c1d1a8ae64..360125fb170 100644 --- a/test/cpp/qps/qps_worker.h +++ b/test/cpp/qps/qps_worker.h @@ -21,8 +21,8 @@ #include -#include #include +#include namespace grpc { From 1435bfc718b2816d17ff52fbac011bf5ba971109 Mon Sep 17 00:00:00 2001 From: Yihua Zhang Date: Mon, 17 Jul 2017 11:20:51 -0700 Subject: [PATCH 09/16] Add GTS plugin --- BUILD | 3 ++ CMakeLists.txt | 2 + Makefile | 3 ++ binding.gyp | 1 + build.yaml | 4 ++ config.m4 | 1 + config.w32 | 1 + gRPC-Core.podspec | 3 ++ grpc.gemspec | 2 + package.xml | 2 + .../grpc_cronet_plugin_registry.c | 4 ++ .../plugin_registry/grpc_plugin_registry.c | 4 ++ src/core/tsi/gts_transport_security.c | 40 +++++++++++++++++++ src/core/tsi/gts_transport_security.h | 37 +++++++++++++++++ src/python/grpcio/grpc_core_dependencies.py | 1 + tools/doxygen/Doxyfile.core.internal | 2 + .../generated/sources_and_headers.json | 4 ++ vsprojects/vcxproj/grpc/grpc.vcxproj | 3 ++ vsprojects/vcxproj/grpc/grpc.vcxproj.filters | 6 +++ 19 files changed, 123 insertions(+) create mode 100644 src/core/tsi/gts_transport_security.c create mode 100644 src/core/tsi/gts_transport_security.h diff --git a/BUILD b/BUILD index 6f92ef3858f..e8ef2cb2eed 100644 --- a/BUILD +++ b/BUILD @@ -1402,12 +1402,14 @@ grpc_cc_library( name = "tsi", srcs = [ "src/core/tsi/fake_transport_security.c", + "src/core/tsi/gts_transport_security.c", "src/core/tsi/ssl_transport_security.c", "src/core/tsi/transport_security.c", "src/core/tsi/transport_security_adapter.c", ], hdrs = [ "src/core/tsi/fake_transport_security.h", + "src/core/tsi/gts_transport_security.h", "src/core/tsi/ssl_transport_security.h", "src/core/tsi/ssl_types.h", "src/core/tsi/transport_security.h", @@ -1420,6 +1422,7 @@ grpc_cc_library( language = "c", deps = [ "gpr", + "grpc_base", "grpc_trace", ], ) diff --git a/CMakeLists.txt b/CMakeLists.txt index 6ee9490c250..e4243766bfa 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -1122,6 +1122,7 @@ add_library(grpc src/core/lib/security/util/json_util.c src/core/lib/surface/init_secure.c src/core/tsi/fake_transport_security.c + src/core/tsi/gts_transport_security.c src/core/tsi/ssl_transport_security.c src/core/tsi/transport_security.c src/core/tsi/transport_security_adapter.c @@ -1487,6 +1488,7 @@ add_library(grpc_cronet src/core/lib/security/util/json_util.c src/core/lib/surface/init_secure.c src/core/tsi/fake_transport_security.c + src/core/tsi/gts_transport_security.c src/core/tsi/ssl_transport_security.c src/core/tsi/transport_security.c src/core/tsi/transport_security_adapter.c diff --git a/Makefile b/Makefile index 72468f8f928..add767b0f15 100644 --- a/Makefile +++ b/Makefile @@ -3059,6 +3059,7 @@ LIBGRPC_SRC = \ src/core/lib/security/util/json_util.c \ src/core/lib/surface/init_secure.c \ src/core/tsi/fake_transport_security.c \ + src/core/tsi/gts_transport_security.c \ src/core/tsi/ssl_transport_security.c \ src/core/tsi/transport_security.c \ src/core/tsi/transport_security_adapter.c \ @@ -3422,6 +3423,7 @@ LIBGRPC_CRONET_SRC = \ src/core/lib/security/util/json_util.c \ src/core/lib/surface/init_secure.c \ src/core/tsi/fake_transport_security.c \ + src/core/tsi/gts_transport_security.c \ src/core/tsi/ssl_transport_security.c \ src/core/tsi/transport_security.c \ src/core/tsi/transport_security_adapter.c \ @@ -19133,6 +19135,7 @@ src/core/lib/surface/init_secure.c: $(OPENSSL_DEP) src/core/plugin_registry/grpc_cronet_plugin_registry.c: $(OPENSSL_DEP) src/core/plugin_registry/grpc_plugin_registry.c: $(OPENSSL_DEP) src/core/tsi/fake_transport_security.c: $(OPENSSL_DEP) +src/core/tsi/gts_transport_security.c: $(OPENSSL_DEP) src/core/tsi/ssl_transport_security.c: $(OPENSSL_DEP) src/core/tsi/transport_security.c: $(OPENSSL_DEP) src/core/tsi/transport_security_adapter.c: $(OPENSSL_DEP) diff --git a/binding.gyp b/binding.gyp index 00fbe70fee8..6bc6450bbd8 100644 --- a/binding.gyp +++ b/binding.gyp @@ -813,6 +813,7 @@ 'src/core/lib/security/util/json_util.c', 'src/core/lib/surface/init_secure.c', 'src/core/tsi/fake_transport_security.c', + 'src/core/tsi/gts_transport_security.c', 'src/core/tsi/ssl_transport_security.c', 'src/core/tsi/transport_security.c', 'src/core/tsi/transport_security_adapter.c', diff --git a/build.yaml b/build.yaml index 1b9eb63bcdc..adb1e7b947c 100644 --- a/build.yaml +++ b/build.yaml @@ -874,6 +874,7 @@ filegroups: - name: tsi headers: - src/core/tsi/fake_transport_security.h + - src/core/tsi/gts_transport_security.h - src/core/tsi/ssl_transport_security.h - src/core/tsi/ssl_types.h - src/core/tsi/transport_security.h @@ -881,14 +882,17 @@ filegroups: - src/core/tsi/transport_security_interface.h src: - src/core/tsi/fake_transport_security.c + - src/core/tsi/gts_transport_security.c - src/core/tsi/ssl_transport_security.c - src/core/tsi/transport_security.c - src/core/tsi/transport_security_adapter.c deps: - gpr + plugin: grpc_tsi_gts secure: true uses: - grpc_trace + - grpc_base - name: grpc++_base language: c++ public_headers: diff --git a/config.m4 b/config.m4 index 71e5776d851..ebe0a1b3a05 100644 --- a/config.m4 +++ b/config.m4 @@ -263,6 +263,7 @@ if test "$PHP_GRPC" != "no"; then src/core/lib/security/util/json_util.c \ src/core/lib/surface/init_secure.c \ src/core/tsi/fake_transport_security.c \ + src/core/tsi/gts_transport_security.c \ src/core/tsi/ssl_transport_security.c \ src/core/tsi/transport_security.c \ src/core/tsi/transport_security_adapter.c \ diff --git a/config.w32 b/config.w32 index cd374bae650..e8ce70f158d 100644 --- a/config.w32 +++ b/config.w32 @@ -240,6 +240,7 @@ if (PHP_GRPC != "no") { "src\\core\\lib\\security\\util\\json_util.c " + "src\\core\\lib\\surface\\init_secure.c " + "src\\core\\tsi\\fake_transport_security.c " + + "src\\core\\tsi\\gts_transport_security.c " + "src\\core\\tsi\\ssl_transport_security.c " + "src\\core\\tsi\\transport_security.c " + "src\\core\\tsi\\transport_security_adapter.c " + diff --git a/gRPC-Core.podspec b/gRPC-Core.podspec index fc51a92daf8..1d9eca36dc5 100644 --- a/gRPC-Core.podspec +++ b/gRPC-Core.podspec @@ -397,6 +397,7 @@ Pod::Spec.new do |s| 'src/core/lib/security/transport/tsi_error.h', 'src/core/lib/security/util/json_util.h', 'src/core/tsi/fake_transport_security.h', + 'src/core/tsi/gts_transport_security.h', 'src/core/tsi/ssl_transport_security.h', 'src/core/tsi/ssl_types.h', 'src/core/tsi/transport_security.h', @@ -635,6 +636,7 @@ Pod::Spec.new do |s| 'src/core/lib/security/util/json_util.c', 'src/core/lib/surface/init_secure.c', 'src/core/tsi/fake_transport_security.c', + 'src/core/tsi/gts_transport_security.c', 'src/core/tsi/ssl_transport_security.c', 'src/core/tsi/transport_security.c', 'src/core/tsi/transport_security_adapter.c', @@ -877,6 +879,7 @@ Pod::Spec.new do |s| 'src/core/lib/security/transport/tsi_error.h', 'src/core/lib/security/util/json_util.h', 'src/core/tsi/fake_transport_security.h', + 'src/core/tsi/gts_transport_security.h', 'src/core/tsi/ssl_transport_security.h', 'src/core/tsi/ssl_types.h', 'src/core/tsi/transport_security.h', diff --git a/grpc.gemspec b/grpc.gemspec index ab4a634c9b0..dad1b31ec91 100755 --- a/grpc.gemspec +++ b/grpc.gemspec @@ -329,6 +329,7 @@ Gem::Specification.new do |s| s.files += %w( src/core/lib/security/transport/tsi_error.h ) s.files += %w( src/core/lib/security/util/json_util.h ) s.files += %w( src/core/tsi/fake_transport_security.h ) + s.files += %w( src/core/tsi/gts_transport_security.h ) s.files += %w( src/core/tsi/ssl_transport_security.h ) s.files += %w( src/core/tsi/ssl_types.h ) s.files += %w( src/core/tsi/transport_security.h ) @@ -571,6 +572,7 @@ Gem::Specification.new do |s| s.files += %w( src/core/lib/security/util/json_util.c ) s.files += %w( src/core/lib/surface/init_secure.c ) s.files += %w( src/core/tsi/fake_transport_security.c ) + s.files += %w( src/core/tsi/gts_transport_security.c ) s.files += %w( src/core/tsi/ssl_transport_security.c ) s.files += %w( src/core/tsi/transport_security.c ) s.files += %w( src/core/tsi/transport_security_adapter.c ) diff --git a/package.xml b/package.xml index 602800b1b51..c79b38f686c 100644 --- a/package.xml +++ b/package.xml @@ -343,6 +343,7 @@ + @@ -585,6 +586,7 @@ + diff --git a/src/core/plugin_registry/grpc_cronet_plugin_registry.c b/src/core/plugin_registry/grpc_cronet_plugin_registry.c index b468c03aa34..322ebea1115 100644 --- a/src/core/plugin_registry/grpc_cronet_plugin_registry.c +++ b/src/core/plugin_registry/grpc_cronet_plugin_registry.c @@ -26,6 +26,8 @@ extern void grpc_deadline_filter_init(void); extern void grpc_deadline_filter_shutdown(void); extern void grpc_client_channel_init(void); extern void grpc_client_channel_shutdown(void); +extern void grpc_tsi_gts_init(void); +extern void grpc_tsi_gts_shutdown(void); extern void grpc_load_reporting_plugin_init(void); extern void grpc_load_reporting_plugin_shutdown(void); @@ -38,6 +40,8 @@ void grpc_register_built_in_plugins(void) { grpc_deadline_filter_shutdown); grpc_register_plugin(grpc_client_channel_init, grpc_client_channel_shutdown); + grpc_register_plugin(grpc_tsi_gts_init, + grpc_tsi_gts_shutdown); grpc_register_plugin(grpc_load_reporting_plugin_init, grpc_load_reporting_plugin_shutdown); } diff --git a/src/core/plugin_registry/grpc_plugin_registry.c b/src/core/plugin_registry/grpc_plugin_registry.c index b0d06a3f43a..fa9974952c0 100644 --- a/src/core/plugin_registry/grpc_plugin_registry.c +++ b/src/core/plugin_registry/grpc_plugin_registry.c @@ -22,6 +22,8 @@ extern void grpc_http_filters_init(void); extern void grpc_http_filters_shutdown(void); extern void grpc_chttp2_plugin_init(void); extern void grpc_chttp2_plugin_shutdown(void); +extern void grpc_tsi_gts_init(void); +extern void grpc_tsi_gts_shutdown(void); extern void grpc_deadline_filter_init(void); extern void grpc_deadline_filter_shutdown(void); extern void grpc_client_channel_init(void); @@ -58,6 +60,8 @@ void grpc_register_built_in_plugins(void) { grpc_http_filters_shutdown); grpc_register_plugin(grpc_chttp2_plugin_init, grpc_chttp2_plugin_shutdown); + grpc_register_plugin(grpc_tsi_gts_init, + grpc_tsi_gts_shutdown); grpc_register_plugin(grpc_deadline_filter_init, grpc_deadline_filter_shutdown); grpc_register_plugin(grpc_client_channel_init, diff --git a/src/core/tsi/gts_transport_security.c b/src/core/tsi/gts_transport_security.c new file mode 100644 index 00000000000..e2ac685e441 --- /dev/null +++ b/src/core/tsi/gts_transport_security.c @@ -0,0 +1,40 @@ +/* + * + * Copyright 2017 gRPC authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +#include "src/core/tsi/gts_transport_security.h" + +#include + +static gts_shared_resource g_gts_resource; + +gts_shared_resource *gts_get_shared_resource(void) { return &g_gts_resource; } + +void grpc_tsi_gts_init() { + memset(&g_gts_resource, 0, sizeof(gts_shared_resource)); + gpr_mu_init(&g_gts_resource.mu); +} + +void grpc_tsi_gts_shutdown() { + gpr_mu_destroy(&g_gts_resource.mu); + if (g_gts_resource.cq == NULL) { + return; + } + grpc_completion_queue_destroy(g_gts_resource.cq); + grpc_channel_destroy(g_gts_resource.channel); + gpr_thd_join(g_gts_resource.thread_id); +} diff --git a/src/core/tsi/gts_transport_security.h b/src/core/tsi/gts_transport_security.h new file mode 100644 index 00000000000..538e1030bc5 --- /dev/null +++ b/src/core/tsi/gts_transport_security.h @@ -0,0 +1,37 @@ +/* + * + * Copyright 2017 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. + * + */ + +#ifndef GRPC_CORE_TSI_GTS_TRANSPORT_SECURITY_H +#define GRPC_CORE_TSI_GTS_TRANSPORT_SECURITY_H + +#include +#include +#include + +typedef struct gts_shared_resource { + gpr_thd_id thread_id; + grpc_channel *channel; + grpc_completion_queue *cq; + gpr_mu mu; +} gts_shared_resource; + +/* This method returns the address of gts_shared_resource object shared by all + * TSI handshakes. */ +gts_shared_resource *gts_get_shared_resource(void); + +#endif /* GRPC_CORE_TSI_GTS_TRANSPORT_SECURITY_H */ diff --git a/src/python/grpcio/grpc_core_dependencies.py b/src/python/grpcio/grpc_core_dependencies.py index b30d340ce56..17cde1ac8ff 100644 --- a/src/python/grpcio/grpc_core_dependencies.py +++ b/src/python/grpcio/grpc_core_dependencies.py @@ -239,6 +239,7 @@ CORE_SOURCE_FILES = [ 'src/core/lib/security/util/json_util.c', 'src/core/lib/surface/init_secure.c', 'src/core/tsi/fake_transport_security.c', + 'src/core/tsi/gts_transport_security.c', 'src/core/tsi/ssl_transport_security.c', 'src/core/tsi/transport_security.c', 'src/core/tsi/transport_security_adapter.c', diff --git a/tools/doxygen/Doxyfile.core.internal b/tools/doxygen/Doxyfile.core.internal index 4c6c656b1b5..bcdcab6c9ac 100644 --- a/tools/doxygen/Doxyfile.core.internal +++ b/tools/doxygen/Doxyfile.core.internal @@ -1402,6 +1402,8 @@ src/core/plugin_registry/grpc_plugin_registry.c \ src/core/tsi/README.md \ src/core/tsi/fake_transport_security.c \ src/core/tsi/fake_transport_security.h \ +src/core/tsi/gts_transport_security.c \ +src/core/tsi/gts_transport_security.h \ src/core/tsi/ssl_transport_security.c \ src/core/tsi/ssl_transport_security.h \ src/core/tsi/ssl_types.h \ diff --git a/tools/run_tests/generated/sources_and_headers.json b/tools/run_tests/generated/sources_and_headers.json index 0a91a0cec45..c9ecfe9f39e 100644 --- a/tools/run_tests/generated/sources_and_headers.json +++ b/tools/run_tests/generated/sources_and_headers.json @@ -8872,10 +8872,12 @@ { "deps": [ "gpr", + "grpc_base", "grpc_trace" ], "headers": [ "src/core/tsi/fake_transport_security.h", + "src/core/tsi/gts_transport_security.h", "src/core/tsi/ssl_transport_security.h", "src/core/tsi/ssl_types.h", "src/core/tsi/transport_security.h", @@ -8888,6 +8890,8 @@ "src": [ "src/core/tsi/fake_transport_security.c", "src/core/tsi/fake_transport_security.h", + "src/core/tsi/gts_transport_security.c", + "src/core/tsi/gts_transport_security.h", "src/core/tsi/ssl_transport_security.c", "src/core/tsi/ssl_transport_security.h", "src/core/tsi/ssl_types.h", diff --git a/vsprojects/vcxproj/grpc/grpc.vcxproj b/vsprojects/vcxproj/grpc/grpc.vcxproj index 112f3ec30ac..a7940833fb3 100644 --- a/vsprojects/vcxproj/grpc/grpc.vcxproj +++ b/vsprojects/vcxproj/grpc/grpc.vcxproj @@ -454,6 +454,7 @@ + @@ -876,6 +877,8 @@ + + diff --git a/vsprojects/vcxproj/grpc/grpc.vcxproj.filters b/vsprojects/vcxproj/grpc/grpc.vcxproj.filters index 5c29e58badb..6857c3d8a1f 100644 --- a/vsprojects/vcxproj/grpc/grpc.vcxproj.filters +++ b/vsprojects/vcxproj/grpc/grpc.vcxproj.filters @@ -535,6 +535,9 @@ src\core\tsi + + src\core\tsi + src\core\tsi @@ -1313,6 +1316,9 @@ src\core\tsi + + src\core\tsi + src\core\tsi From 1c0b20dda793124f3b97978060ba13678b614d3e Mon Sep 17 00:00:00 2001 From: Nathaniel Manista Date: Mon, 17 Jul 2017 20:30:32 +0000 Subject: [PATCH 10/16] Narrow .gitignore to only what it needs to ignore "proto" and "*.egg-info/" are no longer generated, "src/" can be narrowed to "/src/", and "*_pb2.py" and "*_pb2_grpc.py" are covered by .gitignore files in parent directories. --- src/python/grpcio_tests/.gitignore | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/python/grpcio_tests/.gitignore b/src/python/grpcio_tests/.gitignore index dcba283a8ca..4f00cd9a736 100644 --- a/src/python/grpcio_tests/.gitignore +++ b/src/python/grpcio_tests/.gitignore @@ -1,5 +1 @@ -proto/ -src/ -*_pb2.py -*_pb2_grpc.py -*.egg-info/ +/src/ From 457b02e6b2ef32b2e57430250ac5fcb8d04237e1 Mon Sep 17 00:00:00 2001 From: Adele Zhou Date: Mon, 17 Jul 2017 13:48:08 -0700 Subject: [PATCH 11/16] Fix image path when pulled from dockerhub. --- tools/run_tests/dockerize/build_interop_image.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/run_tests/dockerize/build_interop_image.sh b/tools/run_tests/dockerize/build_interop_image.sh index 77c7a84828e..9d8ad5325c1 100755 --- a/tools/run_tests/dockerize/build_interop_image.sh +++ b/tools/run_tests/dockerize/build_interop_image.sh @@ -69,8 +69,8 @@ fi if [ "$DOCKERHUB_ORGANIZATION" != "" ] then - DOCKER_IMAGE_NAME=$DOCKERHUB_ORGANIZATION/$BASE_IMAGE - docker pull $DOCKER_IMAGE_NAME + BASE_IMAGE=$DOCKERHUB_ORGANIZATION/$BASE_IMAGE + docker pull $BASE_IMAGE else # Make sure docker image has been built. Should be instantaneous if so. docker build -t $BASE_IMAGE --force-rm=true tools/dockerfile/interoptest/$BASE_NAME || exit $? From a2eb6f7e701c7dff0eba6fed5a61c0f9d7034e8e Mon Sep 17 00:00:00 2001 From: Jan Tattermusch Date: Tue, 18 Jul 2017 15:13:14 +0200 Subject: [PATCH 12/16] set path to mingw on kokoro --- tools/internal_ci/windows/grpc_build_artifacts.bat | 3 +++ tools/run_tests/artifacts/build_artifact_python.bat | 4 ++-- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/tools/internal_ci/windows/grpc_build_artifacts.bat b/tools/internal_ci/windows/grpc_build_artifacts.bat index d5ade5a83d5..8cfa2fb0bea 100644 --- a/tools/internal_ci/windows/grpc_build_artifacts.bat +++ b/tools/internal_ci/windows/grpc_build_artifacts.bat @@ -19,10 +19,13 @@ rename C:\Python34_32bit Python34_32bits rename C:\Python35_32bit Python35_32bits rename C:\Python36_32bit Python36_32bits +pacman -S --noconfirm mingw64/mingw-w64-x86_64-gcc mingw32/mingw-w64-i686-gcc + @rem make sure msys binaries are preferred over cygwin binaries @rem set path to python 2.7 set PATH=C:\tools\msys64\usr\bin;C:\Python27;%PATH% + @rem enter repo root cd /d %~dp0\..\..\.. diff --git a/tools/run_tests/artifacts/build_artifact_python.bat b/tools/run_tests/artifacts/build_artifact_python.bat index 4c34fc50f04..3a0564129cf 100644 --- a/tools/run_tests/artifacts/build_artifact_python.bat +++ b/tools/run_tests/artifacts/build_artifact_python.bat @@ -12,8 +12,8 @@ @rem See the License for the specific language governing permissions and @rem limitations under the License. - -set PATH=C:\%1;C:\%1\scripts;C:\msys64\mingw%2\bin;%PATH% +@rem set path to python & mingw compiler +set PATH=C:\%1;C:\%1\scripts;C:\msys64\mingw%2\bin;C:\tools\msys64\mingw%2\bin;%PATH% pip install --upgrade six pip install --upgrade setuptools From cb8eaaa88364fb3ff657e3c5bbf7f75d737859ec Mon Sep 17 00:00:00 2001 From: Jan Tattermusch Date: Tue, 18 Jul 2017 15:12:49 +0200 Subject: [PATCH 13/16] fix grpcio_tools build on kokoro --- tools/distrib/python/grpcio_tools/setup.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/distrib/python/grpcio_tools/setup.py b/tools/distrib/python/grpcio_tools/setup.py index bc24e4c462a..ddbb348e0cd 100644 --- a/tools/distrib/python/grpcio_tools/setup.py +++ b/tools/distrib/python/grpcio_tools/setup.py @@ -62,9 +62,9 @@ if EXTRA_ENV_COMPILE_ARGS is None: # envvars) without adding yet more GRPC-specific envvars. # See https://sourceforge.net/p/mingw-w64/bugs/363/ if '32' in platform.architecture()[0]: - EXTRA_ENV_COMPILE_ARGS += ' -D_ftime=_ftime32 -D_timeb=__timeb32 -D_ftime_s=_ftime32_s' + EXTRA_ENV_COMPILE_ARGS += ' -D_ftime=_ftime32 -D_timeb=__timeb32 -D_ftime_s=_ftime32_s -D_hypot=hypot' else: - EXTRA_ENV_COMPILE_ARGS += ' -D_ftime=_ftime64 -D_timeb=__timeb64' + EXTRA_ENV_COMPILE_ARGS += ' -D_ftime=_ftime64 -D_timeb=__timeb64 -D_hypot=hypot' else: # We need to statically link the C++ Runtime, only the C runtime is # available dynamically From 0f95fa4909ba6f65364cb0b5c8c37a27c09b67b4 Mon Sep 17 00:00:00 2001 From: Vijay Pai Date: Tue, 18 Jul 2017 10:42:33 -0700 Subject: [PATCH 14/16] Add idiomatic C++ API for grpc::Slice construction that doesn't require using grpc_slice --- include/grpc++/support/slice.h | 14 ++++++++++ src/cpp/util/slice_cc.cc | 13 +++++++++ test/cpp/qps/client.h | 4 +-- test/cpp/util/byte_buffer_proto_helper.cc | 3 +-- test/cpp/util/byte_buffer_test.cc | 21 +++++++-------- test/cpp/util/cli_call.cc | 3 +-- test/cpp/util/slice_test.cc | 33 +++++++++++++++++++---- 7 files changed, 67 insertions(+), 24 deletions(-) diff --git a/include/grpc++/support/slice.h b/include/grpc++/support/slice.h index db5cf942a0c..0b4ba7cecb4 100644 --- a/include/grpc++/support/slice.h +++ b/include/grpc++/support/slice.h @@ -44,6 +44,20 @@ class Slice final { /// Construct a slice from \a slice, stealing a reference. Slice(grpc_slice slice, StealRef); + /// Allocate a slice of specified size + Slice(size_t len); + + /// Construct a slice from a copied buffer + Slice(const void* buf, size_t len); + + /// Construct a slice from a copied string + Slice(const grpc::string& str); + + enum StaticSlice { STATIC_SLICE }; + + /// Construct a slice from a static buffer + Slice(const void* buf, size_t len, StaticSlice); + /// Copy constructor, adds a reference. Slice(const Slice& other); diff --git a/src/cpp/util/slice_cc.cc b/src/cpp/util/slice_cc.cc index 80b4b3a4712..56e0328b949 100644 --- a/src/cpp/util/slice_cc.cc +++ b/src/cpp/util/slice_cc.cc @@ -28,6 +28,19 @@ Slice::Slice(grpc_slice slice, AddRef) : slice_(grpc_slice_ref(slice)) {} Slice::Slice(grpc_slice slice, StealRef) : slice_(slice) {} +Slice::Slice(size_t len) : slice_(grpc_slice_malloc(len)) {} + +Slice::Slice(const void* buf, size_t len) + : slice_(grpc_slice_from_copied_buffer(reinterpret_cast(buf), + len)) {} + +Slice::Slice(const grpc::string& str) + : slice_(grpc_slice_from_copied_buffer(str.c_str(), str.length())) {} + +Slice::Slice(const void* buf, size_t len, StaticSlice) + : slice_(grpc_slice_from_static_buffer(reinterpret_cast(buf), + len)) {} + Slice::Slice(const Slice& other) : slice_(grpc_slice_ref(other.slice_)) {} } // namespace grpc diff --git a/test/cpp/qps/client.h b/test/cpp/qps/client.h index e759446c1ae..ecbf9c31fa1 100644 --- a/test/cpp/qps/client.h +++ b/test/cpp/qps/client.h @@ -88,9 +88,7 @@ class ClientRequestCreator { if (payload_config.has_bytebuf_params()) { std::unique_ptr buf( new char[payload_config.bytebuf_params().req_size()]); - grpc_slice s = grpc_slice_from_copied_buffer( - buf.get(), payload_config.bytebuf_params().req_size()); - Slice slice(s, Slice::STEAL_REF); + Slice slice(buf.get(), payload_config.bytebuf_params().req_size()); *req = ByteBuffer(&slice, 1); } else { GPR_ASSERT(false); // not appropriate for this specialization diff --git a/test/cpp/util/byte_buffer_proto_helper.cc b/test/cpp/util/byte_buffer_proto_helper.cc index 22dd074fe93..bb5162f86e6 100644 --- a/test/cpp/util/byte_buffer_proto_helper.cc +++ b/test/cpp/util/byte_buffer_proto_helper.cc @@ -36,8 +36,7 @@ std::unique_ptr SerializeToByteBuffer( grpc::protobuf::Message* message) { grpc::string buf; message->SerializeToString(&buf); - grpc_slice s = grpc_slice_from_copied_string(buf.c_str()); - Slice slice(s, Slice::STEAL_REF); + Slice slice(buf); return std::unique_ptr(new ByteBuffer(&slice, 1)); } diff --git a/test/cpp/util/byte_buffer_test.cc b/test/cpp/util/byte_buffer_test.cc index 6c077ddb2c3..cac01a73078 100644 --- a/test/cpp/util/byte_buffer_test.cc +++ b/test/cpp/util/byte_buffer_test.cc @@ -34,33 +34,30 @@ const char* kContent2 = "yyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyy world"; class ByteBufferTest : public ::testing::Test {}; TEST_F(ByteBufferTest, CreateFromSingleSlice) { - grpc_slice hello = grpc_slice_from_copied_string(kContent1); - Slice s(hello, Slice::STEAL_REF); + Slice s(kContent1); ByteBuffer buffer(&s, 1); + EXPECT_EQ(strlen(kContent1), buffer.Length()); } TEST_F(ByteBufferTest, CreateFromVector) { - grpc_slice hello = grpc_slice_from_copied_string(kContent1); - grpc_slice world = grpc_slice_from_copied_string(kContent2); std::vector slices; - slices.push_back(Slice(hello, Slice::STEAL_REF)); - slices.push_back(Slice(world, Slice::STEAL_REF)); + slices.emplace_back(kContent1); + slices.emplace_back(kContent2); ByteBuffer buffer(&slices[0], 2); + EXPECT_EQ(strlen(kContent1) + strlen(kContent2), buffer.Length()); } TEST_F(ByteBufferTest, Clear) { - grpc_slice hello = grpc_slice_from_copied_string(kContent1); - Slice s(hello, Slice::STEAL_REF); + Slice s(kContent1); ByteBuffer buffer(&s, 1); buffer.Clear(); + EXPECT_EQ(static_cast(0), buffer.Length()); } TEST_F(ByteBufferTest, Length) { - grpc_slice hello = grpc_slice_from_copied_string(kContent1); - grpc_slice world = grpc_slice_from_copied_string(kContent2); std::vector slices; - slices.push_back(Slice(hello, Slice::STEAL_REF)); - slices.push_back(Slice(world, Slice::STEAL_REF)); + slices.emplace_back(kContent1); + slices.emplace_back(kContent2); ByteBuffer buffer(&slices[0], 2); EXPECT_EQ(strlen(kContent1) + strlen(kContent2), buffer.Length()); } diff --git a/test/cpp/util/cli_call.cc b/test/cpp/util/cli_call.cc index fb6d76bb906..c3220efa544 100644 --- a/test/cpp/util/cli_call.cc +++ b/test/cpp/util/cli_call.cc @@ -119,8 +119,7 @@ void CliCall::WritesDone() { } void CliCall::WriteAndWait(const grpc::string& request) { - grpc_slice s = grpc_slice_from_copied_string(request.c_str()); - grpc::Slice req_slice(s, grpc::Slice::STEAL_REF); + grpc::Slice req_slice(request); grpc::ByteBuffer send_buffer(&req_slice, 1); gpr_mu_lock(&write_mu_); diff --git a/test/cpp/util/slice_test.cc b/test/cpp/util/slice_test.cc index 0f800035801..9e3329fab02 100644 --- a/test/cpp/util/slice_test.cc +++ b/test/cpp/util/slice_test.cc @@ -28,6 +28,9 @@ const char* kContent = "hello xxxxxxxxxxxxxxxxxxxx world"; class SliceTest : public ::testing::Test { protected: + void CheckSliceSize(const Slice& s, const grpc::string& content) { + EXPECT_EQ(content.size(), s.size()); + } void CheckSlice(const Slice& s, const grpc::string& content) { EXPECT_EQ(content.size(), s.size()); EXPECT_EQ(content, @@ -35,6 +38,31 @@ class SliceTest : public ::testing::Test { } }; +TEST_F(SliceTest, Empty) { + Slice empty_slice; + CheckSlice(empty_slice, ""); +} + +TEST_F(SliceTest, Sized) { + Slice sized_slice(strlen(kContent)); + CheckSliceSize(sized_slice, kContent); +} + +TEST_F(SliceTest, String) { + Slice spp(kContent); + CheckSlice(spp, kContent); +} + +TEST_F(SliceTest, Buf) { + Slice spp(kContent, strlen(kContent)); + CheckSlice(spp, kContent); +} + +TEST_F(SliceTest, StaticBuf) { + Slice spp(kContent, strlen(kContent), Slice::STATIC_SLICE); + CheckSlice(spp, kContent); +} + TEST_F(SliceTest, Steal) { grpc_slice s = grpc_slice_from_copied_string(kContent); Slice spp(s, Slice::STEAL_REF); @@ -48,11 +76,6 @@ TEST_F(SliceTest, Add) { CheckSlice(spp, kContent); } -TEST_F(SliceTest, Empty) { - Slice empty_slice; - CheckSlice(empty_slice, ""); -} - TEST_F(SliceTest, Cslice) { grpc_slice s = grpc_slice_from_copied_string(kContent); Slice spp(s, Slice::STEAL_REF); From bbd348c5e18794b0fee65a752bc101cdde4b247d Mon Sep 17 00:00:00 2001 From: Yong Ni Date: Tue, 18 Jul 2017 08:38:11 -0700 Subject: [PATCH 15/16] Added all previous minor releases of gRPC to the matrix. Modified the testsuite name format for better display test results in sponge. --- tools/interop_matrix/client_matrix.py | 26 ++++++++++++++++--- .../run_interop_matrix_tests.py | 2 +- 2 files changed, 23 insertions(+), 5 deletions(-) diff --git a/tools/interop_matrix/client_matrix.py b/tools/interop_matrix/client_matrix.py index 4394e32c085..4d1b5f01695 100644 --- a/tools/interop_matrix/client_matrix.py +++ b/tools/interop_matrix/client_matrix.py @@ -30,9 +30,27 @@ LANG_RUNTIME_MATRIX = { } # Dictionary of releases per language. For each language, we need to provide -# a tuple of release tag (used as the tag for the GCR image) and also github hash. +# a release tag pointing to the latest build of the branch. LANG_RELEASE_MATRIX = { - 'cxx': ['v1.0.1', 'v1.1.2'], - 'go': ['v1.0.1-GA', 'v1.3.0'], - 'java': ['v1.0.3', 'v1.1.2'], + 'cxx': [ + 'v1.0.1', + 'v1.1.4', + 'v1.2.5', + 'v1.3.9', + 'v1.4.2', + ], + 'go': [ + 'v1.0.5', + 'v1.2.1', + 'v1.3.0', + 'v1.4.2', + ], + 'java': [ + 'v1.0.3', + 'v1.1.2', + 'v1.2.0', + 'v1.3.1', + 'v1.4.0', + 'v1.5.0', + ], } diff --git a/tools/interop_matrix/run_interop_matrix_tests.py b/tools/interop_matrix/run_interop_matrix_tests.py index 28126ddf5cc..4315c8277df 100755 --- a/tools/interop_matrix/run_interop_matrix_tests.py +++ b/tools/interop_matrix/run_interop_matrix_tests.py @@ -168,7 +168,7 @@ def run_tests_for_lang(lang, runtime, images): _xml_report_tree, resultset, 'grpc_interop_matrix', - '%s__%s:%s'%(lang,runtime,release), + '%s__%s %s'%(lang,runtime,release), str(uuid.uuid4())) _docker_images_cleanup = [] From 02d426ef6bc3c8babb4535c9ef758cee8eadce39 Mon Sep 17 00:00:00 2001 From: Ben Sykes Date: Tue, 11 Jul 2017 13:58:42 -0700 Subject: [PATCH 16/16] Check for no_proxy environment variable and skip proxy if host is listed This will fix #9989, #11603, #11751. no_proxy is typically comma separated list of hosts and or domains. Matching must be from the end of the uri to accomodate whole domain and subdomain whitelisting. This will work for well-formed no_proxy strings, but may fail if there are additional spaces. --- .../ext/filters/client_channel/http_proxy.c | 46 +++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/src/core/ext/filters/client_channel/http_proxy.c b/src/core/ext/filters/client_channel/http_proxy.c index cfb5ec6f00e..aa3f61c9917 100644 --- a/src/core/ext/filters/client_channel/http_proxy.c +++ b/src/core/ext/filters/client_channel/http_proxy.c @@ -22,6 +22,7 @@ #include #include +#include #include #include @@ -30,6 +31,7 @@ #include "src/core/ext/filters/client_channel/uri_parser.h" #include "src/core/lib/channel/channel_args.h" #include "src/core/lib/support/env.h" +#include "src/core/lib/support/string.h" static char* grpc_get_http_proxy_server(grpc_exec_ctx* exec_ctx) { char* uri_str = gpr_getenv("http_proxy"); @@ -80,6 +82,50 @@ static bool proxy_mapper_map_name(grpc_exec_ctx* exec_ctx, grpc_uri_destroy(uri); return false; } + char* no_proxy_str = gpr_getenv("no_proxy"); + if (no_proxy_str != NULL) { + static const char* NO_PROXY_SEPARATOR = ","; + bool use_proxy = true; + char* server_host; + char* server_port; + if (!gpr_split_host_port(uri->path[0] == '/' ? uri->path + 1 : uri->path, + &server_host, &server_port)) { + gpr_log(GPR_INFO, + "unable to split host and port, not checking no_proxy list for " + "host '%s'", + server_uri); + } else { + size_t uri_len = strlen(server_host); + char** no_proxy_hosts; + size_t num_no_proxy_hosts; + gpr_string_split(no_proxy_str, NO_PROXY_SEPARATOR, &no_proxy_hosts, + &num_no_proxy_hosts); + for (size_t i = 0; i < num_no_proxy_hosts; i++) { + char* no_proxy_entry = no_proxy_hosts[i]; + size_t no_proxy_len = strlen(no_proxy_entry); + if (no_proxy_len <= uri_len && + gpr_stricmp(no_proxy_entry, &server_host[uri_len - no_proxy_len]) == + 0) { + gpr_log(GPR_INFO, "not using proxy for host in no_proxy list '%s'", + server_uri); + use_proxy = false; + break; + } + } + for (size_t i = 0; i < num_no_proxy_hosts; i++) { + gpr_free(no_proxy_hosts[i]); + } + gpr_free(no_proxy_hosts); + gpr_free(server_host); + gpr_free(server_port); + if (!use_proxy) { + grpc_uri_destroy(uri); + gpr_free(*name_to_resolve); + *name_to_resolve = NULL; + return false; + } + } + } grpc_arg new_arg = grpc_channel_arg_string_create( GRPC_ARG_HTTP_CONNECT_SERVER, uri->path[0] == '/' ? uri->path + 1 : uri->path);