diff --git a/src/BloomExe/Book/BookInfo.cs b/src/BloomExe/Book/BookInfo.cs index a629d69e3051..8bd252a7931d 100644 --- a/src/BloomExe/Book/BookInfo.cs +++ b/src/BloomExe/Book/BookInfo.cs @@ -355,6 +355,7 @@ public bool TryGetPremadeThumbnail(out Image image) { try { + // Caller owns this image and must dispose it promptly to release any file lock. image = RobustImageIO.GetImageFromFile(path); return true; } diff --git a/src/BloomExe/Book/BookStarter.cs b/src/BloomExe/Book/BookStarter.cs index c01f2118489f..dc0b0f83044f 100644 --- a/src/BloomExe/Book/BookStarter.cs +++ b/src/BloomExe/Book/BookStarter.cs @@ -71,7 +71,7 @@ string parentCollectionPath var newBookFolder = Path.Combine(parentCollectionPath, initialBookName); CopyFolder(sourceBookFolder, newBookFolder); BookStorage.RemoveLocalOnlyFiles(newBookFolder); - //if something bad happens from here on out, we need to delete that folder we just made + //if something bad happens from here on out, we need to delete that folder we just made try { var oldNamedFile = Path.Combine( @@ -93,7 +93,11 @@ string parentCollectionPath RobustFile.Move(oldNamedFile, newNamedFile); //the destination may change here... - newBookFolder = SetupNewDocumentContents(sourceBookFolder, newBookFolder, newBookInstanceId); + newBookFolder = SetupNewDocumentContents( + sourceBookFolder, + newBookFolder, + newBookInstanceId + ); if (OnNextRunSimulateFailureMakingBook) throw new ApplicationException("Simulated failure for unit test"); @@ -166,7 +170,11 @@ private string GetMetaValue(SafeXmlDocument Dom, string name, string defaultValu return defaultValue; } - private string SetupNewDocumentContents(string sourceFolderPath, string initialPath, string newBookInstanceId) + private string SetupNewDocumentContents( + string sourceFolderPath, + string initialPath, + string newBookInstanceId + ) { // This bookInfo is temporary, just used to make the (also temporary) BookStorage we // use here in this method. I don't think it actually matters what its save context is. @@ -416,7 +424,11 @@ private static void ProcessXMatterMetaTags(BookStorage storage) storage.Dom.RemoveMetaElement("xmatter-for-children"); } - private void SetLineageAndId(BookStorage storage, string sourceFolderPath, string newBookInstanceId) + private void SetLineageAndId( + BookStorage storage, + string sourceFolderPath, + string newBookInstanceId + ) { string parentId = null; string lineage = null; diff --git a/src/BloomExe/Book/BookStorage.cs b/src/BloomExe/Book/BookStorage.cs index 3bedaad7c6e6..43ebb4661bba 100644 --- a/src/BloomExe/Book/BookStorage.cs +++ b/src/BloomExe/Book/BookStorage.cs @@ -45,6 +45,8 @@ public interface IBookStorage string FolderName { get; } string FolderPath { get; } string PathToExistingHtml { get; } + + // Caller owns the returned image and must dispose it promptly. bool TryGetPremadeThumbnail(string fileName, out Image image); //bool DeleteBook(); @@ -435,6 +437,7 @@ public bool TryGetPremadeThumbnail(string fileName, out Image image) string path = Path.Combine(FolderPath, fileName); if (RobustFile.Exists(path)) { + // Caller owns this image and must dispose it promptly to release any file lock. image = RobustImageIO.GetImageFromFile(path); return true; } diff --git a/src/BloomExe/BookThumbNailer.cs b/src/BloomExe/BookThumbNailer.cs index c5c4c8a14e27..b317bb25ebd3 100644 --- a/src/BloomExe/BookThumbNailer.cs +++ b/src/BloomExe/BookThumbNailer.cs @@ -498,7 +498,7 @@ HtmlThumbNailer.ThumbnailOptions thumbnailOptions RebuildThumbNail( book, thumbnailOptions, - (info, image) => { }, + (info, image) => image?.Dispose(), (info, ex) => { throw ex; diff --git a/src/BloomExe/CollectionTab/CollectionModel.cs b/src/BloomExe/CollectionTab/CollectionModel.cs index a925357ac2ae..f361d1819b9d 100644 --- a/src/BloomExe/CollectionTab/CollectionModel.cs +++ b/src/BloomExe/CollectionTab/CollectionModel.cs @@ -721,7 +721,11 @@ public void UpdateThumbnailAsync(Book.Book book) UpdateThumbnailAsync( book, BookThumbNailer.GetCoverThumbnailOptions(-1, Guid.Empty), - RefreshOneThumbnail, + (bookInfo, image) => + { + RefreshOneThumbnail(bookInfo, image); + image?.Dispose(); + }, HandleThumbnailerErrror ); } diff --git a/src/BloomExe/ImageProcessing/RuntimeImageProcessor.cs b/src/BloomExe/ImageProcessing/RuntimeImageProcessor.cs index 1cc27cd5bddb..658ad9e1e1cd 100644 --- a/src/BloomExe/ImageProcessing/RuntimeImageProcessor.cs +++ b/src/BloomExe/ImageProcessing/RuntimeImageProcessor.cs @@ -225,36 +225,37 @@ Color backColor (newW * originalImage.Image.Height + (originalImage.Image.Width / 2)) / originalImage.Image.Width; - var thumbnail = new Bitmap(newW, newH); - - var g = Graphics.FromImage(thumbnail); - if (backColor != Color.Empty) + using (var thumbnail = new Bitmap(newW, newH)) + using (var g = Graphics.FromImage(thumbnail)) { - using (var brush = new SolidBrush(backColor)) + if (backColor != Color.Empty) { - g.FillRectangle(brush, new Rectangle(0, 0, newW, newH)); + using (var brush = new SolidBrush(backColor)) + { + g.FillRectangle(brush, new Rectangle(0, 0, newW, newH)); + } } + Image imageToDraw = originalImage.Image; + bool makeTransparentImage = ImageUtils.ShouldMakeBackgroundTransparent( + originalImage + ); + if (makeTransparentImage) + { + imageToDraw = MakePngBackgroundTransparent(originalImage); + } + var destRect = new Rectangle(0, 0, newW, newH); + // Note the image size may change when the background is made transparent. + // See https://silbloom.myjetbrains.com/youtrack/issue/BL-5632. + g.DrawImage( + imageToDraw, + destRect, + new Rectangle(0, 0, imageToDraw.Width, imageToDraw.Height), + GraphicsUnit.Pixel + ); + if (makeTransparentImage) + imageToDraw.Dispose(); + RobustImageIO.SaveImage(thumbnail, pathToProcessedImage); } - Image imageToDraw = originalImage.Image; - bool makeTransparentImage = ImageUtils.ShouldMakeBackgroundTransparent( - originalImage - ); - if (makeTransparentImage) - { - imageToDraw = MakePngBackgroundTransparent(originalImage); - } - var destRect = new Rectangle(0, 0, newW, newH); - // Note the image size may change when the background is made transparent. - // See https://silbloom.myjetbrains.com/youtrack/issue/BL-5632. - g.DrawImage( - imageToDraw, - destRect, - new Rectangle(0, 0, imageToDraw.Width, imageToDraw.Height), - GraphicsUnit.Pixel - ); - if (makeTransparentImage) - imageToDraw.Dispose(); - RobustImageIO.SaveImage(thumbnail, pathToProcessedImage); } return true; diff --git a/src/BloomExe/web/RequestInfo.cs b/src/BloomExe/web/RequestInfo.cs index 12887525b3c0..d576837c12a2 100644 --- a/src/BloomExe/web/RequestInfo.cs +++ b/src/BloomExe/web/RequestInfo.cs @@ -13,6 +13,7 @@ using Bloom.Book; using Bloom.web; using Bloom.web.controllers; +using SIL.Code; using SIL.IO; using SIL.Reporting; @@ -136,6 +137,23 @@ private static void ReportHttpListenerProblem(HttpListenerException e) public bool HaveFullyProcessedRequest { get; private set; } + // We intentionally do not use RobustFile.OpenRead() in this endpoint. + // We need a read stream that permits concurrent overwrite/delete of the same file + // while it is being served (notably for thumbnails and media), so we require + // FileShare.ReadWrite | FileShare.Delete. We still want robust retry behavior, + // so we wrap this open in RetryUtility.Retry(). + private static FileStream OpenSharedReadStreamWithRetry(string path) + { + return RetryUtility.Retry(() => + new FileStream( + path, + FileMode.Open, + FileAccess.Read, + FileShare.ReadWrite | FileShare.Delete + ) + ); + } + public void ReplyWithFileContent(string path, string originalPath = null) { //Deal with BL-3153, where the file was still open in another thread @@ -152,7 +170,7 @@ public void ReplyWithFileContent(string path, string originalPath = null) try { - fs = RobustFile.OpenRead(path); + fs = OpenSharedReadStreamWithRetry(path); } catch (Exception error) { @@ -255,7 +273,7 @@ public void ReplyWithFileContent(string path, string originalPath = null) _actualContext.Response.OutputStream.Write(buffer, 0, read); try { - fs = RobustFile.OpenRead(path); + fs = OpenSharedReadStreamWithRetry(path); } catch (FileNotFoundException) {