Skip to content

Commit

Permalink
fix: Fix gRCP context leaks. (#904)
Browse files Browse the repository at this point in the history 
The calls to gRPC methods were each allocating new `ClientContext`
object, then leaking them. ClientContext ownership is retained by the
caller, so it's the caller's responsibility to delete them. The normal
way this is done is to declare a local stack-allocated context, and pass
a pointer to that.

I also did a few small C++-y cleanups, such as the following:

*  Removed the public declaration for `SDKImpl` by making it a private
   internal class. Also, declare this as a struct rather than class
   since it had all public fields.

*  Removed the kPort int. It was never needed as an int, only a string.
   And it was only ever used in one place. The Google style guide says
   that data should be declared in the smallest scope possible, so
   declaring that at file-scope was too broad. In this case, the best
   solution is to just declare the grpc "target" as a string inline
   since it's needed in exactly one place.

*  Using `std::move()` to efficiently move (not copy) the string value
   function arguments.

*  Fixed formatting in .cc file to match .h file by putting the pointer
   asterisk adjacent to the type (this matches the .h file).

I verified that all the code still compiles by running `make`, but I
have not run any tests because AFAICT, there are no unit tests.
  • Loading branch information
@devjgm @markmandel
devjgm authored and markmandel committed Jul 16, 2019
1 parent 5904de8 commit 787f57f
Show file tree
Hide file tree
Showing 2 changed files with 34 additions and 38 deletions.
3 changes: 1 addition & 2 deletions sdks/cpp/include/agones/sdk.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,6 @@

namespace agones {

class SDKImpl;

// The Agones SDK
class SDK {
public:
Expand Down Expand Up @@ -68,6 +66,7 @@ class SDK {
callback);

private:
struct SDKImpl;
std::unique_ptr<SDKImpl> pimpl_;
};

Expand Down
69 changes: 33 additions & 36 deletions sdks/cpp/src/agones/sdk.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,22 +15,18 @@
#include "agones/sdk.h"

#include <grpcpp/grpcpp.h>

namespace {
const int kPort = 59357;
}
#include <utility>

namespace agones {

class SDKImpl final {
public:
struct SDK::SDKImpl {
std::shared_ptr<grpc::Channel> channel_;
std::unique_ptr<stable::agones::dev::sdk::SDK::Stub> stub_;
std::unique_ptr<grpc::ClientWriter<stable::agones::dev::sdk::Empty>> health_;
};

SDK::SDK() : pimpl_{std::make_unique<SDKImpl>()} {
pimpl_->channel_ = grpc::CreateChannel("localhost:" + std::to_string(kPort),
pimpl_->channel_ = grpc::CreateChannel("localhost:59357",
grpc::InsecureChannelCredentials());
}

Expand All @@ -47,84 +43,85 @@ bool SDK::Connect() {

// make the health connection
stable::agones::dev::sdk::Empty response;
pimpl_->health_ = pimpl_->stub_->Health(new grpc::ClientContext(), &response);
grpc::ClientContext context;
pimpl_->health_ = pimpl_->stub_->Health(&context, &response);

return true;
}

grpc::Status SDK::Ready() {
grpc::ClientContext *context = new grpc::ClientContext();
context->set_deadline(gpr_time_add(gpr_now(GPR_CLOCK_REALTIME),
gpr_time_from_seconds(30, GPR_TIMESPAN)));
grpc::ClientContext context;
context.set_deadline(gpr_time_add(gpr_now(GPR_CLOCK_REALTIME),
gpr_time_from_seconds(30, GPR_TIMESPAN)));
stable::agones::dev::sdk::Empty request;
stable::agones::dev::sdk::Empty response;

return pimpl_->stub_->Ready(context, request, &response);
return pimpl_->stub_->Ready(&context, request, &response);
}

bool SDK::Health() {
stable::agones::dev::sdk::Empty request;
return pimpl_->health_->Write(request);
}

grpc::Status SDK::GameServer(stable::agones::dev::sdk::GameServer *response) {
grpc::ClientContext *context = new grpc::ClientContext();
context->set_deadline(gpr_time_add(gpr_now(GPR_CLOCK_REALTIME),
gpr_time_from_seconds(30, GPR_TIMESPAN)));
grpc::Status SDK::GameServer(stable::agones::dev::sdk::GameServer* response) {
grpc::ClientContext context;
context.set_deadline(gpr_time_add(gpr_now(GPR_CLOCK_REALTIME),
gpr_time_from_seconds(30, GPR_TIMESPAN)));
stable::agones::dev::sdk::Empty request;

return pimpl_->stub_->GetGameServer(context, request, response);
return pimpl_->stub_->GetGameServer(&context, request, response);
}

grpc::Status SDK::WatchGameServer(
const std::function<void(stable::agones::dev::sdk::GameServer)> &callback) {
grpc::ClientContext *context = new grpc::ClientContext();
const std::function<void(stable::agones::dev::sdk::GameServer)>& callback) {
grpc::ClientContext context;
stable::agones::dev::sdk::Empty request;
stable::agones::dev::sdk::GameServer gameServer;

std::unique_ptr<grpc::ClientReader<stable::agones::dev::sdk::GameServer>>
reader = pimpl_->stub_->WatchGameServer(context, request);
reader = pimpl_->stub_->WatchGameServer(&context, request);
while (reader->Read(&gameServer)) {
callback(gameServer);
}
return reader->Finish();
}

grpc::Status SDK::Shutdown() {
grpc::ClientContext *context = new grpc::ClientContext();
context->set_deadline(gpr_time_add(gpr_now(GPR_CLOCK_REALTIME),
gpr_time_from_seconds(30, GPR_TIMESPAN)));
grpc::ClientContext context;
context.set_deadline(gpr_time_add(gpr_now(GPR_CLOCK_REALTIME),
gpr_time_from_seconds(30, GPR_TIMESPAN)));
stable::agones::dev::sdk::Empty request;
stable::agones::dev::sdk::Empty response;

return pimpl_->stub_->Shutdown(context, request, &response);
return pimpl_->stub_->Shutdown(&context, request, &response);
}

grpc::Status SDK::SetLabel(std::string key, std::string value) {
grpc::ClientContext *context = new grpc::ClientContext();
context->set_deadline(gpr_time_add(gpr_now(GPR_CLOCK_REALTIME),
gpr_time_from_seconds(30, GPR_TIMESPAN)));
grpc::ClientContext context;
context.set_deadline(gpr_time_add(gpr_now(GPR_CLOCK_REALTIME),
gpr_time_from_seconds(30, GPR_TIMESPAN)));

stable::agones::dev::sdk::KeyValue request;
request.set_key(key);
request.set_value(value);
request.set_key(std::move(key));
request.set_value(std::move(value));

stable::agones::dev::sdk::Empty response;

return pimpl_->stub_->SetLabel(context, request, &response);
return pimpl_->stub_->SetLabel(&context, request, &response);
}

grpc::Status SDK::SetAnnotation(std::string key, std::string value) {
grpc::ClientContext *context = new grpc::ClientContext();
context->set_deadline(gpr_time_add(gpr_now(GPR_CLOCK_REALTIME),
gpr_time_from_seconds(30, GPR_TIMESPAN)));
grpc::ClientContext context;
context.set_deadline(gpr_time_add(gpr_now(GPR_CLOCK_REALTIME),
gpr_time_from_seconds(30, GPR_TIMESPAN)));

stable::agones::dev::sdk::KeyValue request;
request.set_key(key);
request.set_value(value);
request.set_key(std::move(key));
request.set_value(std::move(value));

stable::agones::dev::sdk::Empty response;

return pimpl_->stub_->SetAnnotation(context, request, &response);
return pimpl_->stub_->SetAnnotation(&context, request, &response);
}
} // namespace agones

0 comments on commit 787f57f

Please sign in to comment.