Fix a multithreading bug in grpc ClientCall
#5196
Conversation
| GrpcServer(const std::string &name, const uint32_t port) | ||
| : name_(name), port_(port), is_closed_(true) {} | ||
| GrpcServer(std::string name, const uint32_t port) | ||
| : name_(std::move(name)), port_(port), is_closed_(true) {} |
There was a problem hiding this comment.
The previous code seemed clearer to me, why did you change it?
There was a problem hiding this comment.
Clang-tidy suggests this change. Passing by value + move has some advantages over passing by const reference + copy.
The biggest advantage is that you can avoid copying data. E.g.,
std::string name = "....";
GrpcServer(std::move(name)); // no copy at all.
See https://stackoverflow.com/questions/51705967/advantages-of-pass-by-value-and-stdmove-over-pass-by-reference/51706522 for more details.
There was a problem hiding this comment.
Ok interesting, thanks for the explanation :)
|
Test PASSed. |
|
Test FAILed. |
What do these changes do?
Problem.
ClientCallManagerreturns a raw pointer ofClientCall, and the caller can usecall->GetStatus()to tell whether the request was successfully sent. However, there's a small chance that the reply is received too fast. And a call may be deleted beforecall->GetStatus().Solution
This PR fixes this issue by adding a
ClientCallTagand makeClientCallashared_ptr.Also fixes some typos and some small issues reported by clang-tidy.
Related issue number
Linter
scripts/format.shto lint the changes in this PR.