From 1a2518d19bb447f4d3821cb71e32e712cc99d9c0 Mon Sep 17 00:00:00 2001 From: Justin Clark-Casey (justincc) Date: Fri, 5 Aug 2011 19:57:47 +0100 Subject: Instead of moving the file to its final place when FlotsamCache writes to disk, copy it instead. This is to eliminate IOException where two threads compete to cache the same file. --- OpenSim/Region/CoreModules/Asset/FlotsamAssetCache.cs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) (limited to 'OpenSim/Region/CoreModules/Asset/FlotsamAssetCache.cs') diff --git a/OpenSim/Region/CoreModules/Asset/FlotsamAssetCache.cs b/OpenSim/Region/CoreModules/Asset/FlotsamAssetCache.cs index da39202..0c700e3 100644 --- a/OpenSim/Region/CoreModules/Asset/FlotsamAssetCache.cs +++ b/OpenSim/Region/CoreModules/Asset/FlotsamAssetCache.cs @@ -600,7 +600,13 @@ namespace Flotsam.RegionModules.AssetCache stream.Close(); // Now that it's written, rename it so that it can be found. - File.Move(tempname, filename); + // We're doing this as a file copy operation so that if two threads are competing to cache this asset, + // then both suceed instead of one failing when it tries to move the file to a final filename that + // already exists. + // This assumes that the file copy operation is atomic. Assuming this holds, then copying also works + // if another simulator is using the same cache directory. + File.Copy(tempname, filename, true); + File.Delete(tempname); if (m_LogLevel >= 2) m_log.DebugFormat("[FLOTSAM ASSET CACHE]: Cache Stored :: {0}", asset.ID); @@ -635,7 +641,6 @@ namespace Flotsam.RegionModules.AssetCache } #endif } - } } -- cgit v1.1 From 7d35bf819374a323fa737f8342a4239e0759ce8f Mon Sep 17 00:00:00 2001 From: Justin Clark-Casey (justincc) Date: Fri, 5 Aug 2011 22:45:42 +0100 Subject: refactor: remove a sliver of unnecessary code --- OpenSim/Region/CoreModules/Asset/FlotsamAssetCache.cs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) (limited to 'OpenSim/Region/CoreModules/Asset/FlotsamAssetCache.cs') diff --git a/OpenSim/Region/CoreModules/Asset/FlotsamAssetCache.cs b/OpenSim/Region/CoreModules/Asset/FlotsamAssetCache.cs index 0c700e3..7ef759c 100644 --- a/OpenSim/Region/CoreModules/Asset/FlotsamAssetCache.cs +++ b/OpenSim/Region/CoreModules/Asset/FlotsamAssetCache.cs @@ -635,10 +635,7 @@ namespace Flotsam.RegionModules.AssetCache waitEvent.Set(); } #else - if (m_CurrentlyWriting.Contains(filename)) - { - m_CurrentlyWriting.Remove(filename); - } + m_CurrentlyWriting.Remove(filename); #endif } } -- cgit v1.1 From dcb4b2de09032380fb428f73d9e3fd10aa662377 Mon Sep 17 00:00:00 2001 From: Justin Clark-Casey (justincc) Date: Sat, 13 Aug 2011 15:16:43 +0100 Subject: Fix a problem in the Flotsam asset cache where assets were being put into the memory cache even when it wasn't enabled. This hopefully addresses http://opensimulator.org/mantis/view.php?id=5634 This is the most probable cause of the memory problems that people have been seeing in the past month. This bug has been around since commit 5dc785b (4th July 2011). Doh! This is why regressions tests are such a good idea... :) Many thanks to Nebadon for using git bisect to track down this bug, which made it a 5 minute fix. --- OpenSim/Region/CoreModules/Asset/FlotsamAssetCache.cs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) (limited to 'OpenSim/Region/CoreModules/Asset/FlotsamAssetCache.cs') diff --git a/OpenSim/Region/CoreModules/Asset/FlotsamAssetCache.cs b/OpenSim/Region/CoreModules/Asset/FlotsamAssetCache.cs index 7ef759c..abfc771 100644 --- a/OpenSim/Region/CoreModules/Asset/FlotsamAssetCache.cs +++ b/OpenSim/Region/CoreModules/Asset/FlotsamAssetCache.cs @@ -357,8 +357,6 @@ namespace Flotsam.RegionModules.AssetCache asset = (AssetBase)bformatter.Deserialize(stream); - UpdateMemoryCache(id, asset); - m_DiskHits++; } catch (System.Runtime.Serialization.SerializationException e) @@ -419,9 +417,15 @@ namespace Flotsam.RegionModules.AssetCache if (m_MemoryCacheEnabled) asset = GetFromMemoryCache(id); + if (asset == null && m_FileCacheEnabled) + { asset = GetFromFileCache(id); + if (m_MemoryCacheEnabled && asset != null) + UpdateMemoryCache(id, asset); + } + if (((m_LogLevel >= 1)) && (m_HitRateDisplay != 0) && (m_Requests % m_HitRateDisplay == 0)) { m_HitRateFile = (double)m_DiskHits / m_Requests * 100.0; -- cgit v1.1 From 8c95c83562db7e273cbf555514224773f21a2b19 Mon Sep 17 00:00:00 2001 From: Justin Clark-Casey (justincc) Date: Tue, 16 Aug 2011 21:03:43 +0100 Subject: On Flotsam asset cache, go back to moving the file from the temporary location rather than copying. Copying doesn't prevent IOExceptions on Windows due to file locking. (e.g. Mantis 5642, 5630). So instead go back to moving the file, swallowing IOExceptions that occur just for the move due to competing caching threads or even different opensimulator instances. --- .../Region/CoreModules/Asset/FlotsamAssetCache.cs | 73 ++++++++++++++-------- 1 file changed, 47 insertions(+), 26 deletions(-) (limited to 'OpenSim/Region/CoreModules/Asset/FlotsamAssetCache.cs') diff --git a/OpenSim/Region/CoreModules/Asset/FlotsamAssetCache.cs b/OpenSim/Region/CoreModules/Asset/FlotsamAssetCache.cs index abfc771..b2f6e13 100644 --- a/OpenSim/Region/CoreModules/Asset/FlotsamAssetCache.cs +++ b/OpenSim/Region/CoreModules/Asset/FlotsamAssetCache.cs @@ -593,39 +593,60 @@ namespace Flotsam.RegionModules.AssetCache try { - if (!Directory.Exists(directory)) + try { - Directory.CreateDirectory(directory); + if (!Directory.Exists(directory)) + { + Directory.CreateDirectory(directory); + } + + stream = File.Open(tempname, FileMode.Create); + BinaryFormatter bformatter = new BinaryFormatter(); + bformatter.Serialize(stream, asset); } + catch (IOException e) + { + m_log.ErrorFormat( + "[FLOTSAM ASSET CACHE]: Failed to write asset {0} to temporary location {1} (final {2}) on cache in {3}. Exception {4} {5}.", + asset.ID, tempname, filename, directory, e.Message, e.StackTrace); - stream = File.Open(tempname, FileMode.Create); - BinaryFormatter bformatter = new BinaryFormatter(); - bformatter.Serialize(stream, asset); - stream.Close(); - - // Now that it's written, rename it so that it can be found. - // We're doing this as a file copy operation so that if two threads are competing to cache this asset, - // then both suceed instead of one failing when it tries to move the file to a final filename that - // already exists. - // This assumes that the file copy operation is atomic. Assuming this holds, then copying also works - // if another simulator is using the same cache directory. - File.Copy(tempname, filename, true); - File.Delete(tempname); + return; + } + finally + { + if (stream != null) + stream.Close(); + } - if (m_LogLevel >= 2) - m_log.DebugFormat("[FLOTSAM ASSET CACHE]: Cache Stored :: {0}", asset.ID); - } - catch (Exception e) - { - m_log.ErrorFormat( - "[FLOTSAM ASSET CACHE]: Failed to write asset {0} to cache. Directory {1}, tempname {2}, filename {3}. Exception {4} {5}.", - asset.ID, directory, tempname, filename, e.Message, e.StackTrace); + try + { + // Now that it's written, rename it so that it can be found. + // + // File.Copy(tempname, filename, true); + // File.Delete(tempname); + // + // For a brief period, this was done as a separate copy and then temporary file delete operation. + // However, this causes exceptions on Windows when other threads attempt to read a file + // which is still being copied. So instead, go back to moving the file and swallowing any IOException + // that occurs because two threads race to cache the same data (and the second fails because the file + // already exists). + // + // This situation occurs fairly rarely anyway. We assume in this that moves are atomic on the + // filesystem. + File.Move(tempname, filename); + + if (m_LogLevel >= 2) + m_log.DebugFormat("[FLOTSAM ASSET CACHE]: Cache Stored :: {0}", asset.ID); + } + catch (IOException) + { + // If we see an IOException here it's likely that some other competing thread has written the + // cache file first, so ignore. Other IOException errors (e.g. filesystem full) should be + // signally by the earlier temporary file writing code. + } } finally { - if (stream != null) - stream.Close(); - // Even if the write fails with an exception, we need to make sure // that we release the lock on that file, otherwise it'll never get // cached -- cgit v1.1 From fd3a7ab70c04742a8ea776180144ea016e0d4e31 Mon Sep 17 00:00:00 2001 From: Justin Clark-Casey (justincc) Date: Tue, 16 Aug 2011 21:31:08 +0100 Subject: minor: change some comment text in flotsam asset cache --- OpenSim/Region/CoreModules/Asset/FlotsamAssetCache.cs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) (limited to 'OpenSim/Region/CoreModules/Asset/FlotsamAssetCache.cs') diff --git a/OpenSim/Region/CoreModules/Asset/FlotsamAssetCache.cs b/OpenSim/Region/CoreModules/Asset/FlotsamAssetCache.cs index b2f6e13..22c301b 100644 --- a/OpenSim/Region/CoreModules/Asset/FlotsamAssetCache.cs +++ b/OpenSim/Region/CoreModules/Asset/FlotsamAssetCache.cs @@ -625,11 +625,10 @@ namespace Flotsam.RegionModules.AssetCache // File.Copy(tempname, filename, true); // File.Delete(tempname); // - // For a brief period, this was done as a separate copy and then temporary file delete operation. + // For a brief period, this was done as a separate copy and then temporary file delete operation to + // avoid an IOException caused by move if some competing thread had already written the file. // However, this causes exceptions on Windows when other threads attempt to read a file - // which is still being copied. So instead, go back to moving the file and swallowing any IOException - // that occurs because two threads race to cache the same data (and the second fails because the file - // already exists). + // which is still being copied. So instead, go back to moving the file and swallow any IOException. // // This situation occurs fairly rarely anyway. We assume in this that moves are atomic on the // filesystem. -- cgit v1.1