Bug 112439 - Caching in CategoryItemResolverImpl is broken
Caching in CategoryItemResolverImpl is broken
Status: CLOSED WONTFIX
Product: Red Hat Enterprise CMS
Classification: Retired
Component: other (Show other bugs)
6.0
All Linux
medium Severity medium
: ---
: ---
Assigned To: Vadim Nasardinov
Jon Orris
:
: 117208 (view as bug list)
Depends On: 116731
Blocks: 108447 113496
  Show dependency treegraph
 
Reported: 2003-12-19 13:29 EST by Carsten Clasohm
Modified: 2007-04-18 13:00 EDT (History)
1 user (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-07-12 05:08:22 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
instrumentation for obtaining the stack trace in comment 16 (5.52 KB, patch)
2004-02-27 13:00 EST, Vadim Nasardinov
no flags Details | Diff

  None (edit)
Description Carsten Clasohm 2003-12-19 13:29:07 EST
From Bugzilla Helper:
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.5) Gecko/20030925

Description of problem:
CategoryItemResolverImpl.getItem() puts some information in the
attributes of an HttpServletRequest, to be used later by a couple of
other methods. This only works if getItem() is called on every
request, which is no longer the case since ContentSectionServlet
started to cache the result of the getItem() call. Because of the
missing information, CategoryItemResolverImpl.getItemTemplate() no
longer returns the correct template.

For more information, see Scott's mail about this:

http://post-office.corp.redhat.com/archives/ftvi-dev/2003-June/msg00043.html


Version-Release number of selected component (if applicable):


How reproducible:
Always

Steps to Reproduce:
0. Configure CMS to use the CategoryItemResolverImpl class.
1. Create two templates for a content type.
2. Assign them to two different categories.
2. Create a content item, assign the two categories, and publish it.
3. View the item via the URL for the second category.
4. Repeat step 3.


Actual Results:  The first time, the correct template is used, because
ContentSectionServlet's cache is empty and
CategoryItemResolverImpl.getItem() is called. The second time, the
wrong template is used, because nothing was stored in request attributes.


Additional info:

See changelist 38980 for a solution.
Comment 1 Richard Li 2004-02-13 09:32:23 EST
Comments from Vadim: Upon cursory inspection I noticed they put
instances of the inner class ItemURLInfo into CacheTable, but they
don't provide
a reasonable implementation of hashCode() and equals() for this class.
 This will
