From 466c443a208fcae26fae816e937833b037b77af9 Mon Sep 17 00:00:00 2001 From: Flamefire Date: Wed, 24 Sep 2014 15:33:24 +0200 Subject: [PATCH] Fix race condition when getting the loader task --- Vocaluxe/Lib/Draw/CDrawBase.cs | 167 ++++++++++++++++++--------------- 1 file changed, 92 insertions(+), 75 deletions(-) diff --git a/Vocaluxe/Lib/Draw/CDrawBase.cs b/Vocaluxe/Lib/Draw/CDrawBase.cs index 06718c84..a77ce9a7 100644 --- a/Vocaluxe/Lib/Draw/CDrawBase.cs +++ b/Vocaluxe/Lib/Draw/CDrawBase.cs @@ -346,7 +346,9 @@ namespace Vocaluxe.Lib.Draw } /// - /// Adds the texture to the cache + /// Adds the texture to the cache
+ /// Asserts that the cache entry does not exist yet. So make Get/Add cache atomic through use of _TextureCache lock!
+ /// Thread safe /// /// /// @@ -367,25 +369,37 @@ namespace Vocaluxe.Lib.Draw /// /// Gets a new texture reference from the cache
- /// Thread safe but you should hold the _TextureCache lock if you need the loader task + /// Thread safe /// /// + /// Task that loads the texture or null if not loading /// - private CTextureRef _GetFromCache(string texturePath) + private CTextureRef _GetFromCache(string texturePath, out Task loader) { + loader = null; if (String.IsNullOrEmpty(texturePath)) return null; - lock (_Textures) - { - STextureCacheEntry cacheEntry; - if (_TextureCache.TryGetValue(texturePath, out cacheEntry)) + lock (_TextureCache) + lock (_Textures) { - if (cacheEntry.Texture.RefCount <= 0) - _TextureCache.Remove(texturePath); - else - return _GetTextureReference(cacheEntry.OrigSize, cacheEntry.Texture); + STextureCacheEntry cacheEntry; + if (_TextureCache.TryGetValue(texturePath, out cacheEntry)) + { + if (cacheEntry.Texture.RefCount <= 0) + _TextureCache.Remove(texturePath); + else + { + if (cacheEntry.OrigSize.Width < 0) + { + lock (_BitmapsLoading) + { + loader = _BitmapsLoading[texturePath]; + } + } + return _GetTextureReference(cacheEntry.OrigSize, cacheEntry.Texture); + } + } } - } return null; } @@ -522,47 +536,31 @@ namespace Vocaluxe.Lib.Draw /// /// Replaces one oldTexture by newTexture summing the refCounts. Also disposes oldTexture - /// IMPORTANT: Caller MUST hold the _Textures lock! /// /// /// private void _MergeTextures(TTextureType oldTexture, TTextureType newTexture) { + _EnsureMainThread(); Debug.Assert(oldTexture.RefCount > 0); - newTexture.RefCount += oldTexture.RefCount; - // ReSharper disable AccessToDisposedClosure - IEnumerable oldKeys = _Textures.Where(pair => pair.Value == oldTexture).Select(pair => pair.Key).ToArray(); - // ReSharper restore AccessToDisposedClosure - foreach (int key in oldKeys) - _Textures[key] = newTexture; - oldTexture.Dispose(); - } - - /// - /// Checks if a texture is currently beeing loaded and waits for completion if it is
- /// Required for cached entries, but should not be called from inside any locks unless async is true - /// - /// - /// - /// If true, do not block - private void _WaitForTextureLoaded(CTextureRef textureRef, string filePath, bool async) - { - // Check if texture is already loaded - if (textureRef.OrigSize.Width > 0) - return; - Task loader; - lock (_BitmapsLoading) + lock (_Textures) { - if (!_BitmapsLoading.TryGetValue(filePath, out loader)) - Debug.Assert(false, "Corruption: Texture not loaded but no loader found"); + newTexture.RefCount += oldTexture.RefCount; + IEnumerable oldKeys = _Textures.Where(pair => pair.Value == oldTexture).Select(pair => pair.Key).ToArray(); + foreach (int key in oldKeys) + _Textures[key] = newTexture; + if (!String.IsNullOrEmpty(oldTexture.TexturePath)) + { + newTexture.TexturePath = oldTexture.TexturePath; + STextureCacheEntry cacheEntry; + if (_TextureCache.TryGetValue(newTexture.TexturePath, out cacheEntry)) + { + cacheEntry.Texture = newTexture; + _TextureCache[newTexture.TexturePath] = cacheEntry; + } + } + oldTexture.Dispose(); } - // We found the texture in the cache, but its bitmap is not yet loaded - // So wait for completion and update the origSize once it is available - // Note: loader.Result blocks till completion of the task - if (async) - Task.Factory.StartNew(() => textureRef.OrigSize = loader.Result); - else - textureRef.OrigSize = loader.Result; } #endregion private/protected @@ -575,13 +573,15 @@ namespace Vocaluxe.Lib.Draw { _EnsureMainThread(); CTextureRef textureRef; + Task loader; lock (_TextureCache) { - textureRef = _GetFromCache(texturePath); + textureRef = _GetFromCache(texturePath, out loader); } if (textureRef != null) { - _WaitForTextureLoaded(textureRef, texturePath, false); + if (loader != null) + textureRef.OrigSize = loader.Result; Debug.Assert(textureRef.OrigSize.Width > 0); return textureRef; } @@ -621,10 +621,11 @@ namespace Vocaluxe.Lib.Draw CTextureRef textureRef; Size origSize = bmp.GetSize(); TTextureType texture = null; + Task loader; // Make the Get/Add Cache methods atomic lock (_TextureCache) { - textureRef = _GetFromCache(texturePath); + textureRef = _GetFromCache(texturePath, out loader); if (textureRef == null) { _WriteBitmapToTexture(ref texture, bmp); @@ -634,8 +635,8 @@ namespace Vocaluxe.Lib.Draw } if (textureRef == null) textureRef = _GetTextureReference(origSize, texture); - else - _WaitForTextureLoaded(textureRef, texturePath, false); + else if (loader != null) + textureRef.OrigSize = loader.Result; Debug.Assert(textureRef.OrigSize.Width > 0); return textureRef; } @@ -693,13 +694,14 @@ namespace Vocaluxe.Lib.Draw CTextureRef textureRef; TTextureType texture = null; - Task loader = null; + Task loader; // Make Get/Add cache atomic lock (_TextureCache) { - textureRef = _GetFromCache(filePath); + textureRef = _GetFromCache(filePath, out loader); if (textureRef == null) { + Debug.Assert(loader == null); Size invalidSize = new Size(-1, -1); texture = _CreateTexture(invalidSize); // This cache entry has to be updated once the real origSize is available @@ -718,7 +720,9 @@ namespace Vocaluxe.Lib.Draw if (texture == null) { // Found in cache - _WaitForTextureLoaded(textureRef, filePath, true); + CTextureRef tmp = textureRef; // Workaround for implicitly captured closure warning (false positive) + if (loader != null) + Task.Factory.StartNew(() => tmp.OrigSize = loader.Result); return textureRef; } @@ -736,42 +740,46 @@ namespace Vocaluxe.Lib.Draw /// private Size _LoadAndEnqueueBitmap(string filePath, CTextureRef textureRef) { - Size origSize; Bitmap bmp = CHelper.LoadBitmap(filePath); if (bmp == null) { RemoveTexture(ref textureRef); // Done asynchonously in the function - origSize = new Size(-1, -1); - } - else - { - origSize = bmp.GetSize(); - textureRef.OrigSize = origSize; - // Update cache, use the same lock as in add/get cache methods lock (_Textures) { - STextureCacheEntry cacheEntry; - if (_TextureCache.TryGetValue(filePath, out cacheEntry)) + _TextureCache.Remove(filePath); + lock (_BitmapsLoading) { - cacheEntry.OrigSize = origSize; - _TextureCache[filePath] = cacheEntry; + _BitmapsLoading.Remove(filePath); } } - Size size = _GetNewTextureSize(bmp.GetSize()); - if (!size.Equals(bmp.GetSize())) + return new Size(-1, -1); + } + Size origSize = bmp.GetSize(); + textureRef.OrigSize = origSize; + // Update cache, use the same lock as in add/get cache methods + lock (_Textures) + { + STextureCacheEntry cacheEntry; + if (_TextureCache.TryGetValue(filePath, out cacheEntry)) { - Bitmap bmp2 = bmp.Resize(size); - bmp.Dispose(); - bmp = bmp2; + cacheEntry.OrigSize = origSize; + _TextureCache[filePath] = cacheEntry; } - lock (_TextureQueue) + lock (_BitmapsLoading) { - _TextureQueue.Enqueue(new STextureQueue(textureRef, EQueueAction.Add, bmp)); + _BitmapsLoading.Remove(filePath); } } - lock (_BitmapsLoading) + Size size = _GetNewTextureSize(origSize); + if (!size.Equals(origSize)) { - _BitmapsLoading.Remove(filePath); + Bitmap bmp2 = bmp.Resize(size); + bmp.Dispose(); + bmp = bmp2; + } + lock (_TextureQueue) + { + _TextureQueue.Enqueue(new STextureQueue(textureRef, EQueueAction.Add, bmp)); } return origSize; } @@ -840,7 +848,16 @@ namespace Vocaluxe.Lib.Draw return null; // If bitmap is not yet loaded, wait for it if (!String.IsNullOrEmpty(texture.TexturePath)) - _WaitForTextureLoaded(textureRef, texture.TexturePath, false); + { + Task loader; + lock (_BitmapsLoading) + { + _BitmapsLoading.TryGetValue(texture.TexturePath, out loader); + } + if (loader != null) + textureRef.OrigSize = loader.Result; + } + Debug.Assert(textureRef.OrigSize.Width > 0); return _GetTextureReference(textureRef.OrigSize, texture); }