Commit dfb1afc6 authored by Igor Poboiko's avatar Igor Poboiko
Browse files

[contacts/ContactModifyJob] Avoid race condition

Summary:
This patch is similar to {D28178}. Although `uid` is populated, and we can
freely dispatch both contact and photo modify requests in parallel, the
following race condition still sometimes happens:

 1) Contact modify request dispatched
 2) Contact photo modify request dispatched
 3) Photo reply arrives

At this point, `d->lastContact` is not yet populated, and we call
`processNextContact()`, which repeats steps 1 and 2 for the very same contact.
Which could happen again and again, if photo reply arrives first again.

Instead, I suggest to fire photo modify request only after we receive reply
for contact modify request, ensuring everything happens in the following order:

 1) Contact modify request dispatched
 2) Contact modify reply received
 3) Contact photo modify request dispatched
 4) Contact photo reply received

Test Plan:
 1) Modify a contact via KAddressBook
 2) Check logs
 3) (without patch) Bunch of modify requests somtimes gets fired
 4) (with patch) Only one modify request gets fired

Reviewers: dvratil

Reviewed By: dvratil

Subscribers: kde-pim

Tags: #kde_pim

Differential Revision: https://phabricator.kde.org/D28181
parent 7de9af3d
......@@ -41,6 +41,7 @@ class Q_DECL_HIDDEN ContactModifyJob::Private
public:
Private(ContactModifyJob *parent);
void processNextContact();
void updatePhoto(const ContactPtr &contact);
QueueHelper<ContactPtr> contacts;
ContactPtr lastContact;
......@@ -84,7 +85,10 @@ void ContactModifyJob::Private::processNextContact()
}
q->enqueueRequest(request, rawData, QStringLiteral("application/atom+xml"));
}
void ContactModifyJob::Private::updatePhoto(const ContactPtr &contact)
{
const QUrl photoUrl = ContactsService::photoUrl(q->account()->accountName(), contact->uid());
QNetworkRequest photoRequest(photoUrl);
if (!contact->photo().isEmpty()) {
......@@ -94,7 +98,7 @@ void ContactModifyJob::Private::processNextContact()
q->enqueueRequest(photoRequest, pendingPhoto.first, QStringLiteral("modifyImage"));
} else {
q->enqueueRequest(photoRequest, QByteArray(), QStringLiteral("deleteImage"));
}
}
}
......@@ -154,17 +158,17 @@ ObjectsList ContactModifyJob::handleReplyWithItems(const QNetworkReply *reply, c
if (ct == KGAPI2::JSON) {
d->lastContact = ContactsService::JSONToContact(rawData);
items << d->lastContact;
d->contacts.currentProcessed();
} else if (ct == KGAPI2::XML) {
d->lastContact = ContactsService::XMLToContact(rawData);
items << d->lastContact;
d->contacts.currentProcessed();
} else {
setError(KGAPI2::InvalidResponse);
setErrorString(tr("Invalid response content type"));
emitFinished();
return items;
}
d->updatePhoto(d->contacts.current());
d->contacts.currentProcessed();
} else {
if (d->lastContact && !d->pendingPhoto.first.isEmpty()) {
KContacts::Picture picture;
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment