Bug 112439

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.

"Jon's ticket" that I was referring to in the above comment is
bug 115066.

I have added equals() and hashCode(), the changelist is 40518.

Another change to CategoryItemResolverImpl that you can integrate: 40553

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).

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.

See 40750.

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.

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.

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.

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?

Arts
arts

Movies
movies

Television
television

It is just the URL part belonging to the category, without a slash.

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.

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?

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.

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().

Created attachment 98113 [details]
instrumentation for obtaining the stack trace in comment 16

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.

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.

*** Bug 117208 has been marked as a duplicate of this bug. ***

Closing old tickets.