Details
-
Improvement
-
Resolution: Fixed
-
Major
-
None
-
None
-
1
-
KV Sept 2022
Description
TL;DR: Investigate using std::any for Cookie::engine_storage to allow the frontend to own the stored engine data, and destroy it if the operation fails.
Also investigate other potential cleaner solutions.
Currently, the engine-specific data stored in a Cookie while an operation is ewouldblocked is a void*.
In some cases, this requires an object to be allocated in an initial engine call, and a raw pointer be stored in the cookie for use after some background work is complete.
The engine then must ensure the frontend calls into the engine again after being notified, or the object may be leaked.
This boils down to requiring that background tasks in this situation only ever call notifyIOComplete with success, and the "real" status of the operation must be managed separately.
Alternatively, the background task may directly clear the engine storage and free the associated object - this makes the lifetime of the stored object slightly more convoluted, and is easily forgotten when adding new background tasks. This also comes with the typical drawbacks of "manual" delete e.g., thrown exceptions might unwind the stack and skip the clear+free, any and all early exits must account for the cleanup work.
|
Successful flow |
Frontend Engine
|
|
|
getStats()
|
--------------------------------->
|
<Allocates something, schedules background task>
|
engine_errc::would_block
|
<--------------------------------
|
|
|
.
|
.
|
.
|
<background work completes>
|
notifyIOComplete(..., success)
|
<--------------------------------
|
.
|
.
|
.
|
|
|
getStats()
|
--------------------------------->
|
<Retrieves background task result, frees thing>
|
addStat(foo, bar)
|
engine_errc::success
|
<--------------------------------
|
|
|
notifyIOComplete in a failure scenario |
Frontend Engine
|
|
|
getStats()
|
--------------------------------->
|
<Allocates something, schedules background task>
|
engine_errc::would_block
|
<--------------------------------
|
|
|
.
|
.
|
.
|
<background work fails>
|
notifyIOComplete(..., failed)
|
<--------------------------------
|
<allocated object leaked!>
|
|
Workaround |
Frontend Engine
|
|
|
getStats()
|
--------------------------------->
|
<Allocates something, schedules background task>
|
engine_errc::would_block
|
<--------------------------------
|
|
|
.
|
.
|
.
|
<background work fails!>
|
<store real status somewhere>
|
notifyIOComplete(..., success)
|
<--------------------------------
|
.
|
.
|
.
|
|
|
getStats()
|
--------------------------------->
|
<Retrieves background task result, frees thing>
|
<return real status>
|
engine_errc::failed
|
<--------------------------------
|
|
To avoid this issue, Cookie::engine_storage could be made owning. std::any would allow the engine to store and (type-safely) retrieve an arbitrary object, but would also mean the cookie can destroy the contained object if the operation fails.
Attachments
Issue Links
- relates to
-
MB-48816 TSan: Data race on CBStatCollector::addStat
-
- Closed
-