Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Fix gRCP context leaks. #904

Merged
merged 2 commits into from
Jul 16, 2019
+34 −38
Merged

fix: Fix gRCP context leaks. #904

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8c7a9ec
fix: Fix gRCP context leaks.
devjgm Jul 13, 2019
6787681
Merge branch 'master' into fix-context-leak
markmandel Jul 15, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions sdks/cpp/include/agones/sdk.h
View file
Open in desktop
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
Open in desktop
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));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

std::move is redundant here and in next calls of set_key and set_value because it accepts argument via const reference.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since C++11, protos also have setters that take rvalues to support efficient moves. To quote from this doc:

void set_foo(string&& value) (C++11 and beyond): Sets the value of the field, moving from the passed string. After calling this, foo() will return a copy of value.

So I think we want to do one of the following:

A: Keep these moves, or ...
B: Change the function's argument types to be const std::string& and not move.

I recommend (A).

request.set_value(std::move(value));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove std::move

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See previous reply.


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));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove std::move

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See previous reply.

request.set_value(std::move(value));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove std::move

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See previous reply.


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

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