From b588e200a454ba47e7a2a173e81708d4525994fb Mon Sep 17 00:00:00 2001 From: John Thomson Date: Fri, 3 Apr 2026 15:03:25 -0500 Subject: [PATCH 1/2] Fix various things that might keep thumbnail files locked (BL-16122) --- src/BloomExe/Book/BookInfo.cs | 1 + src/BloomExe/Book/BookStarter.cs | 20 +++++-- src/BloomExe/Book/BookStorage.cs | 3 ++ src/BloomExe/BookThumbNailer.cs | 2 +- src/BloomExe/CollectionTab/CollectionModel.cs | 6 ++- .../ImageProcessing/RuntimeImageProcessor.cs | 53 ++++++++++--------- src/BloomExe/web/RequestInfo.cs | 22 +++++++- 7 files changed, 73 insertions(+), 34 deletions(-) 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) { From e413a3778fab8cc8a79f33430560fb863c841ca1 Mon Sep 17 00:00:00 2001 From: John Thomson Date: Mon, 6 Apr 2026 09:24:17 -0500 Subject: [PATCH 2/2] Post-review improvements --- src/BloomExe/BookThumbNailer.cs | 21 ++++++++++++------- src/BloomExe/CollectionTab/CollectionModel.cs | 10 +++++++-- 2 files changed, 22 insertions(+), 9 deletions(-) diff --git a/src/BloomExe/BookThumbNailer.cs b/src/BloomExe/BookThumbNailer.cs index b317bb25ebd3..374c752fb2bc 100644 --- a/src/BloomExe/BookThumbNailer.cs +++ b/src/BloomExe/BookThumbNailer.cs @@ -46,6 +46,11 @@ public HtmlThumbNailer HtmlThumbNailer get { return _thumbnailProvider; } } + private static Image CreateDisposableErrorThumbnail() + { + return Resources.Error70x70.Clone() as Image; + } + private void GetThumbNailOfBookCover( Book.Book book, HtmlThumbNailer.ThumbnailOptions thumbnailOptions, @@ -56,14 +61,14 @@ bool async { if (book is ErrorBook) { - callback(Resources.Error70x70); + callback(CreateDisposableErrorThumbnail()); return; } try { if (book.HasFatalError) //NB: we might not know yet... we don't fully load every book just to show its thumbnail { - callback(Resources.Error70x70); + callback(CreateDisposableErrorThumbnail()); return; } GenerateImageForWeb(book); @@ -79,7 +84,7 @@ bool async var dom = book.GetPreviewXmlDocumentForFirstPage(); if (dom == null) { - callback(Resources.Error70x70); + callback(CreateDisposableErrorThumbnail()); return; } string folderForCachingThumbnail; @@ -97,13 +102,13 @@ bool async #else if (!CreateThumbnailOfCoverImage(book, thumbnailOptions, callback)) { - callback(Resources.Error70x70); + callback(CreateDisposableErrorThumbnail()); } #endif } catch (Exception err) { - callback(Resources.Error70x70); + callback(CreateDisposableErrorThumbnail()); errorCallback(err); Debug.Fail(err.Message); } @@ -469,7 +474,8 @@ private void SetSizeAndOrientationClass(SafeXmlElement pageDiv, string sizeOrien } /// - /// Will call either 'callback' or 'errorCallback' UNLESS the thumbnail is readonly, in which case it will do neither. + /// Will call either 'callback' or 'errorCallback'. + /// The callback receives an image that the callback must dispose when done. /// /// /// @@ -517,7 +523,8 @@ Book.Book book } /// - /// Will call either 'callback' or 'errorCallback' UNLESS the thumbnail is readonly, in which case it will do neither. + /// Will call either 'callback' or 'errorCallback'. + /// The callback receives an image that the callback must dispose when done. /// /// /// diff --git a/src/BloomExe/CollectionTab/CollectionModel.cs b/src/BloomExe/CollectionTab/CollectionModel.cs index f361d1819b9d..1b5f6212080a 100644 --- a/src/BloomExe/CollectionTab/CollectionModel.cs +++ b/src/BloomExe/CollectionTab/CollectionModel.cs @@ -723,8 +723,14 @@ public void UpdateThumbnailAsync(Book.Book book) BookThumbNailer.GetCoverThumbnailOptions(-1, Guid.Empty), (bookInfo, image) => { - RefreshOneThumbnail(bookInfo, image); - image?.Dispose(); + try + { + RefreshOneThumbnail(bookInfo, image); + } + finally + { + image?.Dispose(); + } }, HandleThumbnailerErrror );