-
Notifications
You must be signed in to change notification settings - Fork 36
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.
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
Additional fixes for yadu/raii-client. #304
Conversation
Signed-off-by: Chris Lalancette <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for addressing the outstanding issues with this branch! Left some minor comments to consider.
std::move(request_type_support), | ||
std::move(response_type_support) | ||
}); | ||
duplicate_pointers.push_back(client_data); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we store the instantiated client_data
ptr in this vector?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we didn't do this, then we could conceivably get the same address over and over again. Consider the case where we have the pointer 0x1 in deleted_clients
. The first call to new ClientData
might return 0x1. At that point, we'd detect that this is in deleted_clients
, and loop around again. But right before we make the second call to new ClientData
, we drop the reference to the old one, at which point it may be freed. At that point the allocator might decide to give us address 0x1 again, and we'd never make any progress here.
By holding all of the shared_ptr in a vector like this, we guarantee that the allocator will give us a new address. And then at the end of the function, when std::vector
goes out of scope, we'll free all of them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I forgot we need to address the issue with std::move
before we can merge. But apart from that, this generally looks good to me.
Co-authored-by: yadunund <[email protected]> Signed-off-by: Chris Lalancette <[email protected]>
Signed-off-by: Chris Lalancette <[email protected]>
No description provided.