Merge branch 'master' into keepaliveopt

src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.cc

@@ -478,7 +478,8 @@ static grpc_address_resolver_vtable ares_resolver = {
     grpc_resolve_address_ares, blocking_resolve_address_ares};
 
 static bool should_use_ares(const char* resolver_env) {
-  return resolver_env != nullptr && gpr_stricmp(resolver_env, "ares") == 0;
+  return resolver_env == nullptr || strlen(resolver_env) == 0 ||
+         gpr_stricmp(resolver_env, "ares") == 0;
 }
 
 void grpc_resolver_dns_ares_init() {

src/core/ext/filters/max_age/max_age_filter.cc

@@ -106,7 +106,7 @@ struct channel_data {
      +--------------------------------+----------------+---------+
 
      MAX_IDLE_STATE_INIT: The initial and final state of 'idle_state'. The
-     channel has 1 or 1+ active calls, and the the timer is not set. Note that
+     channel has 1 or 1+ active calls, and the timer is not set. Note that
      we may put a virtual call to hold this state at channel initialization or
      shutdown, so that the channel won't enter other states.
 

src/core/ext/transport/chttp2/transport/chttp2_transport.cc

@@ -1119,9 +1119,6 @@ static void queue_setting_update(grpc_chttp2_transport* t,
 void grpc_chttp2_add_incoming_goaway(grpc_chttp2_transport* t,
                                      uint32_t goaway_error,
                                      grpc_slice goaway_text) {
-  // GRPC_CHTTP2_IF_TRACING(
-  //     gpr_log(GPR_INFO, "got goaway [%d]: %s", goaway_error, msg));
-
   // Discard the error from a previous goaway frame (if any)
   if (t->goaway_error != GRPC_ERROR_NONE) {
     GRPC_ERROR_UNREF(t->goaway_error);
@@ -1132,6 +1129,10 @@ void grpc_chttp2_add_incoming_goaway(grpc_chttp2_transport* t,
           GRPC_ERROR_INT_HTTP2_ERROR, static_cast<intptr_t>(goaway_error)),
       GRPC_ERROR_STR_RAW_BYTES, goaway_text);
 
+  /* We want to log this irrespective of whether http tracing is enabled */
+  gpr_log(GPR_INFO, "%s: Got goaway [%d] err=%s", t->peer_string, goaway_error,
+          grpc_error_string(t->goaway_error));
+
   /* When a client receives a GOAWAY with error code ENHANCE_YOUR_CALM and debug
    * data equal to "too_many_pings", it should log the occurrence at a log level
    * that is enabled by default and double the configured KEEPALIVE_TIME used
@@ -1774,6 +1775,9 @@ void grpc_chttp2_ack_ping(grpc_chttp2_transport* t, uint64_t id) {
 }
 
 static void send_goaway(grpc_chttp2_transport* t, grpc_error* error) {
+  /* We want to log this irrespective of whether http tracing is enabled */
+  gpr_log(GPR_INFO, "%s: Sending goaway err=%s", t->peer_string,
+          grpc_error_string(error));
   t->sent_goaway_state = GRPC_CHTTP2_GOAWAY_SEND_SCHEDULED;
   grpc_http2_error_code http_error;
   grpc_slice slice;
@@ -2727,6 +2731,9 @@ static void start_keepalive_ping_locked(void* arg, grpc_error* error) {
   if (t->channelz_socket != nullptr) {
     t->channelz_socket->RecordKeepaliveSent();
   }
+  if (grpc_http_trace.enabled()) {
+    gpr_log(GPR_INFO, "%s: Start keepalive ping", t->peer_string);
+  }
   GRPC_CHTTP2_REF_TRANSPORT(t, "keepalive watchdog");
   grpc_timer_init(&t->keepalive_watchdog_timer,
                   grpc_core::ExecCtx::Get()->Now() + t->keepalive_timeout,
@@ -2737,6 +2744,9 @@ static void finish_keepalive_ping_locked(void* arg, grpc_error* error) {
   grpc_chttp2_transport* t = static_cast<grpc_chttp2_transport*>(arg);
   if (t->keepalive_state == GRPC_CHTTP2_KEEPALIVE_STATE_PINGING) {
     if (error == GRPC_ERROR_NONE) {
+      if (grpc_http_trace.enabled()) {
+        gpr_log(GPR_INFO, "%s: Finish keepalive ping", t->peer_string);
+      }
       t->keepalive_state = GRPC_CHTTP2_KEEPALIVE_STATE_WAITING;
       grpc_timer_cancel(&t->keepalive_watchdog_timer);
       GRPC_CHTTP2_REF_TRANSPORT(t, "init keepalive ping");
@@ -2752,6 +2762,8 @@ static void keepalive_watchdog_fired_locked(void* arg, grpc_error* error) {
   grpc_chttp2_transport* t = static_cast<grpc_chttp2_transport*>(arg);
   if (t->keepalive_state == GRPC_CHTTP2_KEEPALIVE_STATE_PINGING) {
     if (error == GRPC_ERROR_NONE) {
+      gpr_log(GPR_ERROR, "%s: Keepalive watchdog fired. Closing transport.",
+              t->peer_string);
       t->keepalive_state = GRPC_CHTTP2_KEEPALIVE_STATE_DYING;
       close_transport_locked(
           t, grpc_error_set_int(GRPC_ERROR_CREATE_FROM_STATIC_STRING(

src/core/ext/transport/chttp2/transport/frame_window_update.cc

@@ -88,8 +88,9 @@ grpc_error* grpc_chttp2_window_update_parser_parse(void* parser,
   }
 
   if (p->byte == 4) {
-    uint32_t received_update = p->amount;
-    if (received_update == 0 || (received_update & 0x80000000u)) {
+    // top bit is reserved and must be ignored.
+    uint32_t received_update = p->amount & 0x7fffffffu;
+    if (received_update == 0) {
       char* msg;
       gpr_asprintf(&msg, "invalid window update bytes: %d", p->amount);
       grpc_error* err = GRPC_ERROR_CREATE_FROM_COPIED_STRING(msg);

src/core/lib/iomgr/error.cc

@@ -303,11 +303,15 @@ static void internal_add_error(grpc_error** err, grpc_error* new_err) {
 // It is very common to include and extra int and string in an error
 #define SURPLUS_CAPACITY (2 * SLOTS_PER_INT + SLOTS_PER_TIME)
 
-static bool g_error_creation_allowed = true;
+static gpr_atm g_error_creation_allowed = true;
 
-void grpc_disable_error_creation() { g_error_creation_allowed = false; }
+void grpc_disable_error_creation() {
+  gpr_atm_no_barrier_store(&g_error_creation_allowed, false);
+}
 
-void grpc_enable_error_creation() { g_error_creation_allowed = true; }
+void grpc_enable_error_creation() {
+  gpr_atm_no_barrier_store(&g_error_creation_allowed, true);
+}
 
 grpc_error* grpc_error_create(const char* file, int line, grpc_slice desc,
                               grpc_error** referencing,
@@ -323,7 +327,7 @@ grpc_error* grpc_error_create(const char* file, int line, grpc_slice desc,
     return GRPC_ERROR_OOM;
   }
 #ifndef NDEBUG
-  if (!g_error_creation_allowed) {
+  if (!gpr_atm_no_barrier_load(&g_error_creation_allowed)) {
     gpr_log(GPR_ERROR,
             "Error creation occurred when error creation was disabled [%s:%d]",
             file, line);

src/core/lib/iomgr/ev_epollex_linux.cc

@@ -180,7 +180,7 @@ struct grpc_fd {
     grpc_iomgr_unregister_object(&iomgr_object);
 
     POLLABLE_UNREF(pollable_obj, "fd_pollable");
-    pollsets.clear();
+    pollset_fds.clear();
     gpr_mu_destroy(&pollable_mu);
     gpr_mu_destroy(&orphan_mu);
 
@@ -220,9 +220,9 @@ struct grpc_fd {
 
   gpr_mu orphan_mu;
 
-  // Protects pollable_obj and pollsets.
+  // Protects pollable_obj and pollset_fds.
   gpr_mu pollable_mu;
-  grpc_core::InlinedVector<grpc_pollset*, 1> pollsets;  // Used in PO_MULTI.
+  grpc_core::InlinedVector<int, 1> pollset_fds;  // Used in PO_MULTI.
   pollable* pollable_obj = nullptr;              // Used in PO_FD.
 
   grpc_core::LockfreeEvent read_closure;
@@ -422,7 +422,6 @@ static int fd_wrapped_fd(grpc_fd* fd) {
   return (gpr_atm_acq_load(&fd->refst) & 1) ? ret_fd : -1;
 }
 
-static int pollset_epoll_fd_locked(grpc_pollset* pollset);
 static void fd_orphan(grpc_fd* fd, grpc_closure* on_done, int* release_fd,
                       const char* reason) {
   bool is_fd_closed = false;
@@ -452,9 +451,8 @@ static void fd_orphan(grpc_fd* fd, grpc_closure* on_done, int* release_fd,
       if (pollable_obj != nullptr) {  // For PO_FD.
         epoll_ctl(pollable_obj->epfd, EPOLL_CTL_DEL, fd->fd, &ev_fd);
       }
-      for (size_t i = 0; i < fd->pollsets.size(); ++i) {  // For PO_MULTI.
-        grpc_pollset* pollset = fd->pollsets[i];
-        const int epfd = pollset_epoll_fd_locked(pollset);
+      for (size_t i = 0; i < fd->pollset_fds.size(); ++i) {  // For PO_MULTI.
+        const int epfd = fd->pollset_fds[i];
         epoll_ctl(epfd, EPOLL_CTL_DEL, fd->fd, &ev_fd);
       }
     }
@@ -517,9 +515,10 @@ static void fd_notify_on_error(grpc_fd* fd, grpc_closure* closure) {
 }
 
 static bool fd_has_pollset(grpc_fd* fd, grpc_pollset* pollset) {
+  const int epfd = pollset->active_pollable->epfd;
   grpc_core::MutexLock lock(&fd->pollable_mu);
-  for (size_t i = 0; i < fd->pollsets.size(); ++i) {
-    if (fd->pollsets[i] == pollset) {
+  for (size_t i = 0; i < fd->pollset_fds.size(); ++i) {
+    if (fd->pollset_fds[i] == epfd) {
       return true;
     }
   }
@@ -527,8 +526,9 @@ static bool fd_has_pollset(grpc_fd* fd, grpc_pollset* pollset) {
 }
 
 static void fd_add_pollset(grpc_fd* fd, grpc_pollset* pollset) {
+  const int epfd = pollset->active_pollable->epfd;
   grpc_core::MutexLock lock(&fd->pollable_mu);
-  fd->pollsets.push_back(pollset);
+  fd->pollset_fds.push_back(epfd);
 }
 
 /*******************************************************************************
@@ -1292,11 +1292,6 @@ static grpc_error* pollset_as_multipollable_locked(grpc_pollset* pollset,
   return error;
 }
 
-// Caller must hold the lock for `pollset->mu`.
-static int pollset_epoll_fd_locked(grpc_pollset* pollset) {
-  return pollset->active_pollable->epfd;
-}
-
 static void pollset_add_fd(grpc_pollset* pollset, grpc_fd* fd) {
   GPR_TIMER_SCOPE("pollset_add_fd", 0);
 

src/core/lib/json/json.cc

@@ -35,24 +35,21 @@ grpc_json* grpc_json_create(grpc_json_type type) {
 }
 
 void grpc_json_destroy(grpc_json* json) {
+  if (json == nullptr) return;
   while (json->child) {
     grpc_json_destroy(json->child);
   }
-
   if (json->next) {
     json->next->prev = json->prev;
   }
-
   if (json->prev) {
     json->prev->next = json->next;
   } else if (json->parent) {
     json->parent->child = json->next;
   }
-
   if (json->owns_value) {
     gpr_free((void*)json->value);
   }
-
   gpr_free(json);
 }
 

src/core/lib/security/credentials/google_default/google_default_credentials.cc

@@ -272,7 +272,7 @@ end:
   GPR_ASSERT((result == nullptr) + (error == GRPC_ERROR_NONE) == 1);
   if (creds_path != nullptr) gpr_free(creds_path);
   grpc_slice_unref_internal(creds_data);
-  if (json != nullptr) grpc_json_destroy(json);
+  grpc_json_destroy(json);
   *creds = result;
   return error;
 }

src/core/lib/security/credentials/jwt/json_token.cc

@@ -121,7 +121,7 @@ grpc_auth_json_key grpc_auth_json_key_create_from_string(
   char* scratchpad = gpr_strdup(json_string);
   grpc_json* json = grpc_json_parse_string(scratchpad);
   grpc_auth_json_key result = grpc_auth_json_key_create_from_json(json);
-  if (json != nullptr) grpc_json_destroy(json);
+  grpc_json_destroy(json);
   gpr_free(scratchpad);
   return result;
 }

src/core/lib/security/credentials/jwt/jwt_verifier.cc

@@ -666,7 +666,7 @@ static void on_keys_retrieved(void* user_data, grpc_error* error) {
   }
 
 end:
-  if (json != nullptr) grpc_json_destroy(json);
+  grpc_json_destroy(json);
   EVP_PKEY_free(verification_key);
   ctx->user_cb(ctx->user_data, status, claims);
   verifier_cb_ctx_destroy(ctx);
@@ -719,7 +719,7 @@ static void on_openid_config_retrieved(void* user_data, grpc_error* error) {
   return;
 
 error:
-  if (json != nullptr) grpc_json_destroy(json);
+  grpc_json_destroy(json);
   ctx->user_cb(ctx->user_data, GRPC_JWT_VERIFIER_KEY_RETRIEVAL_ERROR, nullptr);
   verifier_cb_ctx_destroy(ctx);
 }

src/core/lib/security/credentials/oauth2/oauth2_credentials.cc

@@ -80,7 +80,7 @@ grpc_auth_refresh_token grpc_auth_refresh_token_create_from_string(
   grpc_json* json = grpc_json_parse_string(scratchpad);
   grpc_auth_refresh_token result =
       grpc_auth_refresh_token_create_from_json(json);
-  if (json != nullptr) grpc_json_destroy(json);
+  grpc_json_destroy(json);
   gpr_free(scratchpad);
   return result;
 }
@@ -199,7 +199,7 @@ end:
   }
   if (null_terminated_body != nullptr) gpr_free(null_terminated_body);
   if (new_access_token != nullptr) gpr_free(new_access_token);
-  if (json != nullptr) grpc_json_destroy(json);
+  grpc_json_destroy(json);
   return status;
 }
 

src/csharp/Grpc.Core/ForwardedTypes.cs

@@ -26,8 +26,6 @@ using Grpc.Core.Utils;
 
 // TODO(jtattermusch): move types needed for implementing a client
 
-[assembly:TypeForwardedToAttribute(typeof(ILogger))]
-[assembly:TypeForwardedToAttribute(typeof(LogLevel))]
 [assembly:TypeForwardedToAttribute(typeof(GrpcPreconditions))]
 [assembly:TypeForwardedToAttribute(typeof(AuthContext))]
 [assembly:TypeForwardedToAttribute(typeof(ContextPropagationOptions))]

summerofcode/2018/naresh.md

@@ -128,7 +128,7 @@ bazel test --spawn_strategy=standalone --genrule_strategy=standalone //src/pytho
 
 - Use `bazel build` with a `-s` flag to see the logs being printed out to
     standard output while building. 
-- Similarly, use `bazel test` with a `--test_output=streamed` to see the the
+- Similarly, use `bazel test` with a `--test_output=streamed` to see the
     test logs while testing. Something to know while using this flag is that all
     tests will be run locally, without sharding, one at a time.
 

templates/README.md

@@ -41,7 +41,7 @@ filegroups:  # groups of files that are automatically expanded
   ...
 libs:  # list of libraries to build
   ...
-target:   # list of targets to build
+targets:   # list of targets to build
   ...
 ```
 

templates/test/cpp/naming/resolver_component_tests_defs.include

@@ -55,7 +55,6 @@ if cur_resolver and cur_resolver != 'ares':
       'needs to use GRPC_DNS_RESOLVER=ares.'))
   test_runner_log('Exit 1 without running tests.')
   sys.exit(1)
-os.environ.update({'GRPC_DNS_RESOLVER': 'ares'})
 os.environ.update({'GRPC_TRACE': 'cares_resolver'})
 
 def wait_until_dns_server_is_up(args,

test/core/bad_client/tests/simple_request.cc

@@ -147,11 +147,12 @@ int main(int argc, char** argv) {
   /* push a window update with bad flags */
   GRPC_RUN_BAD_CLIENT_TEST(failure_verifier, nullptr,
                            PFX_STR "\x00\x00\x00\x08\x10\x00\x00\x00\x01", 0);
-  /* push a window update with bad data */
+  /* push a window update with bad data (0 is not legal window size increment)
+   */
   GRPC_RUN_BAD_CLIENT_TEST(failure_verifier, nullptr,
                            PFX_STR
                            "\x00\x00\x04\x08\x00\x00\x00\x00\x01"
-                           "\xff\xff\xff\xff",
+                           "\x00\x00\x00\x00",
                            0);
   /* push a short goaway */
   GRPC_RUN_BAD_CLIENT_TEST(failure_verifier, nullptr,

test/core/client_channel/resolvers/dns_resolver_test.cc

@@ -75,7 +75,7 @@ int main(int argc, char** argv) {
   test_succeeds(dns, "dns:www.google.com");
   test_succeeds(dns, "dns:///www.google.com");
   char* resolver_env = gpr_getenv("GRPC_DNS_RESOLVER");
-  if (resolver_env == nullptr || gpr_stricmp(resolver_env, "native") == 0) {
+  if (resolver_env != nullptr && gpr_stricmp(resolver_env, "native") == 0) {
     test_fails(dns, "dns://8.8.8.8/8.8.8.8:8888");
   } else {
     test_succeeds(dns, "dns://8.8.8.8/8.8.8.8:8888");

test/core/util/ubsan_suppressions.txt

@@ -21,3 +21,4 @@ enum:grpc_op_string
 signed-integer-overflow:chrono
 enum:grpc_http2_error_to_grpc_status
 enum:grpc_chttp2_cancel_stream
+enum:api_fuzzer

test/cpp/end2end/channelz_service_test.cc

@@ -708,7 +708,7 @@ TEST_F(ChannelzServerTest, GetServerSocketsPaginationTest) {
                                          get_server_sockets_request,
                                          &get_server_sockets_response);
     EXPECT_TRUE(s.ok()) << "s.error_message() = " << s.error_message();
-    // We add one to account the the channelz stub that will end up creating
+    // We add one to account the channelz stub that will end up creating
     // a serversocket.
     EXPECT_EQ(get_server_sockets_response.socket_ref_size(),
               kNumServerSocketsCreated + 1);

test/cpp/end2end/end2end_test.cc

@@ -758,6 +758,18 @@ TEST_P(End2endTest, MultipleRpcs) {
   }
 }
 
+TEST_P(End2endTest, EmptyBinaryMetadata) {
+  ResetStub();
+  EchoRequest request;
+  EchoResponse response;
+  request.set_message("Hello hello hello hello");
+  ClientContext context;
+  context.AddMetadata("custom-bin", "");
+  Status s = stub_->Echo(&context, request, &response);
+  EXPECT_EQ(response.message(), request.message());
+  EXPECT_TRUE(s.ok());
+}
+
 TEST_P(End2endTest, ReconnectChannel) {
   if (GetParam().inproc) {
     return;

test/cpp/naming/resolver_component_tests_runner.py

@@ -55,7 +55,6 @@ if cur_resolver and cur_resolver != 'ares':
       'needs to use GRPC_DNS_RESOLVER=ares.'))
   test_runner_log('Exit 1 without running tests.')
   sys.exit(1)
-os.environ.update({'GRPC_DNS_RESOLVER': 'ares'})
 os.environ.update({'GRPC_TRACE': 'cares_resolver'})
 
 def wait_until_dns_server_is_up(args,

test/cpp/util/channel_trace_proto_helper.cc

@@ -55,7 +55,7 @@ void VaidateProtoJsonTranslation(char* json_c_str) {
   // print_options.always_print_primitive_fields = true;
   s = grpc::protobuf::json::MessageToJsonString(msg, &proto_json_str);
   EXPECT_TRUE(s.ok());
-  // uncomment these to compare the the json strings.
+  // uncomment these to compare the json strings.
   // gpr_log(GPR_ERROR, "tracer json: %s", json_str.c_str());
   // gpr_log(GPR_ERROR, "proto  json: %s", proto_json_str.c_str());
   EXPECT_EQ(json_str, proto_json_str);

tools/internal_ci/linux/grpc_flaky_network.cfg

@@ -0,0 +1,23 @@
+# Copyright 2019 The gRPC Authors
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+# Config file for the internal CI (in protobuf text format)
+
+# Location of the continuous shell script in repository.
+build_file: "grpc/tools/internal_ci/linux/grpc_bazel.sh"
+timeout_mins: 240
+env_vars {
+  key: "BAZEL_SCRIPT"
+  value: "tools/internal_ci/linux/grpc_flaky_network_in_docker.sh"
+}

tools/internal_ci/linux/grpc_flaky_network_in_docker.sh

@@ -0,0 +1,31 @@
+#!/usr/bin/env bash
+# Copyright 2019 The gRPC Authors
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+# Run the flaky network test
+#
+# NOTE: No empty lines should appear in this file before igncr is set!
+set -ex -o igncr || set -ex
+
+mkdir -p /var/local/git
+git clone /var/local/jenkins/grpc /var/local/git/grpc
+(cd /var/local/jenkins/grpc/ && git submodule foreach 'cd /var/local/git/grpc \
+&& git submodule update --init --reference /var/local/jenkins/grpc/${name} \
+${name}')
+cd /var/local/git/grpc
+
+# TODO(jtattermusch): install prerequsites if needed
+
+# TODO(jtattermusch): run the flaky network test instead
+bazel build --spawn_strategy=standalone --genrule_strategy=standalone :all test/... examples/...