Move internals of many C++ Async classes to ref-counted struct #5099
Conversation
whenever appropriate so as to avoid any unintentional free-before-use problems. Potential performance issue: this triggers an additional allocation for each Async call initiation, along with the cost of ref-counting shared_ptr . But this is worth it for the additional safety provided here without any change to the exposed C++ API.
|
Can one of the admins verify this patch? |
1 similar comment
|
Can one of the admins verify this patch? |
|
Would it be saner to restrict this refactoring to just those stream objects that also contain SneakyOps? Justification:
|
|
So, I was working with the idea that we should protect against any On Fri, Feb 5, 2016 at 2:04 PM Craig Tiller notifications@github.com
|
|
That's correct. And they're obliged to deal with them. I think this is only a problem when we don't show the user what's going on (to simplify API). |
with a Sneaky member are in a collection.
|
ok, did that.... |
|
Shouldn't |
|
You're right. I was thinking that most of our accesses had "delete this" on them, but that's not for these struct's at all. Yes, we need to drop the ref ourselves. |
|
I believe that this is ready to review |
|
... and I believe that all affected test (C++) pass. |
|
LGTM |
|
Green enough to merge ? |
Move internals of many C++ Async classes to ref-counted struct
The purpose of this is to reduce the likelihood of races caused in the async code without changing the API. The current situation was that an async operation queued multiple entries on the core completion_queue, only some of which get delivered up to C++. If the entries are processed by different threads, a regular (as opposed to Sneaky) CallOpSet could successfully return control to the application thread from CompletionQueue::Next and thus allow the application to delete the Async class that it belongs to; meanwhile, one of the other (sneaky or regular) CallOpSets might still have work to do on a different thread and could have its object destructed before it is invoked.