Fix leaking of requests in ItemRetriever::exec()

Authored by dfaure on May 10 2019, 11:41 AM.



This one is serious because it impacts actual runtime, not just tests.

Test Plan

ctest -R itemretrievertest in an ASAN build

Diff Detail

R165 Akonadi
itemretriever (branched from Applications/19.04)
No Linters Available
No Unit Test Coverage
Build Status
Buildable 11749
Build 11767: arc lint + arc unit
dfaure requested review of this revision.May 10 2019, 11:41 AM
dfaure created this revision.

That's not the right place do it, it should be done in ItemRetrievalManager.

dfaure added inline comments.May 10 2019, 2:51 PM

I don't agree because the "new" happens here.


... and some requests get deleted here.

dvratil requested changes to this revision.May 10 2019, 3:12 PM

Let's fix this properly and get rid of the new/delete completely. Could you wrap the value in std::shared_ptr/QSharedPointer?

This revision now requires changes to proceed.May 10 2019, 3:12 PM

I don't really like shared_ptr for things like this. To me shared_ptr means "ownership is unclear" (when misused) or "ownership is shared between multiple objects, we need refcounting so that the last user deletes it".
This isn't the case here. ItemRetriever creates the requests, ItemRetrievalManager processes them (but never owns them), and then we need to delete them after use.
A refcount is unnecessary (i.e. overkill) and makes it harder to find out the ownership situation by reading the code.
The owner is clear and unique: it's the requests list. Hence the delete after the removeOne.

Can we make readyItems unique_ptr?

Not readyItems, but requests should then be QVector<std::unique_ptr<ItemRetrievalRequest>> as a clear owner of the object.

dfaure updated this revision to Diff 57893.May 11 2019, 9:00 AM

Port to std::vector<std::unique_ptr<ItemRetrievalRequest>>

The code isn't exactly simpler, in this particular case...

Restricted Application added a project: KDE PIM. · View Herald TranscriptMay 11 2019, 9:00 AM



for (auto it = requests.begin(); it != requests.end(); ++it)
Will be better.

dfaure updated this revision to Diff 57904.May 11 2019, 12:58 PM

Simplify using emplace_back.

dfaure marked an inline comment as done.May 11 2019, 1:00 PM
dfaure added inline comments.

Good suggestion, thanks.


A for loop better than find_if? What makes you say that?
I want to find one element and process it.
With a for loop I'd need to insert an if(), and a break... and it would mean mixing finding and processing. And the reader wouldn't be immediately sure that I'm only looking for one item, until finding the break much further down.

It's general consensus that using an algorithm is always better than writing a for loop.

anthonyfieroni added inline comments.May 11 2019, 5:09 PM

I made a wrong assumption, i mean well readable

for (auto it = requests.begin(); it != requests.end(); ++it) {
    if (it->get() == finishedRequest) {
dfaure marked an inline comment as done.May 12 2019, 7:08 PM

Yes I understood your suggestion to be exactly that. My reply still stands :-)

dvratil accepted this revision.May 13 2019, 10:22 AM

Thanks, David.

This revision is now accepted and ready to land.May 13 2019, 10:22 AM
dfaure closed this revision.May 13 2019, 4:08 PM