Skip to content
This repository has been archived by the owner on Mar 28, 2022. It is now read-only.

Commit

Permalink
Use 'exec*()' to perform client and server death tests
Browse files Browse the repository at this point in the history
Before this commit we tried to perform the death part of the client or
server death tests in the forked child process created from
'ASSERT_DEATH()'. This has "worked" but investigating ongoing flaky
tests suggests this approach is insufficient. This commit does less in
the forked child by immediately doing an 'exec*()'.
  • Loading branch information
benh committed Feb 24, 2022
1 parent d31f49d commit 7db4003
Show file tree
Hide file tree
Showing 7 changed files with 324 additions and 94 deletions.
32 changes: 30 additions & 2 deletions test/BUILD.bazel
Original file line number Diff line number Diff line change
@@ -1,4 +1,26 @@
load("@rules_cc//cc:defs.bzl", "cc_test")
load("@rules_cc//cc:defs.bzl", "cc_binary", "cc_test")

cc_binary(
name = "death-client",
srcs = [
"death-client.cc",
],
deps = [
"//:grpc",
"@com_github_grpc_grpc//examples/protos:helloworld_cc_grpc",
],
)

cc_binary(
name = "death-server",
srcs = [
"death-server.cc",
],
deps = [
"//:grpc",
"@com_github_grpc_grpc//examples/protos:helloworld_cc_grpc",
],
)

