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