fix: Fix gRCP context leaks. #904
Conversation
|
@dsazonoff any issues? |
markmandel
added
area/performance
|
Build Succeeded 👏 Build Id: 603049a6-6a5b-4f59-b04b-ec2338c4a0c7 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
|
|
||
| stable::agones::dev::sdk::KeyValue request; | ||
| request.set_key(key); | ||
| request.set_value(value); | ||
| request.set_key(std::move(key)); |
There was a problem hiding this comment.
std::move is redundant here and in next calls of set_key and set_value because it accepts argument via const reference.
There was a problem hiding this comment.
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_key(key); | ||
| request.set_value(value); | ||
| request.set_key(std::move(key)); | ||
| request.set_value(std::move(value)); |
|
|
||
| stable::agones::dev::sdk::KeyValue request; | ||
| request.set_key(key); | ||
| request.set_value(value); | ||
| request.set_key(std::move(key)); |
| request.set_key(key); | ||
| request.set_value(value); | ||
| request.set_key(std::move(key)); | ||
| request.set_value(std::move(value)); |
|
Build Failed 😱 Build Id: 046a1317-bee5-405d-928d-7c60fefc1121 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
|
I think the build failure is an unrelated flake. |
|
@markmandel could you restart a build, please. Code is Ok. |
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.
|
Build Succeeded 👏 Build Id: 1a2cdbc2-9d6f-430c-826f-e67f7b94781f The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
|
Build Succeeded 👏 Build Id: 89d12e4e-4c84-46fd-87ac-ead21569a5e5 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
The calls to gRPC methods were each allocating new
ClientContextobject, 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
SDKImplby making it a privateinternal 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 valuefunction 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 Ihave not run any tests because AFAICT, there are no unit tests.