Skip to content

Commit

Permalink
Merge pull request grpc#21680 from vjpai/cq_ordering
Browse files Browse the repository at this point in the history
Log an error (in dbg mode) if CQ is Shutdown before its Server(s)
  • Loading branch information
vjpai authored Jan 17, 2020
2 parents 4466a4c + 6466c7f commit 5969ce6
Show file tree
Hide file tree
Showing 5 changed files with 66 additions and 9 deletions.
36 changes: 34 additions & 2 deletions include/grpcpp/impl/codegen/completion_queue_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,14 @@
#ifndef GRPCPP_IMPL_CODEGEN_COMPLETION_QUEUE_IMPL_H
#define GRPCPP_IMPL_CODEGEN_COMPLETION_QUEUE_IMPL_H

#include <list>

#include <grpc/impl/codegen/atm.h>
#include <grpcpp/impl/codegen/completion_queue_tag.h>
#include <grpcpp/impl/codegen/core_codegen_interface.h>
#include <grpcpp/impl/codegen/grpc_library.h>
#include <grpcpp/impl/codegen/status.h>
#include <grpcpp/impl/codegen/sync.h>
#include <grpcpp/impl/codegen/time.h>

struct grpc_completion_queue;
Expand Down Expand Up @@ -250,6 +253,11 @@ class CompletionQueue : private ::grpc::GrpcLibraryCodegen {
}

private:
// Friends for access to server registration lists that enable checking and
// logging on shutdown
friend class ::grpc_impl::ServerBuilder;
friend class ::grpc_impl::Server;

// Friend synchronous wrappers so that they can access Pluck(), which is
// a semi-private API geared towards the synchronous implementation.
template <class R>
Expand All @@ -274,7 +282,6 @@ class CompletionQueue : private ::grpc::GrpcLibraryCodegen {
friend class ::grpc_impl::internal::TemplatedBidiStreamingHandler;
template <::grpc::StatusCode code>
friend class ::grpc_impl::internal::ErrorMethodHandler;
friend class ::grpc_impl::Server;
friend class ::grpc_impl::ServerContextBase;
friend class ::grpc::ServerInterface;
template <class InputMessage, class OutputMessage>
Expand Down Expand Up @@ -379,13 +386,38 @@ class CompletionQueue : private ::grpc::GrpcLibraryCodegen {
}
}

void RegisterServer(const Server* server) {
#ifndef NDEBUG
grpc::internal::MutexLock l(&server_list_mutex_);
server_list_.push_back(server);
#endif
}
void UnregisterServer(const Server* server) {
#ifndef NDEBUG
grpc::internal::MutexLock l(&server_list_mutex_);
server_list_.remove(server);
#endif
}
bool ServerListEmpty() const {
#ifndef NDEBUG
grpc::internal::MutexLock l(&server_list_mutex_);
return server_list_.empty();
#endif
return true;
}

#ifndef NDEBUG
mutable grpc::internal::Mutex server_list_mutex_;
std::list<const Server*> server_list_ /* GUARDED_BY(server_list_mutex_) */;
#endif

grpc_completion_queue* cq_; // owned

gpr_atm avalanches_in_flight_;
};

/// A specific type of completion queue used by the processing of notifications
/// by servers. Instantiated by \a ServerBuilder.
/// by servers. Instantiated by \a ServerBuilder or Server (for health checker).
class ServerCompletionQueue : public CompletionQueue {
public:
bool IsFrequentlyPolled() { return polling_type_ != GRPC_CQ_NON_LISTENING; }
Expand Down
6 changes: 6 additions & 0 deletions include/grpcpp/server_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -385,6 +385,12 @@ class Server : public grpc::ServerInterface, private grpc::GrpcLibraryCodegen {
// shutdown callback tag (invoked when the CQ is fully shutdown).
// It is protected by mu_
CompletionQueue* callback_cq_ = nullptr;

#ifndef NDEBUG
// List of CQs passed in by user that must be Shutdown only after Server is
// Shutdown.
std::vector<CompletionQueue*> cq_list_;
#endif
};

} // namespace grpc_impl
Expand Down
6 changes: 6 additions & 0 deletions src/cpp/common/completion_queue_cc.cc
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,12 @@ CompletionQueue::CompletionQueue(grpc_completion_queue* take)

void CompletionQueue::Shutdown() {
g_gli_initializer.summon();
#ifndef NDEBUG
if (!ServerListEmpty()) {
gpr_log(GPR_ERROR,
"CompletionQueue shutdown being shutdown before its server.");
}
#endif
CompleteAvalanching();
}

Expand Down
15 changes: 8 additions & 7 deletions src/cpp/server/server_builder.cc
Original file line number Diff line number Diff line change
Expand Up @@ -354,9 +354,8 @@ std::unique_ptr<grpc::Server> ServerBuilder::BuildAndStart() {
// server
// 2. cqs_: Completion queues added via AddCompletionQueue() call

for (const auto& value : *sync_server_cqs) {
grpc_server_register_completion_queue(server->server_, value->cq(),
nullptr);
for (const auto& cq : *sync_server_cqs) {
grpc_server_register_completion_queue(server->server_, cq->cq(), nullptr);
has_frequently_polled_cqs = true;
}

Expand All @@ -369,10 +368,12 @@ std::unique_ptr<grpc::Server> ServerBuilder::BuildAndStart() {
// AddCompletionQueue() API. Some of them may not be frequently polled (i.e by
// calling Next() or AsyncNext()) and hence are not safe to be used for
// listening to incoming channels. Such completion queues must be registered
// as non-listening queues
for (const auto& value : cqs_) {
grpc_server_register_completion_queue(server->server_, value->cq(),
nullptr);
// as non-listening queues. In debug mode, these should have their server list
// tracked since these are provided the user and must be Shutdown by the user
// after the server is shutdown.
for (const auto& cq : cqs_) {
grpc_server_register_completion_queue(server->server_, cq->cq(), nullptr);
cq->RegisterServer(server.get());
}

if (!has_frequently_polled_cqs) {
Expand Down
12 changes: 12 additions & 0 deletions src/cpp/server/server_cc.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1249,6 +1249,9 @@ void Server::Start(grpc::ServerCompletionQueue** cqs, size_t num_cqs) {
}

for (size_t i = 0; i < num_cqs; i++) {
#ifndef NDEBUG
cq_list_.push_back(cqs[i]);
#endif
if (cqs[i]->IsFrequentlyPolled()) {
new UnimplementedAsyncRequest(this, cqs[i]);
}
Expand Down Expand Up @@ -1360,6 +1363,15 @@ void Server::ShutdownInternal(gpr_timespec deadline) {

shutdown_notified_ = true;
shutdown_cv_.Broadcast();

#ifndef NDEBUG
// Unregister this server with the CQs passed into it by the user so that
// those can be checked for properly-ordered shutdown.
for (auto* cq : cq_list_) {
cq->UnregisterServer(this);
}
cq_list_.clear();
#endif
}

void Server::Wait() {
Expand Down

0 comments on commit 5969ce6

Please sign in to comment.