diff --git a/src/core/ext/filters/client_channel/client_channel.cc b/src/core/ext/filters/client_channel/client_channel.cc index 3272bc3ffed..8a0fabfa6fd 100644 --- a/src/core/ext/filters/client_channel/client_channel.cc +++ b/src/core/ext/filters/client_channel/client_channel.cc @@ -3881,6 +3881,7 @@ void CallData::StartPickLocked(void* arg, grpc_error* error) { GRPC_ERROR_UNREF(result.error); GRPC_CLOSURE_SCHED(&calld->pick_closure_, GRPC_ERROR_REF(disconnect_error)); + if (calld->pick_queued_) calld->RemoveCallFromQueuedPicksLocked(elem); break; } // If wait_for_ready is false, then the error indicates the RPC diff --git a/src/core/ext/transport/chttp2/server/chttp2_server.cc b/src/core/ext/transport/chttp2/server/chttp2_server.cc index 5e9cd77d18a..c0ae64eb2cc 100644 --- a/src/core/ext/transport/chttp2/server/chttp2_server.cc +++ b/src/core/ext/transport/chttp2/server/chttp2_server.cc @@ -266,7 +266,7 @@ static void tcp_server_shutdown_complete(void* arg, grpc_error* error) { // may do a synchronous unref. grpc_core::ExecCtx::Get()->Flush(); if (destroy_done != nullptr) { - destroy_done->cb(destroy_done->cb_arg, GRPC_ERROR_REF(error)); + GRPC_CLOSURE_SCHED(destroy_done, GRPC_ERROR_REF(error)); grpc_core::ExecCtx::Get()->Flush(); } grpc_channel_args_destroy(state->args); diff --git a/src/core/lib/iomgr/tcp_posix.cc b/src/core/lib/iomgr/tcp_posix.cc index 498aecc069b..b6d1381298a 100644 --- a/src/core/lib/iomgr/tcp_posix.cc +++ b/src/core/lib/iomgr/tcp_posix.cc @@ -1020,7 +1020,7 @@ static void tcp_handle_write(void* arg /* grpc_tcp */, grpc_error* error) { if (error != GRPC_ERROR_NONE) { cb = tcp->write_cb; tcp->write_cb = nullptr; - cb->cb(cb->cb_arg, error); + GRPC_CLOSURE_SCHED(cb, GRPC_ERROR_REF(error)); TCP_UNREF(tcp, "write"); return; } @@ -1030,6 +1030,8 @@ static void tcp_handle_write(void* arg /* grpc_tcp */, grpc_error* error) { gpr_log(GPR_INFO, "write: delayed"); } notify_on_write(tcp); + // tcp_flush does not populate error if it has returned false. + GPR_DEBUG_ASSERT(error == GRPC_ERROR_NONE); } else { cb = tcp->write_cb; tcp->write_cb = nullptr; @@ -1037,6 +1039,7 @@ static void tcp_handle_write(void* arg /* grpc_tcp */, grpc_error* error) { const char* str = grpc_error_string(error); gpr_log(GPR_INFO, "write: %s", str); } + // No need to take a ref on error since tcp_flush provides a ref. GRPC_CLOSURE_SCHED(cb, error); TCP_UNREF(tcp, "write"); } diff --git a/src/core/lib/slice/slice_intern.cc b/src/core/lib/slice/slice_intern.cc index d813e5195ad..369135c9eac 100644 --- a/src/core/lib/slice/slice_intern.cc +++ b/src/core/lib/slice/slice_intern.cc @@ -208,7 +208,11 @@ static InternedSliceRefcount* InternNewStringLocked(slice_shard* shard, InternedSliceRefcount* s = static_cast(gpr_malloc(sizeof(*s) + len)); new (s) grpc_core::InternedSliceRefcount(len, hash, shard->strs[shard_idx]); - memcpy(reinterpret_cast(s + 1), buffer, len); + // TODO(arjunroy): Investigate why hpack tried to intern the nullptr string. + // https://github.com/grpc/grpc/pull/20110#issuecomment-526729282 + if (len > 0) { + memcpy(reinterpret_cast(s + 1), buffer, len); + } shard->strs[shard_idx] = s; shard->count++; if (shard->count > shard->capacity * 2) { diff --git a/src/core/lib/transport/static_metadata.cc b/src/core/lib/transport/static_metadata.cc index bd6ff09c80d..f412edb1efe 100644 --- a/src/core/lib/transport/static_metadata.cc +++ b/src/core/lib/transport/static_metadata.cc @@ -1217,12 +1217,7 @@ static const uint16_t elem_keys[] = { 9076, 9185, 9294, 9403, 9512, 9621, 6242, 9730, 9839, 9948, 10057, 10166, 1189, 538, 10275, 10384, 212, 10493, 1195, 1196, 1197, 1198, 1080, 10602, 1843, 11365, 0, 0, 0, 1734, 0, 1850, 0, - 0, 0, 356, 1627, 0, 0, 0, 0, 0, 0, 0, - 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, - 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, - 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, - 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, - 0, 0, 0, 0, 0, 0, 0, 0, 0}; + 0, 0, 356, 1627}; static const uint8_t elem_idxs[] = { 7, 8, 9, 10, 11, 12, 13, 76, 78, 71, 1, 2, 5, 6, 25, 3, 4, 66, 65, 30, 83, 62, 63, 67, 61, 73, 57, 37, 14, 19, 21, 22, diff --git a/src/core/tsi/alts/handshaker/alts_tsi_handshaker.cc b/src/core/tsi/alts/handshaker/alts_tsi_handshaker.cc index 917d375fd82..c5383b3a0ba 100644 --- a/src/core/tsi/alts/handshaker/alts_tsi_handshaker.cc +++ b/src/core/tsi/alts/handshaker/alts_tsi_handshaker.cc @@ -251,7 +251,14 @@ static void on_handshaker_service_resp_recv(void* arg, grpc_error* error) { gpr_log(GPR_ERROR, "ALTS handshaker client is nullptr"); return; } - alts_handshaker_client_handle_response(client, true); + bool success = true; + if (error != GRPC_ERROR_NONE) { + gpr_log(GPR_ERROR, + "ALTS handshaker on_handshaker_service_resp_recv error: %s", + grpc_error_string(error)); + success = false; + } + alts_handshaker_client_handle_response(client, success); } /* gRPC provided callback used when dedicatd CQ and thread are used. diff --git a/test/core/transport/BUILD b/test/core/transport/BUILD index f38ecf2f66b..44e4fcb978e 100644 --- a/test/core/transport/BUILD +++ b/test/core/transport/BUILD @@ -82,6 +82,20 @@ grpc_cc_test( ], ) +grpc_cc_test( + name = "static_metadata_test", + srcs = ["static_metadata_test.cc"], + external_deps = [ + "gtest", + ], + language = "C++", + deps = [ + "//:gpr", + "//:grpc", + "//test/core/util:grpc_test_util", + ], +) + grpc_cc_test( name = "status_conversion_test", srcs = ["status_conversion_test.cc"], diff --git a/test/core/transport/static_metadata_test.cc b/test/core/transport/static_metadata_test.cc new file mode 100644 index 00000000000..6d3d5ef5c37 --- /dev/null +++ b/test/core/transport/static_metadata_test.cc @@ -0,0 +1,51 @@ +/* + * + * Copyright 2019 gRPC authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +#include + +#include + +#include "src/core/lib/transport/metadata.h" +#include "src/core/lib/transport/static_metadata.h" +#include "test/core/util/test_config.h" + +namespace grpc_core { +namespace { + +TEST(StaticMetadataTest, ReadAllStaticElements) { + // This makes sure that all static elements are returned when + // grpc_mdelem_from_slices is called with key pairs pregenerated. + for (int i = 0; i < GRPC_STATIC_MDELEM_COUNT; i++) { + const grpc_mdelem mdelem = grpc_static_mdelem_manifested()[i]; + const grpc_mdelem mdelem2 = + grpc_mdelem_from_slices(GRPC_MDKEY(mdelem), GRPC_MDVALUE(mdelem)); + EXPECT_EQ(mdelem.payload, mdelem2.payload); + } +} + +} // namespace +} // namespace grpc_core + +int main(int argc, char** argv) { + grpc_init(); + grpc::testing::TestEnvironment env(argc, argv); + ::testing::InitGoogleTest(&argc, argv); + int retval = RUN_ALL_TESTS(); + grpc_shutdown(); + return retval; +} diff --git a/test/cpp/README-iOS.md b/test/cpp/README-iOS.md new file mode 100644 index 00000000000..898931085b3 --- /dev/null +++ b/test/cpp/README-iOS.md @@ -0,0 +1,52 @@ +## C++ tests on iOS + +[GTMGoogleTestRunner](https://github.com/google/google-toolbox-for-mac/blob/master/UnitTesting/GTMGoogleTestRunner.mm) is used to convert googletest cases to XCTest that can be run on iOS. GTMGoogleTestRunner doesn't execute the `main` function, so we can't have any test logic in `main`. +However, it's ok to call `::testing::InitGoogleTest` in `main`, as `GTMGoogleTestRunner` [calls InitGoogleTest](https://github.com/google/google-toolbox-for-mac/blob/master/UnitTesting/GTMGoogleTestRunner.mm#L151). +`grpc::testing::TestEnvironment` can also be called from `main`, as it does some test initialization (install crash handler, seed RNG) that's not strictly required to run testcases on iOS. + + +## Porting exising C++ tests to run on iOS + +Please follow these guidelines when porting tests to run on iOS: + +- Tests need to use the googletest framework +- Any setup/teardown code in `main` needs to be moved to `SetUpTestCase`/`TearDownTestCase`, and `TEST` needs to be changed to `TEST_F`. +- [Death tests](https://github.com/google/googletest/blob/master/googletest/docs/advanced.md#death-tests) are not supported on iOS, so use the `*_IF_SUPPORTED()` macros to ensure that your code compiles on iOS. + +For example, the following test +```c++ +TEST(MyTest, TestOne) { + ASSERT_DEATH(ThisShouldDie(), ""); +} + +int main(int argc, char** argv) { + grpc::testing::TestEnvironment env(argc, argv); + ::testing::InitGoogleTest(&argc, argv); + grpc_init(); + return RUN_ALL_TESTS(); + grpc_shutdown(); +} +``` + +should be changed to +```c++ +class MyTest : public ::testing::Test { + protected: + static void SetUpTestCase() { grpc_init(); } + static void TearDownTestCase() { grpc_shutdown(); } +}; + +TEST_F(MyTest, TestOne) { + ASSERT_DEATH_IF_SUPPORTED(ThisShouldDie(), ""); +} + +int main(int argc, char** argv) { + grpc::testing::TestEnvironment env(argc, argv); + ::testing::InitGoogleTest(&argc, argv); + return RUN_ALL_TESTS(); +} +``` + +## Limitations + +Due to a [limitation](https://github.com/google/google-toolbox-for-mac/blob/master/UnitTesting/GTMGoogleTestRunner.mm#L48-L56) in GTMGoogleTestRunner, `SetUpTestCase`/`TeardownTestCase` will be called before/after *every* individual test case, similar to `SetUp`/`TearDown`. diff --git a/test/cpp/microbenchmarks/BUILD b/test/cpp/microbenchmarks/BUILD index fdae8246c31..b8e9b14d4b4 100644 --- a/test/cpp/microbenchmarks/BUILD +++ b/test/cpp/microbenchmarks/BUILD @@ -27,14 +27,6 @@ grpc_cc_test( deps = ["//test/core/util:grpc_test_util"], ) -grpc_cc_binary( - name = "bm_chttp2_transport", - testonly = 1, - srcs = ["bm_chttp2_transport.cc"], - tags = ["no_windows"], - deps = [":helpers"], -) - grpc_cc_library( name = "helpers", testonly = 1, diff --git a/tools/codegen/core/gen_static_metadata.py b/tools/codegen/core/gen_static_metadata.py index 4ac8f9dada7..8560fa47657 100755 --- a/tools/codegen/core/gen_static_metadata.py +++ b/tools/codegen/core/gen_static_metadata.py @@ -627,7 +627,6 @@ def perfect_hash(keys, name): return x + p.r[y] return { - 'PHASHRANGE': p.t - 1 + max(p.r), 'PHASHNKEYS': len(p.slots), 'pyfunc': f, 'code': """ @@ -659,7 +658,7 @@ elem_keys = [ elem_hash = perfect_hash(elem_keys, 'elems') print >> C, elem_hash['code'] -keys = [0] * int(elem_hash['PHASHRANGE']) +keys = [0] * int(elem_hash['PHASHNKEYS']) idxs = [255] * int(elem_hash['PHASHNKEYS']) for i, k in enumerate(elem_keys): h = elem_hash['pyfunc'](k)