Fix ItemRetriever in case of concurrent requests for the same item(s)
ClosedPublic

Authored by dfaure on Feb 15 2017, 7:46 AM.

Details

Summary
  • ItemRetrievalManager must emit requestFinished() for those other requests,

otherwise the list of pending requests in ItemRetriever is never emptied

  • ItemRetriever must not assume that a signal being emitted by

ItemRetrievalManager is necessarily about the request it's waiting for, it
could be for another one. So it must first check in its list of pending
requests to determine whether it should react or not.

With multithreaded unittest, checked for races with clang+tsan.
(There is one race, the connect to ItemRetrievalRequest vs the emit
in other threads, we should lock mLock before connect...)

Test Plan

new unittest

Diff Detail

Repository
R165 Akonadi
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
dfaure updated this revision to Diff 11360.Feb 15 2017, 7:46 AM
dfaure retitled this revision from to Fix ItemRetriever in case of concurrent requests for the same item(s).
dfaure updated this object.
dfaure edited the test plan for this revision. (Show Details)
dfaure added a reviewer: dvratil.
dfaure added a subscriber: KDE PIM.
Restricted Application added a project: KDE PIM. · View Herald TranscriptFeb 15 2017, 7:46 AM

If anyone wants to try the patch in master, here's an adapted patch http://www.davidfaure.fr/2017/itemretrievertest.diff

dvratil added inline comments.Feb 16 2017, 10:06 AM
autotests/server/itemretrievertest.cpp
324

Should you maybe thread.wait() here to make sure it finishes before you try to extract the results below?

dfaure added inline comments.Feb 16 2017, 10:09 AM
autotests/server/itemretrievertest.cpp
324

No, I call run() directly, not start(), here.

I admit it's a bit confusing, but at step 0 I wanted to keep the non-multithreaded testcase, to make debugging easier.

dvratil accepted this revision.Feb 16 2017, 10:09 AM
dvratil edited edge metadata.
This revision is now accepted and ready to land.Feb 16 2017, 10:09 AM
This revision was automatically updated to reflect the committed changes.
mwolff added a subscriber: mwolff.Feb 16 2017, 2:44 PM

nice, with the unit test this lgtm, but someone else needs to approve

In D4618#86823, @mwolff wrote:

nice, with the unit test this lgtm, but someone else needs to approve

uhm.. I should reload a page before commenting - sorry for the noise ;-)