Re: Review Request 111718: Prevent EntityListCache from causing endless loop
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111718/#review36588 --- This review has been submitted with commit 82415e5afc8a977b460615bce4217251735d0da4 by Dan Vrátil to branch KDE/4.11. - Commit Hook On July 26, 2013, 12:47 p.m., Dan Vrátil wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/111718/ > --- > > (Updated July 26, 2013, 12:47 p.m.) > > > Review request for KDEPIM-Libraries and Release Team. > > > Description > --- > > Calling ensureCached() can cause the cache and Monitor to end up in an > endless loop. > > In situation, that there are items 1,2,4,5 and 6 in the cache, calling > ensureCached(1,2,3,4,5) will call request(3) and return false. request() > internally calls shrinkCache() which removes all fetched items to make sure > the cache does not grow indefinitely. After the item 3 is fetched, we end up > with cache with only item 3. Monitor will then again call > ensureCached(1,2,3,4,5), which will call request(1,2,4,5) which in return > calls shrinkCache(), which will remove the item 3 and after fetched is > finished the cache only contains items 1,2,4 and 5. This way the cache ends > up in an endless loop because it will never contain all items that Monitor > requested. > > This patch prevents shrinkCache() from removing items that are part of the > ensureCached() request. > > I would like to get this patch to 4.11, because it's not that difficult to > trigger this bug and in such case you end up with Akonadi resource going nuts > (and not processing any further requests) > > > This implementation is still not optimal, because meanwhile the Monitor can > call ensureCached(7,8,9), which will cause (1,2,4,5) to be removed from cache > anyway, so it can take several tries for EntityListCache to retrieve the > entire set (1,2,3,4,5). However now it's guaranteed that at some point the > cache will succeed. We can rethink (and maybe remove) the cache when > server-side recording is implemented. > > > Diffs > - > > akonadi/entitycache_p.h 5f00917 > > Diff: http://git.reviewboard.kde.org/r/111718/diff/ > > > Testing > --- > > > Thanks, > > Dan Vrátil > > ___ release-team mailing list release-team@kde.org https://mail.kde.org/mailman/listinfo/release-team
Re: Review Request 111718: Prevent EntityListCache from causing endless loop
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111718/ --- (Updated July 27, 2013, 12:04 p.m.) Status -- This change has been marked as submitted. Review request for KDEPIM-Libraries and Release Team. Description --- Calling ensureCached() can cause the cache and Monitor to end up in an endless loop. In situation, that there are items 1,2,4,5 and 6 in the cache, calling ensureCached(1,2,3,4,5) will call request(3) and return false. request() internally calls shrinkCache() which removes all fetched items to make sure the cache does not grow indefinitely. After the item 3 is fetched, we end up with cache with only item 3. Monitor will then again call ensureCached(1,2,3,4,5), which will call request(1,2,4,5) which in return calls shrinkCache(), which will remove the item 3 and after fetched is finished the cache only contains items 1,2,4 and 5. This way the cache ends up in an endless loop because it will never contain all items that Monitor requested. This patch prevents shrinkCache() from removing items that are part of the ensureCached() request. I would like to get this patch to 4.11, because it's not that difficult to trigger this bug and in such case you end up with Akonadi resource going nuts (and not processing any further requests) This implementation is still not optimal, because meanwhile the Monitor can call ensureCached(7,8,9), which will cause (1,2,4,5) to be removed from cache anyway, so it can take several tries for EntityListCache to retrieve the entire set (1,2,3,4,5). However now it's guaranteed that at some point the cache will succeed. We can rethink (and maybe remove) the cache when server-side recording is implemented. Diffs - akonadi/entitycache_p.h 5f00917 Diff: http://git.reviewboard.kde.org/r/111718/diff/ Testing --- Thanks, Dan Vrátil ___ release-team mailing list release-team@kde.org https://mail.kde.org/mailman/listinfo/release-team
Re: Review Request 111718: Prevent EntityListCache from causing endless loop
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111718/#review36558 --- Ship it! >From the release team POV i'd say the code looks simple enough and volker says >yes so let's go Bonus point if you add an autotest :D - Albert Astals Cid On July 26, 2013, 12:47 p.m., Dan Vrátil wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/111718/ > --- > > (Updated July 26, 2013, 12:47 p.m.) > > > Review request for KDEPIM-Libraries and Release Team. > > > Description > --- > > Calling ensureCached() can cause the cache and Monitor to end up in an > endless loop. > > In situation, that there are items 1,2,4,5 and 6 in the cache, calling > ensureCached(1,2,3,4,5) will call request(3) and return false. request() > internally calls shrinkCache() which removes all fetched items to make sure > the cache does not grow indefinitely. After the item 3 is fetched, we end up > with cache with only item 3. Monitor will then again call > ensureCached(1,2,3,4,5), which will call request(1,2,4,5) which in return > calls shrinkCache(), which will remove the item 3 and after fetched is > finished the cache only contains items 1,2,4 and 5. This way the cache ends > up in an endless loop because it will never contain all items that Monitor > requested. > > This patch prevents shrinkCache() from removing items that are part of the > ensureCached() request. > > I would like to get this patch to 4.11, because it's not that difficult to > trigger this bug and in such case you end up with Akonadi resource going nuts > (and not processing any further requests) > > > This implementation is still not optimal, because meanwhile the Monitor can > call ensureCached(7,8,9), which will cause (1,2,4,5) to be removed from cache > anyway, so it can take several tries for EntityListCache to retrieve the > entire set (1,2,3,4,5). However now it's guaranteed that at some point the > cache will succeed. We can rethink (and maybe remove) the cache when > server-side recording is implemented. > > > Diffs > - > > akonadi/entitycache_p.h 5f00917 > > Diff: http://git.reviewboard.kde.org/r/111718/diff/ > > > Testing > --- > > > Thanks, > > Dan Vrátil > > ___ release-team mailing list release-team@kde.org https://mail.kde.org/mailman/listinfo/release-team
Re: Review Request 111718: Prevent EntityListCache from causing endless loop
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111718/#review36553 --- Ship it! Looks like it should fix the immediate issue indeed. Thanks! - Volker Krause On July 26, 2013, 12:47 p.m., Dan Vrátil wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/111718/ > --- > > (Updated July 26, 2013, 12:47 p.m.) > > > Review request for KDEPIM-Libraries and Release Team. > > > Description > --- > > Calling ensureCached() can cause the cache and Monitor to end up in an > endless loop. > > In situation, that there are items 1,2,4,5 and 6 in the cache, calling > ensureCached(1,2,3,4,5) will call request(3) and return false. request() > internally calls shrinkCache() which removes all fetched items to make sure > the cache does not grow indefinitely. After the item 3 is fetched, we end up > with cache with only item 3. Monitor will then again call > ensureCached(1,2,3,4,5), which will call request(1,2,4,5) which in return > calls shrinkCache(), which will remove the item 3 and after fetched is > finished the cache only contains items 1,2,4 and 5. This way the cache ends > up in an endless loop because it will never contain all items that Monitor > requested. > > This patch prevents shrinkCache() from removing items that are part of the > ensureCached() request. > > I would like to get this patch to 4.11, because it's not that difficult to > trigger this bug and in such case you end up with Akonadi resource going nuts > (and not processing any further requests) > > > This implementation is still not optimal, because meanwhile the Monitor can > call ensureCached(7,8,9), which will cause (1,2,4,5) to be removed from cache > anyway, so it can take several tries for EntityListCache to retrieve the > entire set (1,2,3,4,5). However now it's guaranteed that at some point the > cache will succeed. We can rethink (and maybe remove) the cache when > server-side recording is implemented. > > > Diffs > - > > akonadi/entitycache_p.h 5f00917 > > Diff: http://git.reviewboard.kde.org/r/111718/diff/ > > > Testing > --- > > > Thanks, > > Dan Vrátil > > ___ release-team mailing list release-team@kde.org https://mail.kde.org/mailman/listinfo/release-team
Review Request 111718: Prevent EntityListCache from causing endless loop
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111718/ --- Review request for KDEPIM-Libraries and Release Team. Description --- Calling ensureCached() can cause the cache and Monitor to end up in an endless loop. In situation, that there are items 1,2,4,5 and 6 in the cache, calling ensureCached(1,2,3,4,5) will call request(3) and return false. request() internally calls shrinkCache() which removes all fetched items to make sure the cache does not grow indefinitely. After the item 3 is fetched, we end up with cache with only item 3. Monitor will then again call ensureCached(1,2,3,4,5), which will call request(1,2,4,5) which in return calls shrinkCache(), which will remove the item 3 and after fetched is finished the cache only contains items 1,2,4 and 5. This way the cache ends up in an endless loop because it will never contain all items that Monitor requested. This patch prevents shrinkCache() from removing items that are part of the ensureCached() request. I would like to get this patch to 4.11, because it's not that difficult to trigger this bug and in such case you end up with Akonadi resource going nuts (and not processing any further requests) This implementation is still not optimal, because meanwhile the Monitor can call ensureCached(7,8,9), which will cause (1,2,4,5) to be removed from cache anyway, so it can take several tries for EntityListCache to retrieve the entire set (1,2,3,4,5). However now it's guaranteed that at some point the cache will succeed. We can rethink (and maybe remove) the cache when server-side recording is implemented. Diffs - akonadi/entitycache_p.h 5f00917 Diff: http://git.reviewboard.kde.org/r/111718/diff/ Testing --- Thanks, Dan Vrátil ___ release-team mailing list release-team@kde.org https://mail.kde.org/mailman/listinfo/release-team
