Bug 47647 - Tp-qt fails to remove temp avatar file
Summary: Tp-qt fails to remove temp avatar file
Status: RESOLVED FIXED
Alias: None
Product: Telepathy
Classification: Unclassified
Component: tp-qt (show other bugs)
Version: git master
Hardware: Other All
: medium normal
Assignee: Telepathy bugs list
QA Contact: Telepathy bugs list
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-21 06:48 UTC by Martin Klapetek
Modified: 2013-09-02 18:46 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
Remove old avatar file before rename (1.38 KB, patch)
2012-09-07 14:01 UTC, Weng Xuetian
Details | Splinter Review
update the patch. (1.38 KB, patch)
2012-09-07 15:35 UTC, Weng Xuetian
Details | Splinter Review
New proposed patch (1.63 KB, patch)
2012-09-24 13:12 UTC, George Kiagiadakis
Details | Splinter Review

Description Martin Klapetek 2012-03-21 06:48:28 UTC
Currently I have over 206 000 avatar files in ~/.cache/telepathy/avatars/gabble/jabber, mostly duplicate files (same md5) with the same name and different suffix (see examples in the log below). Seems like a tp-qt bug (QTemporaryFile?), maybe even in combination with our apparmor bug [1].

[1] - https://bugs.launchpad.net/ubuntu/+source/telepathy-mission-control-5/+bug/939126

Attached is relevant IRC conversation

<andrunko> sjoerd, mck182 tp-qt and tp-glib saves the user avatars in the cache dir using the avatar token as basis for the filename. Once the token changes it should retrieve a new avatar and save it also. We use the same algorithm to generate the filename, so avatars saved by tp-qt/glib are interchangeable. Old avatars are never cleared from the cache, so it may be your issue, but even so, it seems like a lots of avatars you have there. But note that
<andrunko>  for each avatar there is an accompanying mimetype file saved as $avatar_filename.mime
<sjoerd> andrunko: i know how it works, but that seems a lot of avatars still
<sjoerd> especially if there are duplicates
<sjoerd> as those should have the same token
<andrunko> yeah, it could be a bug somewhere, we would need to debug it
<mck182> let me know if I can provide any info
<sjoerd> mck182: well if you could see if there are avatars in there with the same md5sum that would be interesting
<sjoerd> and ofcourse which servers do you use?
<mck182> gtalk and jabber.org
<mck182> well and a local set up jabber server, prosody
<mck182> but I use that one very occasionaly and for testing stuff only
<mck182> it doesn't even have avatars
-*- mck182 tries to get md5
<mck182> sjoerd andrunko: yes, they all have the same md5
<sjoerd> mck182: so is it one avatar you have *loads* of times or several where that happens
<sjoerd> and can you give us some example filenames for those
<mck182> sjoerd: part of ls -l -- http://paste.kde.org/444032/
<mck182> I can't actually check how much files are the same etc as operating with such folder is close to impossible ;)
<mck182> (dolphin hangs completely, ls takes ages to start..)
<sjoerd> -rw------- 1 mck182 500  1818 Mar 13 22:58 f44b207f27561b88e29cb28f825da3aca222676a.yY5899
<sjoerd> -rw------- 1 mck182 500  1818 Feb 27 23:09 f44b207f27561b88e29cb28f825da3aca222676a.Z10848
<andrunko> mck182, sjoerd  hmm it seems like some issue with temporary files not being properly renamed
<sjoerd> -rw------- 1 mck182 500  1818 Feb 23 12:37 f44b207f27561b88e29cb28f825da3aca222676a.Z11905
<sjoerd> andrunko: exactly what i was going to say
<sjoerd> for some reason it's re-retrieving an avatar it already has and failing to remove the temporary file if it couldn't rename it i guess
<andrunko> yeah, seems so, xclaesse wrote that part
<andrunko> xclaesse, any idea why is this happening? did you have any similar issue?
<andrunko> the code seems to be correct
<andrunko> in ContactManager::onAvatarRetrieved
<mck182> sjoerd andrunko: might this be related to the apparmor bug we've been experiencing with the dconf folder?
<sjoerd> this seems to me more like a tp-qt bug, which *may* be triggered by apparmor though
Comment 1 Martin Klapetek 2012-05-02 01:44:30 UTC
I just checked my Nokia N9 and this bug exists on the device too. I have /lots/ of avatars in ~/.cache/telepathy/avatars/gabble/jabber with the same name and different suffix, exactly the same files. It's a normal device, no modifications done to it.

