Don't finish collection sync task too early
AbandonedPublic

Authored by mwolff on Mar 25 2019, 1:28 PM.

Details

Reviewers
dvratil
dfaure
Summary

Only finish tasks in Akonadi::ResourceBasePrivate::changeProcessed
when it is of type ChangeReply. Otherwise, we may end up removing
an unrelated task, such as collection syncing, which then leads
to assertions later on.

BUG: 403642

Test Plan

akonadictl restart regularly asserts on me without this
patch. With this patch, it seems to work - but I have zero clue about
the possible side-effects I may be introducing here. See the bug
report for more details from my tracing endavour.

Diff Detail

Repository
R165 Akonadi
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 10099
Build 10117: arc lint + arc unit
mwolff created this revision.Mar 25 2019, 1:28 PM
Restricted Application added a project: KDE PIM. · View Herald TranscriptMar 25 2019, 1:28 PM
Restricted Application added a subscriber: kde-pim. · View Herald Transcript
mwolff requested review of this revision.Mar 25 2019, 1:28 PM
dvratil added inline comments.Mar 25 2019, 4:25 PM
src/agentbase/resourcebase.cpp
136

I would actually change this into an assert(), move it to the beginning of the function and try to find out what resource implementation calls changeProcessed() when not handling a ChangeReplay task.

The rules are that after successfully finishing a task resource implementation should

for retrieval task, call ResourceBase::*retrievalDone()/ResourceBase::*retrieved()
for add/modify change replay task, call ResourceBase::changeCommitted()
for remove change replay task, call ResourceBase::changeProcessed()
for a custom task, call ResourceBase::taskDone()

In the first 3 cases, the methods will call ResourceBase::taskDone() internally themselves, possibly asynchronously. In any case, you must call taskDone() every time in order to trigger processing of a next task in the ResourceScheduler queue.

mwolff added inline comments.Mar 25 2019, 7:51 PM
src/agentbase/resourcebase.cpp
136

see the last comment here: https://bugs.kde.org/show_bug.cgi?id=403642

agentbase.cpp's collectionChanged calls changeProcessed and then afterwards we get the Akonadi::ResourceBasePrivate::changeCommittedResult signal handler calling changeProcessed again. I naively (i.e. without digging deep) thought that this comes from the same change replay, and it should only call taskDone once, instead of twice like it is doing currently.

Is that enough info for you, or do you need more? How would you "find out what resource implementation" it is? This is always the imap resource which asserts. I can rr record this and send it to you, if you are interested?

dvratil added inline comments.Mar 25 2019, 9:07 PM
src/agentbase/resourcebase.cpp
136

I think I found a root cause for the issue, I attached a patch to your bug report, please test it and let me know.

mwolff abandoned this revision.Mar 26 2019, 9:00 AM

your simplified patch seems to work like a charm, thanks a lot!