Item9264: %QUERY might operate on outdated topic revision
Priority: Urgent
Current State: Closed
Released In: 1.1.0
Target Release: minor
Applies To: Engine
Component:
Branches:
%QUERY must always evaluate the expression on the latest revision. Might happen it gets to see one which cached an older revision.
This problem is hard to reproduce as it is triggered by sideeffects that other plugins have on the Meta cache. It is fixed by
force-reloading the latest revision, as %FORMFIELD does.
--- lib/Foswiki/Macros/QUERY.pm (revision 8036)
+++ lib/Foswiki/Macros/QUERY.pm (working copy)
@@ -13,6 +13,10 @@
$expr = '' unless defined $expr;
my $style = lc( $params->{style} || '' );
+
+ # force-reload latest revision
+ $topicObject->reload(0);
+
# Recursion block.
$this->{evaluatingEval} ||= {};
Me wondering if there's a performance problem here, when every now and then code force-reloads revisions. Wouldn't it be better
to detect the latest revision is loaded, or the cached revision is not the top rev?
--
MichaelDaum - 06 Jul 2010
Kinda. As it says in Meta.pm:
Unloaded objects return undef from getLoadedRev
, or the loaded revision
otherwise. reload
allows you to load different revisions of the same
topic.
So in this case
$topicObject->reload(0) unless defined $topicObject->getLoadedRev()
would be better. Arguably
any topic load should "remember" the top rev, so it can know if the loaded rev is the latest. However that would require loading the entire history whenever a .txt is younger than the corresponding ,v, and I elected not to do that.
--
CrawfordCurrie - 06 Jul 2010
$topicObject->getLoadedRev()
doesn't work as it happily returns 1 when rev 1 was loaded. So it isn't reloading rev 5 which is latest.
--
MichaelDaum - 06 Jul 2010
Foswiki::Meta has a
latestIsLoaded()
method.
$topicObject->reload(0) unless $topicObject->latestIsLoaded();
should do the trick. And before you ask, no, it doesn't load the entire revision history.
--
CrawfordCurrie - 07 Jul 2010
adding the latestIsLoaded() fixes
QUERY. Still each call to latestIsLoaded() results in a fork of rlog. Not so
FORMFIELD: several of them only cost one fork per topic.
--
MichaelDaum - 07 Jul 2010
As we discussed on IRC, the cache that
FORMFIELD uses is in the wrong place, and is unreliable. The right place to cache to avoid repeated rlog calls is in the store impl, because it's there that is the only place that knows a cache is useful. A good store impl will not impose that burden on its callers.
I would have implemented this optimisation, were it not for all the other things I'm doing. It's not going to happen for 1.1 now - but of course, it could be released as a contrib.
--
CrawfordCurrie - 07 Jul 2010
Closing this bug report as its main concern has been fixed. Anything else discussed here needs a new effort and task.
--
MichaelDaum - 07 Jul 2010