Telepathy-gabble is 0.12.6 and telepathy-qt4 is 0.6.5 on the device.
Comment 2 Weng Xuetian 2012-09-07 13:34:01 UTC
I think this have nothing to do with apparmor.. since I'm using chakra (so no apparmor here) and also experience same problem here.
Comment 3 Weng Xuetian 2012-09-07 14:01:43 UTC
Created attachment 66794 [details] [review]
Remove old avatar file before rename

From qt docs:
If a file with the name newName already exists, rename() returns false (i.e., QFile will not overwrite it).
Comment 4 Weng Xuetian 2012-09-07 15:35:03 UTC
Created attachment 66805 [details] [review]
update the patch.

Sorry for a little typo without "!"
Comment 5 George Kiagiadakis 2012-09-24 13:05:50 UTC
(In reply to comment #3)
> From qt docs:
> If a file with the name newName already exists, rename() returns false
> (i.e., QFile will not overwrite it).

Indeed, this is the bug. For some reason, avatars are retrieved multiple times, even from the same application, and only the first time succeeds. Later on, temporary files are written but not renamed to the proper avatar file name and not removed either.

This patch addresses the problem, but it is not correct in the sense that it is not atomic. There could be a race condition between two processes trying to retrieve the same avatar at the same time.

This trick with QTemporaryFile is based on g_file_set_contents() that tp-glib uses, which guarantees an atomic operation, but only on unix. On windows, it behaves like this patch (it removes the original file first, then renames). Now, we could implement a similar function in tp-qt, using the posix rename() function, but it wouldn't work on windows for the same reason that it doesn't work for g_file_set_contents() on windows.

Therefore, a better solution, imho, would be to just ignore the new avatar data if the file already exists on the hard drive, since it is guaranteed that two avatars with the same token and from the same CM/protocol have exactly the same contents. I don't really see a reason why we should overwrite the existing data with the same data. Opinions?
Comment 6 George Kiagiadakis 2012-09-24 13:12:10 UTC
Created attachment 67631 [details] [review]
New proposed patch
Comment 7 Weng Xuetian 2012-09-24 14:06:24 UTC
Well, the problem is that people may change theirs avatar, so we still need to update it.

Even if two process will access the same avatar at the same time, I think it won't harm much right?
Comment 8 Simon McVittie 2012-09-24 14:09:42 UTC
(In reply to comment #7)
> Well, the problem is that people may change theirs avatar, so we still need
> to update it.

The idea is that if the avatar token stays the same, it's the same avatar; if I change my avatar, the token changes.

A typical token would be the MD5 or SHA1 of the image, or some encoding of the tuple (contact ID, timestamp of last change).
Comment 9 Weng Xuetian 2012-09-24 16:54:26 UTC
(In reply to comment #8)
> (In reply to comment #7)
> > Well, the problem is that people may change theirs avatar, so we still need
> > to update it.
> 
> The idea is that if the avatar token stays the same, it's the same avatar;
> if I change my avatar, the token changes.
> 
> A typical token would be the MD5 or SHA1 of the image, or some encoding of
> the tuple (contact ID, timestamp of last change).

Ah.. I'm not familar with telepathy, sorry for my dumb question.
But will the out of date avatar be ever cleaned up?
Comment 10 George Kiagiadakis 2012-09-24 18:09:47 UTC
(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > Well, the problem is that people may change theirs avatar, so we still need
> > > to update it.
> > 
> > The idea is that if the avatar token stays the same, it's the same avatar;
> > if I change my avatar, the token changes.
> > 
> > A typical token would be the MD5 or SHA1 of the image, or some encoding of
> > the tuple (contact ID, timestamp of last change).
> 
> Ah.. I'm not familar with telepathy, sorry for my dumb question.
> But will the out of date avatar be ever cleaned up?

No, and that's another problem. Originally I thought that this bug report had to do with the fact that old avatars are never removed, but it turns out there is also this QTemporaryFile rename issue.
Comment 11 Weng Xuetian 2012-10-07 19:49:38 UTC
So.. any updates on this?

This can be serious problem since it slowly fill up users hard disk (I have 2 gigabyte there after last check) and hope this can be resolved soon.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.