cc_test(
name = "grpc",
Expand All @@ -13,6 +35,7 @@ cc_test(
"greeter-server.cc",
"helloworld.eventuals.cc",
"helloworld.eventuals.h",
"main.cc",
"multiple-hosts.cc",
"server-death-test.cc",
"server-unavailable.cc",
Expand All @@ -21,13 +44,18 @@ cc_test(
"unary.cc",
"unimplemented.cc",
],
data = [
":death-client",
":death-server",
],
# NOTE: need to add 'linkstatic = True' in order to get this to
# link until https://github.com/grpc/grpc/issues/13856 gets
# resolved.
linkstatic = True,
deps = [
"//:grpc",
"@com_github_google_googletest//:gtest_main",
"@bazel_tools//tools/cpp/runfiles",
"@com_github_google_googletest//:gtest",
"@com_github_grpc_grpc//examples/protos:helloworld_cc_grpc",
"@com_github_grpc_grpc//examples/protos:keyvaluestore",
],
Expand Down
93 changes: 44 additions & 49 deletions test/client-death-test.cc
Original file line number Diff line number Diff line change
@@ -1,9 +1,6 @@
#include "eventuals/eventual.h"
#include "eventuals/grpc/client.h"
#include "eventuals/grpc/server.h"
#include "eventuals/head.h"
#include "eventuals/let.h"
#include "eventuals/loop.h"
#include "eventuals/terminal.h"
#include "eventuals/then.h"
#include "examples/protos/helloworld.grpc.pb.h"
Expand All @@ -14,17 +11,11 @@ using helloworld::Greeter;
using helloworld::HelloReply;
using helloworld::HelloRequest;

using stout::Borrowable;

using eventuals::Eventual;
using eventuals::Head;
using eventuals::Let;
using eventuals::Loop;
using eventuals::Terminate;
using eventuals::Then;

using eventuals::grpc::Client;
using eventuals::grpc::CompletionPool;
using eventuals::grpc::Server;
using eventuals::grpc::ServerBuilder;

Expand All @@ -35,57 +26,57 @@ TEST_F(EventualsGrpcTest, ClientDeathTest) {
// https://github.com/grpc/grpc/issues/14055) and (2) the server can
// send the client it's port.
struct {
int fork[2];
int port[2];
int fork[2]; // 'fork[0]' is for reading, 'fork[1]' for writing.
int port[2]; // 'port[0]' is for reading, 'port[1]' for writing.
} pipes;

ASSERT_NE(-1, pipe(pipes.fork));
ASSERT_NE(-1, pipe(pipes.port));

auto wait_for_fork = [&]() {
auto WaitForFork = [&]() {
int _;
CHECK_GT(read(pipes.fork[0], &_, sizeof(int)), 0);
};

auto notify_forked = [&]() {
int _ = 1;
CHECK_GT(write(pipes.fork[1], &_, sizeof(int)), 0);
};

auto wait_for_port = [&]() {
int port;
CHECK_GT(read(pipes.port[0], &port, sizeof(int)), 0);
return port;
};

auto send_port = [&](int port) {
auto SendPort = [&](int port) {
CHECK_GT(write(pipes.port[1], &port, sizeof(port)), 0);
};

auto client = [&]() {
notify_forked();

int port = wait_for_port();

Borrowable<CompletionPool> pool;

Client client(
"0.0.0.0:" + stringify(port),
grpc::InsecureChannelCredentials(),
pool.Borrow());

auto call = [&]() {
return client.Call<Greeter, HelloRequest, HelloReply>("SayHello")
| Then([](auto&& call) {
exit(1);
});
};

*call();
};

// We use a thread to fork/exec the 'death-client' so that we can
// simultaneously run and wait for the client to die while also
// running the server.
std::thread thread([&]() {
ASSERT_DEATH(client(), "");
std::string path = GetRunfilePathFor("death-client").string();

std::string pipe_fork = std::to_string(pipes.fork[1]);
std::string pipe_port = std::to_string(pipes.port[0]);

// NOTE: doing a 'fork()' when a parent has multiple threads is
// wrought with potential issues because the child only gets a
// single one of those threads (the one that called 'fork()') so
// we ensure here that there is only the single extra thread.
ASSERT_EQ(GetThreadCount(), 2);

ASSERT_DEATH(
[&]() {
// Conventional wisdom is to do the least amount possible
// after a 'fork()', ideally just an 'exec*()', and that's
// what we're doing because when we tried to do more the
// tests were flaky likely do to some library that doesn't
// properly work after doing a 'fork()'.
execl(
path.c_str(),
path.c_str(),
pipe_fork.c_str(),
pipe_port.c_str(),
nullptr);

// NOTE: if 'execve()' failed then this lamdba will return
// and gtest will see consider this "death" to be a failure.
}(),
// NOTE: to avoid false positives we check for a death with
// the string 'connected' printed to stderr.
"connected");
});

// NOTE: we detach the thread so that there isn't a race with the
Expand All @@ -97,7 +88,9 @@ TEST_F(EventualsGrpcTest, ClientDeathTest) {
// test which might have been destructed before it destructs.
thread.detach();

wait_for_fork();
// NOTE: need to wait to call into gRPC till _after_ we've forked
// (see comment at top of test for more details).
WaitForFork();

ServerBuilder builder;

Expand Down Expand Up @@ -128,7 +121,9 @@ TEST_F(EventualsGrpcTest, ClientDeathTest) {

k.Start();

send_port(port);
// NOTE: sending this _after_ we start the eventual so that we're
// ready to accept clients!
SendPort(port);

EXPECT_TRUE(cancelled.get());

Expand Down
77 changes: 77 additions & 0 deletions test/death-client.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
#include "eventuals/grpc/client.h"
#include "eventuals/terminal.h"
#include "eventuals/then.h"
#include "examples/protos/helloworld.grpc.pb.h"

using helloworld::Greeter;
using helloworld::HelloReply;
using helloworld::HelloRequest;

using stout::Borrowable;

using eventuals::Eventual;
using eventuals::Terminate;
using eventuals::Then;

using eventuals::grpc::Client;
using eventuals::grpc::CompletionPool;

// Should only be run from tests!
//
// Expects two arguments.
//
// Expects 'argv[1]' to be a string representing the file descriptor
// that this process has inherited from its parent (the test) that can
// be used to indicate that forking has completed and the test can
// continue.
//
// Expects 'argv[2]' to be a string representing the file descriptor
// that this process has inherited from its parent (the test) that can
// be used to read the bound port of the gRPC server to connect to.
//
// See 'client-death-test.cc' for more details.
int main(int argc, char** argv) {
// TODO(benh): use stout-flags!
CHECK_EQ(argc, 3)
<< "expecting 'pipe_fork' and 'pipe_port' to be passed as arguments";

int pipe_fork = atoi(argv[1]);
int pipe_port = atoi(argv[2]);

auto NotifyForked = [&]() {
int one = 1;
CHECK_GT(write(pipe_fork, &one, sizeof(int)), 0);
};

auto WaitForPort = [&]() {
int port;
CHECK_GT(read(pipe_port, &port, sizeof(int)), 0);
return port;
};

NotifyForked();

int port = WaitForPort();

Borrowable<CompletionPool> pool;

Client client(
"0.0.0.0:" + std::to_string(port),
grpc::InsecureChannelCredentials(),
pool.Borrow());

auto call = [&]() {
return client.Call<Greeter, HelloRequest, HelloReply>("SayHello")
| Then([](auto&& call) {
// NOTE: to avoid false positives with, for example, one
// of the 'CHECK()'s failing above, the 'ClientDeathTest'
// expects the string 'connected' to be written to stderr.
std::cerr << "connected" << std::endl;
exit(1);
});
};

*call();

return 0;
}
75 changes: 75 additions & 0 deletions test/death-server.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
#include "eventuals/grpc/server.h"
#include "eventuals/head.h"
#include "eventuals/terminal.h"
#include "eventuals/then.h"
#include "examples/protos/helloworld.grpc.pb.h"

using helloworld::Greeter;
using helloworld::HelloReply;
using helloworld::HelloRequest;

using eventuals::Head;
using eventuals::Terminate;
using eventuals::Then;

using eventuals::grpc::Server;
using eventuals::grpc::ServerBuilder;

// Should only be run from tests!
//
// Expects one argument.
//
// Expects as 'argv[1]' a string representing the file descriptor that
// this process has inherited from its parent (the test) that can be
// used to send the bound port of the gRPC server.
//
// See 'server-death-test.cc' for more details.
int main(int argc, char** argv) {
// TODO(benh): use stout-flags!
CHECK_EQ(argc, 2) << "expecting 'pipe' to be passed as an argument";

int pipe = atoi(argv[1]);

auto SendPort = [&](int port) {
CHECK_GT(write(pipe, &port, sizeof(port)), 0);
};

ServerBuilder builder;

int port = 0;

builder.AddListeningPort(
"0.0.0.0:0",
grpc::InsecureServerCredentials(),
&port);

auto build = builder.BuildAndStart();

CHECK(build.status.ok());

auto server = std::move(build.server);

CHECK(server);

auto serve = [&]() {
return server->Accept<Greeter, HelloRequest, HelloReply>("SayHello")
| Head()
| Then([](auto&& call) {
// NOTE: to avoid false positives with, for example, one
// of the 'CHECK()'s failing above, the 'ServerDeathTest'
// expects the string 'accepted' to be written to stderr.
std::cerr << "accepted" << std::endl;
exit(1);
});
};

auto [future, k] = Terminate(serve());

k.Start();

// NOTE: sending this _after_ we start the eventual so that we're
// ready to accept clients!
SendPort(port);

future.get();
}
Loading

0 comments on commit 7db4003

Please sign in to comment.