Hi Robin,
Thanks for taking a look. I haven't tested this under much load yet, and
based on your comments below, it looks like my understanding of the
async APIs was incorrect.
On 03/17/2011 06:21 AM, Robin Burchell wrote:
Hi Kaitlin,
On 17/03/11 00:22, Kaitlin Rupert wrote:
I've pushed the changes to a remote branch if you want to take a look:
http://meego.gitorious.org/meego-handset-ux/libseaside/commits/async.
I've taken a look through, and while the aim is good, I do see one
problem with this design.
you appear to be storing single instances of many different requests
(QContactSaveRequest instances for updateAvatar, updateContact, etc)
inside SeasideSyncModel. I'm not sure what your reasons are, but at
the least, this won't work as you expect with many requests being
processed at once.
this is because setContact while a request is still running is
probably not a good idea, and at best, start() on an already-running
request is going to return false. So when you have high contention
with multiple running saves, bad things will happen.
I had some concerns about this, but hadn't tested the current code
thoroughly to make sure this wasn't an issue. I should have implemented
something to safe guard against this in the first place though. Agreed
- the approach I have won't work with requests coming in from multiple
applications.
A better option is to abstract your saving of details of contacts to
have an explicit 'commit' functionality somehow, which might look
something like this for saving contacts:-
void SeasideSyncModel::savePendingContacts()
{
QContactLocalId id = priv->uuidToId[uuid];
QContact *contact = priv->idToContact[id];
if (!contact)
return;
QContactSaveRequest *saveRequest = new QContactSaveRequest(this);
connect(saveRequest,
SIGNAL(stateChanged(QContactAbstractRequest::State),
SLOT(onSaveStateChanged(QContactAbstractRequest::State));
saveRequest->setContacts(priv->unsavedContacts);
priv->unsavedContacts.clear();
if (!saveRequest->start()) {
qDebug() << Q_FUNC_INFO << "Save request failed to start";
delete saveRequest;
}
}
void
SeasideSyncModel::onSaveStateChanged(QContactAbstractRequest::State
state)
{
// delete request if it's finished
// log error if it failed for some reason
}
This sounds like an excellent approach. It would also provide a way to
expose errors to the applications. libseaside currently lacks error
handlers for when saveContact() / removeContact() fails.
... and then, presuming that savePendingContacts is a slot, you could
invoke it on a timer set to persist, say, 50-100ms after the last
alteration of data. In addition, this approach also allows you to
easily thread the save request to really prevent UI blocking, should
you wish.
And this would give the application a way to retry in case an error occurs.
If you'd like, I can work on a patch prototyping this approach
sometime in the next day or two.
I won't be able to take a look at libseaside until sometime next week.
So if you have bandwidth to work on this, that would be fantastic. I
can assist with testing.
Thanks!
-Kaitlin
_______________________________________________
MeeGo-dev mailing list
[email protected]
http://lists.meego.com/listinfo/meego-dev
http://wiki.meego.com/Mailing_list_guidelines
