Fix leaking of requests in ItemRetriever::exec()
ClosedPublic

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

Details

Summary

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

Test Plan

ctest -R itemretrievertest in an ASAN build

Diff Detail

Branch
Applications/19.04
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 11720
Build 11738: 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
src/server/storage/itemretriever.cpp
272

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

353

... 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
src/server/storage/itemretriever.cpp
282

emplace_back(lastRequest)

330–331

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.
src/server/storage/itemretriever.cpp
282

Good suggestion, thanks.

330–331

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
src/server/storage/itemretriever.cpp
330–331

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