|Title|Timing error in VersioningFileProvider.getPageProvider(String)
|Date|19-Feb-2006 20:49:23 EET
|Version|HEAD (rev 1.17)
|Submitter|BobKerns
|[Bug criticality]|BadBug
|Browser version|
|[Bug status]|ClosedBug
|[PageProvider] used|VersioningFileProvider
|Servlet Container|
|Operating System|
|URL|
|Java version|

I'm labeling this as BadBug, because there may be potential for data loss, depending on what use is being made of the properties...But I spotted this by inspection, not experience; it's probably a low-frequency event with a low probability of actual harm.

Still, it's an easy fix.

These lines have a timing hole:
{{{
            if( m_cachedProperties != null 
                && m_cachedProperties.m_page.equals(page) 
                && m_cachedProperties.m_lastModified == lastModified)
            {
                return m_cachedProperties.m_props;
            }
            ...
}}}

Later it does:
{{{
            ...
            CachedProperties cp = new CachedProperties();
            cp.m_page = page;
            cp.m_lastModified = lastModified;
            cp.m_props = props;
            
            m_cachedProperties = cp; // Atomic
}}}

Note the comment on the write (Atomic), and the lack of atomic read. There being four reads, there are three points of failure, with four different scenarios:
* NullPointerException -- not possible, since it never becomes null once set.
* Comparing lastModified for a different page
** False positive match (requires major timing coincidence on lastModified): wrong properties
** Cache miss due to change after first test: harmless
* Returning different properties than were compared for page/lastModified: wrong properties

To fix, just do the same thing for the read.

{{{
            CachedProperties cp = m_cachedProperties; // Atomic read
            if( cp != null 
                && cp.m_page.equals(page) 
                && cp.m_lastModified == lastModified)
            {
                return cp.m_props;
            }         
            ...            
            cp = new CachedProperties();
            cp.m_page = page;
            cp.m_lastModified = lastModified;
            cp.m_props = props;
            
            m_cachedProperties = cp; // Atomic write
}}}

----

Good catch, thanks!  I'll look into it.

-- JanneJalkanen

----

Fixed in 2.3.81