Differential D19290

Fix intermittent race in collectionattributetest
ClosedPublic
Actions

Authored by dfaure on Feb 24 2019, 9:54 PM.

Details

Reviewers

dvratil
vkrause

Commits

R165:3a062e6a47f2: Fix intermittent race in collectionattributetest

Summary

Any CollectionModifyJob of a collection with attributes, would send the
full list of attributes, even if they hadn't changed. This leads to the
following problem:

collectionattributetest creates a collection with an attribute
it then proceeds to modify the attribute
then the knut resource's collectionAdded() wants to add a

remoteid to that collection, so it creates a ModifyJob of its own
(and that's where the bug happens: it includes all attributes, with the
orig value).

Therefore the old value overwrites the new value again.

Fixed by only sending attributes if they have changed.

Test Plan

collectionattributetest in a loop, doesn't fail anymore

Diff Detail

Repository

R165 Akonadi

Branch

attribute_stuff

Lint

No Linters Available

Unit

No Unit Test Coverage

Build Status

Buildable 9023
Build 9041: arc lint + arc unit

dfaure created this revision.Feb 24 2019, 9:54 PM

Restricted Application added a project: KDE PIM. · View Herald TranscriptFeb 24 2019, 9:54 PM

dfaure requested review of this revision.Feb 24 2019, 9:54 PM

Harbormaster completed remote builds in B8794: Diff 52490.Feb 24 2019, 9:54 PM

Just for info, Items also send the whole list of attributes (added+changed).

This revision is now accepted and ready to land.Feb 24 2019, 10:06 PM

Hmm, indeed item.cpp has very similar code.

But this means the unittests for item attributes don't cover as many cases as the unittest for collection attributes...

Also, what do you think about that TODO I added?
AFAICS this would work without any changes to modify.cpp, right? Any attribute not sent, is not touched by the server.
Seems to me that I should implement that, for more robustness.

Oh, and this breaks collectionsynctest....

FAIL! : CollectionSyncTest::testAttributeChanges(overwrite local changes) Compared values are not the same

Actual   (resultCol.displayName())       : "foo"
Expected (QString::fromLatin1("default")): "default"
Loc: [/d/kde/src/5/kde/pim/akonadi/autotests/libs/collectionsynctest.cpp(401)]

I agree with the TODO - only sending added/updated attributes should work just fine, the Server already handles it as incremental changes. I also realized that Tags and Relations have attributes too - I wonder if we should have some sort of AttributeStorage helper class that holds the attributes and does the book-keeping to avoid having the code repeated in all four entities.

Regarding the broken test, the question is if the collectionsynctest doesn't only accommodate for the race-y behavior of the current code.

So I started to write unittests for the following scenario: I create an item with an attribute A, and then one session changes A to A', and the other adds an attribute B -- doing that on a copy of the created item, so it will have AB as attributes, and we want to see if the A in there will overwrite A'. That's what we have in mind here, right?

Well, I realized the following: this can never proceed anyway, even if we fix the second job to not send A. This is because the server looks at the item revision and the second session gets a LLCONFLICT.
Sure I can do mjob2->disableRevisionCheck() but then are we really testing a real-world scenario here?

If the second session works with a default constructed item, the cmd.oldRevision is -1 and there's no llconflict check.... but then there's no old attribute value lying around in the item, either.

The bug that started all this was the resource setting a GID, which bypasses the revision check -- but then again that means it's not touching attributes, so the above simple-bool solution is good enough too.

What do you think? Should we forget about that possible scenario since it can't happen, or am I forgetting a case where the revision check is more relaxed and we could still have the A' / AB race? (not necessarily a race as in parallel sessions, I guess the same thing can happen in a single session too, at least A+GID overwriting A' can).

dfaure added a reviewer: vkrause.Mar 1 2019, 8:48 AM

Fix collectionsynctest, calling the non-const attribute() should mark attributes as changed.
Removed the TODO for now, but discussion still open.

This revision is now accepted and ready to land.Mar 1 2019, 2:32 PM

Harbormaster completed remote builds in B9023: Diff 52902.Mar 1 2019, 2:32 PM

You are right, of course, the Item revision check should prevent the race. It only applies to Items though, not to Tags or Relations, so those still might run into a race. However, fixing this for Collections has should have a higher priority, as those are actually used a lot compare to Tags and Relations. So I'm fine with landing this as it is, we can return to the fix for others later...

Ah, good point. So if I want to write a failing test I'll use tags :-)

I'll work on that, but yeah I'll land this first to fix the failing test in CI.

dfaure closed this revision.Mar 3 2019, 9:48 PM

Revision Contents
Changeset List

			Path	Packages
M			src/core/collection.h (3 lines)
M			src/core/collection.cpp (6 lines)
M			src/core/collection_p.h (4 lines)
M			src/core/jobs/collectionmodifyjob.cpp (2 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	52490	a957030		Feb 24 2019, 9:54 PM	★	★
Diff 2	52902	bb50935	Fix collectionsynctest, calling the non-const attribute() should mark…	Mar 1 2019, 2:32 PM	★	★