diff --git a/config/alfresco/ehcache-default.xml b/config/alfresco/ehcache-default.xml index 04321bc64c..ba175cb324 100644 --- a/config/alfresco/ehcache-default.xml +++ b/config/alfresco/ehcache-default.xml @@ -362,14 +362,4 @@ statistics="false" timeToLiveSeconds="60" /> - - - diff --git a/config/alfresco/extension/caching-content-store-context.xml.sample b/config/alfresco/extension/caching-content-store-context.xml.sample index 09203ade9d..1e85bae54d 100644 --- a/config/alfresco/extension/caching-content-store-context.xml.sample +++ b/config/alfresco/extension/caching-content-store-context.xml.sample @@ -18,7 +18,7 @@ - + @@ -51,6 +51,13 @@ org.alfresco.cache.cachingContentStoreCache + + + + + + + @@ -70,7 +77,7 @@ - + @@ -83,7 +90,7 @@ - ${system.content.cachedContentCleanup.cronExpression} + ${system.content.caching.contentCleanup.cronExpression} diff --git a/config/alfresco/repository.properties b/config/alfresco/repository.properties index fa65081396..6f3acd5ac6 100644 --- a/config/alfresco/repository.properties +++ b/config/alfresco/repository.properties @@ -747,3 +747,16 @@ publishing.root=${publishing.root.path}/${spaces.publishing.root.childname} urlshortening.bitly.username=brianalfresco urlshortening.bitly.api.key=R_ca15c6c89e9b25ccd170bafd209a0d4f urlshortening.bitly.url.length=20 + + +# +# Caching Content Store +# +system.content.caching.cacheOnInbound=true +system.content.caching.maxDeleteWatchCount=1 +# Clean up every day at 3 am +system.content.caching.contentCleanup.cronExpression=0 0 3 * * ? +system.content.caching.timeToLiveSeconds=0 +system.content.caching.timeToIdleSeconds=86400 +system.content.caching.maxElementsInMemory=5000 +system.content.caching.maxElementsOnDisk=10000 diff --git a/source/java/org/alfresco/repo/content/caching/CachingContentStore.java b/source/java/org/alfresco/repo/content/caching/CachingContentStore.java index 0e02185d16..27c805271b 100644 --- a/source/java/org/alfresco/repo/content/caching/CachingContentStore.java +++ b/source/java/org/alfresco/repo/content/caching/CachingContentStore.java @@ -51,6 +51,7 @@ public class CachingContentStore implements ContentStore private ContentStore backingStore; private ContentCache cache; private boolean cacheOnInbound; + private int maxCacheTries = 2; static { @@ -175,30 +176,23 @@ public class CachingContentStore implements ContentStore return cacheAndRead(contentUrl); } + private ContentReader cacheAndRead(String url) { WriteLock writeLock = readWriteLock(url).writeLock(); writeLock.lock(); try { - if (!cache.contains(url)) + for (int i = 0; i < maxCacheTries; i++) { - if (cache.put(url, backingStore.getReader(url))) + ContentReader reader = attemptCacheAndRead(url); + if (reader != null) { - return cache.getReader(url); - } - else - { - return backingStore.getReader(url); + return reader; } } - else - { - return cache.getReader(url); - } - } - catch(CacheMissException e) - { + // Have tried multiple times to cache the item and read it back from the cache + // but there is a recurring problem - give up and return the item from the backing store. return backingStore.getReader(url); } finally @@ -207,6 +201,31 @@ public class CachingContentStore implements ContentStore } } + private ContentReader attemptCacheAndRead(String url) + { + ContentReader reader = null; + try + { + if (!cache.contains(url)) + { + if (cache.put(url, backingStore.getReader(url))) + { + reader = cache.getReader(url); + } + } + else + { + reader = cache.getReader(url); + } + } + catch(CacheMissException e) + { + cache.remove(url); + } + + return reader; + } + /* * @see org.alfresco.repo.content.ContentStore#getWriter(org.alfresco.repo.content.ContentContext) */ @@ -279,10 +298,40 @@ public class CachingContentStore implements ContentStore @Override public boolean delete(String contentUrl) { - if (cache.contains(contentUrl)) - cache.remove(contentUrl); + ReentrantReadWriteLock readWriteLock = readWriteLock(contentUrl); + ReadLock readLock = readWriteLock.readLock(); + readLock.lock(); + try + { + if (!cache.contains(contentUrl)) + { + // The item isn't in the cache, so simply delete from the backing store + return backingStore.delete(contentUrl); + } + } + finally + { + readLock.unlock(); + } - return backingStore.delete(contentUrl); + WriteLock writeLock = readWriteLock.writeLock(); + writeLock.lock(); + try + { + // Double check the content still exists in the cache + if (cache.contains(contentUrl)) + { + // The item is in the cache, so remove. + cache.remove(contentUrl); + + } + // Whether the item was in the cache or not, it must still be deleted from the backing store. + return backingStore.delete(contentUrl); + } + finally + { + writeLock.unlock(); + } } /** diff --git a/source/java/org/alfresco/repo/content/caching/CachingContentStoreTest.java b/source/java/org/alfresco/repo/content/caching/CachingContentStoreTest.java index 07c614ca53..1b3e592b85 100644 --- a/source/java/org/alfresco/repo/content/caching/CachingContentStoreTest.java +++ b/source/java/org/alfresco/repo/content/caching/CachingContentStoreTest.java @@ -23,7 +23,9 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertSame; import static org.junit.Assert.assertTrue; +import static org.mockito.Matchers.any; import static org.mockito.Matchers.anyString; +import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; @@ -45,6 +47,7 @@ import org.junit.Test; import org.junit.runner.RunWith; import org.mockito.ArgumentCaptor; import org.mockito.Mock; +import org.mockito.Mockito; import org.mockito.runners.MockitoJUnitRunner; /** @@ -85,16 +88,40 @@ public class CachingContentStoreTest @Test - public void getReaderForItemMissingFromCache() + public void getReaderForItemMissingFromCacheWillGiveUpAfterRetrying() { ContentReader sourceContent = mock(ContentReader.class); when(cache.getReader("url")).thenThrow(new CacheMissException("url")); when(backingStore.getReader("url")).thenReturn(sourceContent); when(cache.put("url", sourceContent)).thenReturn(true); - cachingStore.getReader("url"); + ContentReader returnedReader = cachingStore.getReader("url"); + + // Upon failure, item is removed from cache + verify(cache, Mockito.atLeastOnce()).remove("url"); + + // The content comes direct from the backing store + assertSame(returnedReader, sourceContent); } + + @Test + public void getReaderForItemMissingFromCacheWillRetryAndCanSucceed() + { + ContentReader sourceContent = mock(ContentReader.class); + ContentReader cachedContent = mock(ContentReader.class); + when(cache.getReader("url")). + thenThrow(new CacheMissException("url")). + thenReturn(cachedContent); + when(backingStore.getReader("url")).thenReturn(sourceContent); + when(cache.put("url", sourceContent)).thenReturn(true); + + ContentReader returnedReader = cachingStore.getReader("url"); + + assertSame(returnedReader, cachedContent); + } + + @Test public void getReaderForItemMissingFromCacheButNoContentToCache() { @@ -144,7 +171,35 @@ public class CachingContentStoreTest verify(backingStore).getWriter(ctx); } - + + + @Test(expected=RuntimeException.class) + // Check that exceptions raised by the backing store's putContent(ContentReader) + // aren't swallowed and can therefore cause the transaction to fail. + public void exceptionRaisedWhenCopyingTempToBackingStoreIsPropogatedCorrectly() + throws ContentIOException, IOException + { + cachingStore = new CachingContentStore(backingStore, cache, true); + ContentContext ctx = ContentContext.NULL_CONTEXT; + ContentWriter bsWriter = mock(ContentWriter.class); + when(backingStore.getWriter(ctx)).thenReturn(bsWriter); + when(bsWriter.getContentUrl()).thenReturn("url"); + ContentWriter cacheWriter = mock(ContentWriter.class); + when(cache.getWriter("url")).thenReturn(cacheWriter); + ContentReader readerFromCacheWriter = mock(ContentReader.class); + when(cacheWriter.getReader()).thenReturn(readerFromCacheWriter); + + doThrow(new RuntimeException()).when(bsWriter).putContent(any(ContentReader.class)); + + cachingStore.getWriter(ctx); + + // Get the stream listener and trigger it + ArgumentCaptor arg = ArgumentCaptor.forClass(ContentStreamListener.class); + verify(cacheWriter).addListener(arg.capture()); + // Simulate a stream close + arg.getValue().contentStreamClosed(); + } + @Test public void encodingAttrsCopiedToBackingStoreWriter() diff --git a/source/java/org/alfresco/repo/content/caching/ContentCacheImpl.java b/source/java/org/alfresco/repo/content/caching/ContentCacheImpl.java index 95388faaea..21c19a9700 100644 --- a/source/java/org/alfresco/repo/content/caching/ContentCacheImpl.java +++ b/source/java/org/alfresco/repo/content/caching/ContentCacheImpl.java @@ -107,6 +107,12 @@ public class ContentCacheImpl implements ContentCache if (memoryStore.contains(url)) { String path = memoryStore.get(url); + + // Getting the path for a URL from the memoryStore will reset the timeToIdle for + // that URL. It is important to perform a reverse lookup as well to ensure that the + // cache file path to URL mapping is also kept in the cache. + memoryStore.get(Key.forCacheFile(path)); + File cacheFile = new File(path); if (cacheFile.exists()) { diff --git a/source/java/org/alfresco/repo/content/caching/ContentCacheImplTest.java b/source/java/org/alfresco/repo/content/caching/ContentCacheImplTest.java new file mode 100644 index 0000000000..5388446fd1 --- /dev/null +++ b/source/java/org/alfresco/repo/content/caching/ContentCacheImplTest.java @@ -0,0 +1,228 @@ +/* + * Copyright (C) 2005-2011 Alfresco Software Limited. + * + * This file is part of Alfresco + * + * Alfresco is free software: you can redistribute it and/or modify + * it under the terms of the GNU Lesser General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * Alfresco is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with Alfresco. If not, see . + */ +package org.alfresco.repo.content.caching; + + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +import java.io.File; + +import org.alfresco.repo.cache.SimpleCache; +import org.alfresco.repo.content.filestore.FileContentReader; +import org.alfresco.repo.content.filestore.FileContentWriter; +import org.alfresco.service.cmr.repository.ContentReader; +import org.alfresco.util.TempFileProvider; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.ArgumentCaptor; +import org.mockito.Mock; +import org.mockito.Mockito; +import org.mockito.runners.MockitoJUnitRunner; + +/** + * Tests for the ContentCacheImpl class. + * + * @author Matt Ward + */ +@RunWith(MockitoJUnitRunner.class) +public class ContentCacheImplTest +{ + private ContentCacheImpl contentCache; + private @Mock SimpleCache lookupTable; + + + @Before + public void setUp() throws Exception + { + contentCache = new ContentCacheImpl(); + contentCache.setMemoryStore(lookupTable); + contentCache.setCacheRoot(TempFileProvider.getTempDir()); + } + + + @Test + public void canGetReaderForItemInCacheHavingLiveFile() + { + final String url = "store://content/url.bin"; + Mockito.when(lookupTable.contains(Key.forUrl(url))).thenReturn(true); + final String path = tempfile().getAbsolutePath(); + Mockito.when(lookupTable.get(Key.forUrl(url))).thenReturn(path); + + FileContentReader reader = (FileContentReader) contentCache.getReader(url); + + assertEquals("Reader should have correct URL", url, reader.getContentUrl()); + assertEquals("Reader should be for correct cached content file", path, reader.getFile().getAbsolutePath()); + // Important the get(path) was called, so that the timeToIdle is reset + // for the 'reverse lookup' as well as the URL to path mapping. + Mockito.verify(lookupTable).get(Key.forCacheFile(path)); + } + + + @Test(expected=CacheMissException.class) + public void getReaderForItemInCacheButMissingContentFile() + { + final String url = "store://content/url.bin"; + Mockito.when(lookupTable.contains(Key.forUrl(url))).thenReturn(true); + final String path = "/no/content/file/at/this/path.bin"; + Mockito.when(lookupTable.get(Key.forUrl(url))).thenReturn(path); + + try + { + contentCache.getReader(url); + } + finally + { + // Important the get(path) was called, so that the timeToIdle is reset + // for the 'reverse lookup' as well as the URL to path mapping. + Mockito.verify(lookupTable).get(Key.forCacheFile(path)); + } + } + + + private File tempfile() + { + File file = TempFileProvider.createTempFile("cached-content", ".bin"); + file.deleteOnExit(); + return file; + } + + + @Test(expected=CacheMissException.class) + public void getReaderWhenItemNotInCache() + { + final String url = "store://content/url.bin"; + Mockito.when(lookupTable.contains(Key.forUrl(url))).thenReturn(false); + + contentCache.getReader(url); + } + + + @Test + public void contains() + { + final String url = "store://content/url.bin"; + + Mockito.when(lookupTable.contains(Key.forUrl(url))).thenReturn(true); + assertTrue(contentCache.contains(Key.forUrl(url))); + assertTrue(contentCache.contains(url)); + + Mockito.when(lookupTable.contains(Key.forUrl(url))).thenReturn(false); + assertFalse(contentCache.contains(Key.forUrl(url))); + assertFalse(contentCache.contains(url)); + } + + + @Test + public void putIntoLookup() + { + final Key key = Key.forUrl("store://some/url"); + final String value = "/some/path"; + + contentCache.putIntoLookup(key, value); + + Mockito.verify(lookupTable).put(key, value); + } + + + @Test + public void getCacheFilePath() + { + final String url = "store://some/url.bin"; + final String expectedPath = "/some/cache/file/path"; + Mockito.when(lookupTable.get(Key.forUrl(url))).thenReturn(expectedPath); + + String path = contentCache.getCacheFilePath(url); + + assertEquals("Paths must match", expectedPath, path); + } + + + @Test + public void getContentUrl() + { + final File cacheFile = new File("/some/path"); + final String expectedUrl = "store://some/url"; + Mockito.when(lookupTable.get(Key.forCacheFile(cacheFile))).thenReturn(expectedUrl); + + String url = contentCache.getContentUrl(cacheFile); + + assertEquals("Content URLs should match", expectedUrl, url); + } + + + @Test + public void putForZeroLengthFile() + { + ContentReader contentReader = Mockito.mock(ContentReader.class); + Mockito.when(contentReader.getSize()).thenReturn(0L); + + boolean putResult = contentCache.put("", contentReader); + + assertFalse("Zero length files should not be cached", putResult); + } + + + @Test + public void putForNonEmptyFile() + { + ContentReader contentReader = Mockito.mock(ContentReader.class); + Mockito.when(contentReader.getSize()).thenReturn(999000L); + final String url = "store://some/url.bin"; + boolean putResult = contentCache.put(url, contentReader); + + assertTrue("Non-empty files should be cached", putResult); + ArgumentCaptor cacheFileArg = ArgumentCaptor.forClass(File.class); + Mockito.verify(contentReader).getContent(cacheFileArg.capture()); + // Check cached item is recorded properly in ehcache + Mockito.verify(lookupTable).put(Key.forUrl(url), cacheFileArg.getValue().getAbsolutePath()); + Mockito.verify(lookupTable).put(Key.forCacheFile(cacheFileArg.getValue().getAbsolutePath()), url); + } + + + @Test + public void remove() + { + final String url = "store://some/url.bin"; + final String path = "/some/path"; + Mockito.when(lookupTable.get(Key.forUrl(url))).thenReturn(path); + + contentCache.remove(url); + + Mockito.verify(lookupTable).remove(Key.forUrl(url)); + Mockito.verify(lookupTable).remove(Key.forCacheFile(path)); + } + + + @Test + public void getWriter() + { + final String url = "store://some/url.bin"; + + FileContentWriter writer = (FileContentWriter) contentCache.getWriter(url); + writer.putContent("Some test content for " + getClass().getName()); + + assertEquals(url, writer.getContentUrl()); + // Check cached item is recorded properly in ehcache + Mockito.verify(lookupTable).put(Key.forUrl(url), writer.getFile().getAbsolutePath()); + Mockito.verify(lookupTable).put(Key.forCacheFile(writer.getFile().getAbsolutePath()), url); + } +} diff --git a/source/java/org/alfresco/repo/content/caching/cleanup/CachedContentCleaner.java b/source/java/org/alfresco/repo/content/caching/cleanup/CachedContentCleaner.java index 5f84d5c471..78cd54a77f 100644 --- a/source/java/org/alfresco/repo/content/caching/cleanup/CachedContentCleaner.java +++ b/source/java/org/alfresco/repo/content/caching/cleanup/CachedContentCleaner.java @@ -23,6 +23,7 @@ import java.io.File; import org.alfresco.repo.content.caching.CacheFileProps; import org.alfresco.repo.content.caching.ContentCacheImpl; import org.alfresco.repo.content.caching.FileHandler; +import org.alfresco.util.Deleter; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import org.springframework.beans.factory.annotation.Required; @@ -130,8 +131,11 @@ public class CachedContentCleaner implements FileHandler CacheFileProps props = new CacheFileProps(cacheFile); props.delete(); cacheFile.delete(); + Deleter.deleteEmptyParents(cacheFile, cache.getCacheRoot()); } + + @Required public void setCache(ContentCacheImpl cache) { diff --git a/source/java/org/alfresco/repo/content/caching/cleanup/CachedContentCleanupJobTest.java b/source/java/org/alfresco/repo/content/caching/cleanup/CachedContentCleanupJobTest.java index 71fde36345..766346493b 100644 --- a/source/java/org/alfresco/repo/content/caching/cleanup/CachedContentCleanupJobTest.java +++ b/source/java/org/alfresco/repo/content/caching/cleanup/CachedContentCleanupJobTest.java @@ -24,6 +24,7 @@ import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; import java.io.File; +import java.io.FileNotFoundException; import java.io.PrintWriter; import org.alfresco.repo.content.caching.CacheFileProps; @@ -93,6 +94,21 @@ public class CachedContentCleanupJobTest } } + @Test + public void emptyParentDirectoriesAreDeleted() throws FileNotFoundException + { + cleaner.setMaxDeleteWatchCount(0); + File file = new File(cacheRoot, "243235984/a/b/c/d.bin"); + file.getParentFile().mkdirs(); + PrintWriter writer = new PrintWriter(file); + writer.println("Content for emptyParentDirectoriesAreDeleted"); + writer.close(); + assertTrue("Directory should exist", new File(cacheRoot, "243235984/a/b/c").exists()); + + cleaner.handle(file); + + assertFalse("Directory should have been deleted", new File(cacheRoot, "243235984").exists()); + } @Test public void markedFilesHaveDeletionDeferredUntilCorrectPassOfCleaner() diff --git a/source/java/org/alfresco/repo/content/caching/test/SlowContentStoreTest.java b/source/java/org/alfresco/repo/content/caching/test/SlowContentStoreTest.java index cba7c2ee62..a41700fca4 100644 --- a/source/java/org/alfresco/repo/content/caching/test/SlowContentStoreTest.java +++ b/source/java/org/alfresco/repo/content/caching/test/SlowContentStoreTest.java @@ -59,7 +59,7 @@ public class SlowContentStoreTest TimedStoreReader storeReader = new TimedStoreReader(); storeReader.execute(); assertTrue("First read should take a while", storeReader.timeTakenMillis() > 1000); - logger.info(String.format("First read took %ds", storeReader.timeTakenMillis())); + logger.debug(String.format("First read took %ds", storeReader.timeTakenMillis())); // The content came from the slow backing store... assertEquals("This is the content for my slow ReadableByteChannel", storeReader.content); @@ -69,7 +69,7 @@ public class SlowContentStoreTest storeReader = new TimedStoreReader(); storeReader.execute(); assertTrue("Subsequent reads should be fast", storeReader.timeTakenMillis() < 100); - logger.info(String.format("Cache read took %ds", storeReader.timeTakenMillis())); + logger.debug(String.format("Cache read took %ds", storeReader.timeTakenMillis())); // The content came from the slow backing store, but was cached... assertEquals("This is the content for my slow ReadableByteChannel", storeReader.content); } @@ -89,7 +89,7 @@ public class SlowContentStoreTest TimedStoreReader storeReader = new TimedStoreReader(); storeReader.execute(); assertTrue("First read should be fast", storeReader.timeTakenMillis() < 100); - logger.info(String.format("First read took %ds", storeReader.timeTakenMillis())); + logger.debug(String.format("First read took %ds", storeReader.timeTakenMillis())); assertEquals("Content written from " + getClass().getSimpleName(), storeReader.content); // Subsequent reads will also hit the cache @@ -98,7 +98,7 @@ public class SlowContentStoreTest storeReader = new TimedStoreReader(); storeReader.execute(); assertTrue("Subsequent reads should be fast", storeReader.timeTakenMillis() < 100); - logger.info(String.format("Cache read took %ds", storeReader.timeTakenMillis())); + logger.debug(String.format("Cache read took %ds", storeReader.timeTakenMillis())); // The original cached content, still cached... assertEquals("Content written from " + getClass().getSimpleName(), storeReader.content); } @@ -113,7 +113,7 @@ public class SlowContentStoreTest protected void doExecute() { content = cachingStore.getReader("any-url").getContentString(); - logger.info("Read content: " + content); + logger.debug("Read content: " + content); } } diff --git a/source/java/org/alfresco/repo/content/filestore/FileContentStore.java b/source/java/org/alfresco/repo/content/filestore/FileContentStore.java index 0e79ea19c4..f816d17219 100644 --- a/source/java/org/alfresco/repo/content/filestore/FileContentStore.java +++ b/source/java/org/alfresco/repo/content/filestore/FileContentStore.java @@ -34,6 +34,7 @@ import org.alfresco.repo.content.UnsupportedContentUrlException; import org.alfresco.service.cmr.repository.ContentIOException; import org.alfresco.service.cmr.repository.ContentReader; import org.alfresco.service.cmr.repository.ContentWriter; +import org.alfresco.util.Deleter; import org.alfresco.util.GUID; import org.alfresco.util.Pair; import org.apache.commons.logging.Log; @@ -613,7 +614,7 @@ public class FileContentStore // Delete empty parents regardless of whether the file was ignore above. if (deleteEmptyDirs && deleted) { - deleteEmptyParents(file); + Deleter.deleteEmptyParents(file, getRootLocation()); } // done @@ -626,38 +627,7 @@ public class FileContentStore return deleted; } - /** - * Deletes the parents of the specified file. The file itself must have been - * deleted before calling this method - since only empty directories can be deleted. - * - * @param file - */ - private void deleteEmptyParents(File file) - { - String root = getRootLocation(); - File parent = file.getParentFile(); - boolean deleted = false; - do - { - try - { - if (parent.isDirectory() && !parent.getCanonicalPath().equals(root)) - { - // Only an empty directory will successfully be deleted. - deleted = parent.delete(); - } - } - catch (IOException error) - { - logger.error("Unable to construct canonical path for " + parent.getAbsolutePath()); - break; - } - - parent = parent.getParentFile(); - } - while(deleted); - - } + /** * Creates a new content URL. This must be supported by all