TitleTiming error in VersioningFileProvider.getPageProvider(String)
Date19-Feb-2006 20:49:23 EET
VersionHEAD (rev 1.17)
SubmitterBobKerns
Bug criticalityBadBug
Browser version
Bug statusClosedBug
PageProvider usedVersioningFileProvider
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

Add new attachment

Only authorized users are allowed to upload new attachments.
« This page (revision-5) was last changed on 21-Feb-2006 20:55 by JanneJalkanen