lead to the same cache eviction behavior that Jon observed in his
recent ticket
(don't have the number handy).  We might want to post to the
ccm-engineering
and remind people to make sure to provide a reasonable hashCode()
implementation for things they put into CacheTable.

Oh and also, it looks like ItemURLInfo should be a static class.  It
doesn't seem to
need a reference to the enclosing class.  Leaving  it non-static is a
needless waste
of memory and GC's resources.
Comment 2 Vadim Nasardinov 2004-02-13 11:15:47 EST
"Jon's ticket" that I was referring to in the above comment is
bug 115066.
Comment 3 Carsten Clasohm 2004-02-18 13:11:23 EST
I have added equals() and hashCode(), the changelist is 40518.
Comment 4 Carsten Clasohm 2004-02-19 06:03:03 EST
Another change to CategoryItemResolverImpl that you can integrate: 40553
Comment 5 Vadim Nasardinov 2004-02-23 12:19:19 EST
Carsten,

I have a couple of questions about this ticket.

 1. How would I go about setting up a server instance where the
    CategoryItemResolverImpl class is actually exercised?  As far as I
    can tell, it's not used anywhere in the vanilla CMS.  It's on my
    list of "dead" classes.  See bug 99096, attachment 96428 [details].


 2. Are there any circumstances under which the cached mapping is
    invalidated and must be evicted?  In the two uses of the
    CacheTable use that I'm familiar with, we have code that evicts
    stale entries, if the underlying data changes in the database.

    For example, if you take a look at SiteNode.java on the trunk,
    you'll notice that both beforeSave() and afterDelete() call
    s_cache.scheduleRefresh().

 3. With your patches applied, CategoryItemResolverImpl seems to be
    caching domain objects across transactions.  I don't see calls to
    disconnect() anywhere.  Please help me understand why is it that
    things don't blow up with an "invalid data object"
    PersistenceException, when the cached Category objects are
    accessed outside the transaction in which they were created.

    I'm basing this question on my previous experience with two cases
    where CacheTable is used.  SiteNode.java contains explicit calls
    to dobj.disconnect() before caching the site nodes.
    BaseDispatcher.java caches instances of ApplicationSpec which
    doesn't reference any data objects.

    So, so it would seem to be based on a cursory look that
    CategoryItemResolverImpl should fail under repeated use.  I can't
    set up a test case for this because of (1).
Comment 6 Carsten Clasohm 2004-02-24 08:54:24 EST
1. Set itemResolverClass and templateResolverClass to
CategoryItemResolverImpl in enterprise.init. This is used by FTVI,
where we have a custom PublishToFile class which uses category-based
URLs instead of the folder-based ones in vanilla CMS. For testing, you
can directly open a URL like

  /ccm/content/categories/health/fitness/test1430.en

2. The cache entry has to be removed if the URL attribute of a
category is modified or if a parent category is removed. In these
cases, the itemURLCache of ContentSectionServlet would have to be
cleaned up.

Not evicting the cache will only cause harm when someone changes a
category so a URL path is no longer valid for it, and then changes a
second category to make it available at the same URL path. Improbable,
but perfectly legal. What I would need for solving this is a simple
way to remove all CacheTable entries whose key starts with a given
prefix.

3. FTVI did a number of benchmarks under heavy load, and they
republish items frequently, so this problem should have shown up by
now. But I will add the disconnect() calls.
Comment 7 Richard Li 2004-02-24 09:04:55 EST
See 40750.
Comment 8 Vadim Nasardinov 2004-02-24 10:25:34 EST
Carsten,

What do you get if you do something like this:

 |tip=> select count(*) from cat_categories;
 | count 
 |-------
 |    43
 |(1 row)
 |
 |tip=> select count(*) from cat_categories where url is null;
 | count 
 |-------
 |    43
 |(1 row)

All of the entries in the cat_categories table appear to have a null
"url" column.
Comment 9 Vadim Nasardinov 2004-02-24 10:26:49 EST
The reason I ask is because I'm running into the following errors.

I updated the content_sections table like so:


 |tip=>   update
 |    content_sections
 |  set
 |    item_resolver_class = 'com.arsdigita.cms.dispatcher.CategoryItemResolverImpl',
 |    template_resolver_class = 'com.arsdigita.cms.dispatcher.CategoryItemResolverImpl'
 |  where
 |    pretty_name = 'content';
 |
 |tip=> select * from content_sections where pretty_name = 'content';
 |-[ RECORD 1 ]----------------+------------------------------------------------------
 |section_id                   | 64
 |pretty_name                  | content
 |root_folder_id               | 60
 |templates_folder_id          | 63
 |staff_group_id               | 59
 |viewers_group_id             | 62
 |page_resolver_class          | com.arsdigita.cms.dispatcher.SimplePageResolver
 |item_resolver_class          | com.arsdigita.cms.dispatcher.CategoryItemResolverImpl
 |template_resolver_class      | com.arsdigita.cms.dispatcher.CategoryItemResolverImpl
 |xml_generator_class          | com.arsdigita.cms.dispatcher.SimpleXMLGenerator
 |content_expiration_digest_id | 118

restarted the server and published an article.  The "Preview" link on
the "Publishing" tab links to
http://el-vadimo.home:9000/ccm/content/preview/categories/null/null/article-one.en

Clicking on this results in the following stack trace:

 |2004-02-24 10:09:40,670 [000-0] ERROR rdbms.RDBMSEngine - select cc.category_id as c_1,
 |       ao.object_type as c_2,
 |       ao.display_name as c_3,
 |       ao.default_domain_class as c_4,
 |       cc.description as c_5,
 |       cc.name as c_6,
 |       cc.url as c_7,
 |       cc.enabled_p as c_8,
 |       cc.abstract_p as c_9,
 |       cc.default_ancestors as c_10
 |from cat_categories cc
 |     join acs_objects ao on ao.object_id = cc.category_id
 |where (exists ( select subquery_id from (select related_category_id as subquery_id from (
 |         select m.related_category_id
 |           from cat_category_category_map m
 |          where m.category_id = :parentID
 |            and m.relation_type = 'child') insub1 ) insub2 where insub2.subquery_id = cc.category_id)) and (cc.url = ?)
 |java.sql.SQLException: ERROR:  parser: parse error at or near ":" at character 584

where character 584 is the colon in :parentID.
Comment 10 Carsten Clasohm 2004-02-24 11:16:11 EST
For FTVI, we use a modified version of
cms/web/WEB-INF/resources/article-categories.xml, which contains an
url attribute for every category. Also, the custom user interface that
was implemented for FTVI makes sure that the URL is defined for
categories used in the CMS.

If you manually set the url column in the database for the categories
that you have assigned to your content item, you should be fine.
Comment 11 Vadim Nasardinov 2004-02-24 11:21:47 EST
Can you give me a partial listing of "select name, url from 
cat_categories"?  I want to make sure I set the "url" column correctly.
Should it start with a slash?  Does it reflect the full path to
the category?  For example, if "Movies" is a child of "Arts",
should its "url" be "movies", "/movies", or "/arts/movies" or
something else entirely?
Comment 12 Carsten Clasohm 2004-02-24 11:42:52 EST
Arts
arts

Movies
movies

Television
television

It is just the URL part belonging to the category, without a slash.
Comment 13 Vadim Nasardinov 2004-02-24 12:23:42 EST
Thanks.

I think I figured out why you aren't getting the same exception that
I showed in comment #9.  Your Category.java's getChildrenByURL
method was fixed by Branimir in change 33210, while the version
on the tip remained broken.

Comment 14 Vadim Nasardinov 2004-02-26 17:25:23 EST
I applied changes 38980, 40518, and 40553 to the trunk in change
40817.

Let me follow up on Question #3 I asked in comment #5.  The answer to
that was (comment #6):

    > FTVI did a number of benchmarks under heavy load, and they
    > republish items frequently, so this problem should have shown up
    > by now.

In other words, Even though it can't possibly work, it does appear to
work just fine.

I notice that the cached itemURLInfo entry seems to be refreshed on
every request, regardless of whether a previously cached entry already
exists:

|$ p4 print //cms/.../CategoryItemResolverImpl.java#7 | head -n 247 | tail -n 5
|         ContentItem returnItem =  getItemFromLangAndBundle(lang,item);
|         if (returnItem != null) {
|             HttpServletRequest request = Web.getRequest();
|             itemURLCachePut(request.getRequestURI(), new ItemURLInfo(cat, cats, isIndex));
|         }

This piece of code is the only place where itemURLCachePut is called,
and the call is performed without first checking whether this url has
been already mapped to an ItemURLInfo instance.

Once cached, the entry is used for the remainder of the request.

So, in effect, as of change 40817, we are caching "category",
"categoryPath", and "isIndex" for the duration of the request.  This,
however, is precisely what we had been doing _prior_ to change 40817,
except we'd been doing it slightly cheaper, because we were putting
cached stuff into request attributes, which is less computationally
intensive than using CacheTable.

Here's the process by which I arrived at this conclusion.  I
instrumented the resolver as follows:

|==== //cms/dev/src/com/arsdigita/cms/dispatcher/CategoryItemResolverImpl.java#7
|@@ -93,6 +93,9 @@
|     {
|        String url = request.getRequestURI();
|         ItemURLInfo itemURLInfo = (ItemURLInfo)s_itemURLCache.get(url);
|+        Logger.getLogger("vadim").debug
|+            ("\nurl=" + url +
|+             "\nidentity=" + System.identityHashCode(itemURLInfo));
|        if (itemURLInfo == null)
|            return null;
|            
|@@ -250,6 +253,8 @@
|      }
| 
|     private static synchronized void itemURLCachePut(String url, ItemURLInfo info) {
|+        Logger.getLogger("vadim").debug("itemURLCachePut: url=" + url + "; info=" +
|+                                        System.identityHashCode(info));
|        s_itemURLCache.put(url, info);
|     }

With this in place, I loaded the following page twice:
http://el-vadimo.boston.redhat.com:9000/ccm/content/preview/categories/Economy/Jobs/article-two.en


First page load:
  2004-02-26 16:44:45,676 [000-0] DEBUG vadim - itemURLCachePut:
   url=/__ccm__/servlet/content-section/preview/categories/Economy/Jobs/article-two.en;
   info=1901414
  2004-02-26 16:44:45,819 [000-0] DEBUG vadim - 
   url=/__ccm__/servlet/content-section/preview/categories/Economy/Jobs/article-two.en
   identity=1901414

Second page load:
  2004-02-26 16:44:59,720 [000-0] DEBUG vadim - itemURLCachePut:
   url=/__ccm__/servlet/content-section/preview/categories/Economy/Jobs/article-two.en;
   info=4404019
  2004-02-26 16:44:59,766 [000-0] DEBUG vadim - 
   url=/__ccm__/servlet/content-section/preview/categories/Economy/Jobs/article-two.en
   identity=4404019

What am I missing?  How is this better than what we had before?
Comment 15 Carsten Clasohm 2004-02-27 03:59:45 EST
You are correct when it comes to the preview pages. When you look at
ContentSectionServlet.getItem(), you see that for preview URLs,
ItemResolver.getItem() is called for every request, while for other
URLs, it is not. So with a category-based template resolver and
without itemURLCache, you would get the correct template the first
time you viewed a live item, and the wrong template on each subsequent
access.
Comment 16 Vadim Nasardinov 2004-02-27 12:58:32 EST
You're right.  For live items, cached ItemURLInfo instances do appear
to be reused across multiple requests.

As for the mystery of non-disconnected categories not blowing up
across transactions, it turns out that
com.arsdigita.persistence.DataObjectImpl performs "auto-disconnect"
under some circumstances.

On the first page load, categories are cached, but not disconnected.

On the second load, these categories are retrieved from the cache.
When we call getURL() on such a category, the underlying
DataObjectImpl instance detects that its get() method is called
outside the data object's originating transaction.  Upon detecting
this, the get() methods calls doDisconnect().  From this point on, the
data object is disconnected:

java.lang.Throwable
	at com.arsdigita.persistence.DataObjectImpl.doDisconnect(DataObjectImpl.java:256)
	at com.arsdigita.persistence.DataObjectImpl.get(DataObjectImpl.java:209)
	at com.arsdigita.domain.DomainObject.get(DomainObject.java:441)
	at com.arsdigita.categorization.Category.getURL(Category.java:466)
	at com.arsdigita.cms.dispatcher.CategoryItemResolverImpl\
            $CategoryTemplateResolver.getItemTemplate(CategoryItemResolverImpl.java:769)

The following piece of code in DataObjectImpl#get(String) is
responsible for this:


    if (isDisconnected()) {
        doDisconnect();
        ...

The isDisconnected() method returns

  m_manualDisconnect || m_transactionDone || !isValid();

This returns true, because m_transactionDone is true.

In light of the above, I'm inclined to leave the implementation as is
and not add any explicit calls to Category#disconnect().
Comment 17 Vadim Nasardinov 2004-02-27 13:00:27 EST
Created attachment 98113 [details]
instrumentation for obtaining the stack trace in comment 16
Comment 18 Vadim Nasardinov 2004-02-27 14:23:07 EST
A couple of comments in closing.

I made some cosmetic edits in changes 40871 and 40872.  It might make
sense for you to integrate these onto the francetv branch.

Regarding my second question in comment #5, I agree with your answer
(comment #6).  This use case is very unlikely in practice.  If it does
arise, it may cause temporary confusion, but the situation will
straighten itself out on its own due to the time-limited LRU eviction
policy implemented in CacheTable.

That's pretty much what I argued in bug 111560, comment #1.  For that
ticket, we decided to go with Dan's recommendation to implement cache
eviction upon detecting changes in the persisted data, despite the
fact that the need for this did not appear to be justified by real
user complaints.

Even though we should do the same thing for this ticket (if no other
reason than consistency), I am perfectly willing to let this slip for
now.

Marking as QA_READY.  Thanks for the patches.
Comment 19 Vadim Nasardinov 2004-02-27 14:39:10 EST
A note for Jon, in case he needs to set up a server to test this.

In stock CMS, the default values for
content_section.item_resolver_class and
content_section.template_resolver_class are:

  |select * from content_sections where pretty_name = 'content';
  |-[ RECORD 1 ]----------------+------------------------------------------------------
  |section_id                   | 1012
  |pretty_name                  | content
  |root_folder_id               | 1008
  |templates_folder_id          | 1011
  |staff_group_id               | 1007
  |viewers_group_id             | 1010
  |page_resolver_class          | com.arsdigita.cms.dispatcher.SimplePageResolver
  |item_resolver_class          | com.arsdigita.cms.dispatcher.MultilingualItemResolver
  |template_resolver_class      | com.arsdigita.cms.dispatcher.DefaultTemplateResolver
  |xml_generator_class          | com.arsdigita.cms.dispatcher.SimpleXMLGenerator
  |content_expiration_digest_id | 1066

To switch an existing CMS instance to use CategoryItemResolverImpl
instead, run this:

  update
    content_sections
  set
    item_resolver_class = 'com.arsdigita.cms.dispatcher.CategoryItemResolverImpl',
    template_resolver_class = 'com.arsdigita.cms.dispatcher.CategoryItemResolverImpl'
  where
    pretty_name = 'content';

The second issue is the fact that all values of cat_categories.url are
null in stock CMS (see comment #8).  In order to test this ticket,
these must be assigned non-null values.  The easiest way to do this is
to run

  update cat_categories set url = name;

The third issue is that stock CMS defines two categorization
hierarchies for the "content" content section: "Navigation" and "Key
Words".  The "Key Words" hierarchy is mapped to the content section in
the "subject" use context.  The "Navigation" hierarchy is mapped in
the null context.  CategoryItemResolverImpl only works with the null
context.  Therefore, you must categorize an item within the
"Navigation" hierarchy.  If you categorize an item within the "Key
Words" hierarchy, then CategoryItemResolverImpl fails to retrieve the
item, given its category path.
Comment 20 Richard Li 2004-03-01 12:10:25 EST
*** Bug 117208 has been marked as a duplicate of this bug. ***
Comment 21 Carsten Clasohm 2006-07-12 05:08:22 EDT
Closing old tickets.

Note You need to log in before you can comment on or make changes to this bug.