Skip to content

Fix various things that might keep thumbnail files locked (BL-16122) #7808

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
JohnThomson wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix various things that might keep thumbnail files locked (BL-16122) #7808

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/BloomExe/Book/BookInfo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
20 changes: 16 additions & 4 deletions src/BloomExe/Book/BookStarter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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");
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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;
Expand Down
3 changes: 3 additions & 0 deletions src/BloomExe/Book/BookStorage.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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;
}
Expand Down
2 changes: 1 addition & 1 deletion src/BloomExe/BookThumbNailer.cs
View file
Open in desktop
Copy link
Copy Markdown

@devin-ai-integration devin-ai-integration bot Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚩 RebuildThumbNail has an additional undisposed image path for readonly thumbnails

In RebuildThumbNail at BookThumbNailer.cs:536-543, when the thumbnail is readonly, TryGetPremadeThumbnail returns an image loaded from file (caller-owned per the new comments). This image is passed to callback(book.BookInfo, thumb). For RebuildThumbNailNow (line 501), the callback disposes it correctly. For CollectionModel.UpdateThumbnailAsync (line 724-728), it also disposes correctly. However, for the 4-parameter UpdateThumbnailAsync overload at line 706, callers provide their own callbacks and may not dispose the image — this is a pre-existing concern not introduced by this PR, but worth noting since the PR adds ownership documentation.

(Refers to lines 536-543)

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Original file line number Diff line number Diff line change
Expand Up @@ -498,7 +498,7 @@ HtmlThumbNailer.ThumbnailOptions thumbnailOptions
RebuildThumbNail(
book,
thumbnailOptions,
(info, image) => { },
(info, image) => image?.Dispose(),
Copy link
Copy Markdown

@devin-ai-integration devin-ai-integration bot Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Disposing shared Resources.Error70x70 singleton in RebuildThumbNailNow callback

The new callback (info, image) => image?.Dispose() will dispose Resources.Error70x70 when error paths are hit. In GetThumbNailOfBookCover (BookThumbNailer.cs:59,66,100,106), the callback receives Resources.Error70x70 directly (not a clone). This flows through RebuildThumbNail at BookThumbNailer.cs:551 (image => callback(book.BookInfo, image)) and reaches the disposing callback. Since ResourceManager.GetObject caches and returns the same Bitmap instance on each access, disposing it corrupts the shared resource. All subsequent uses of Resources.Error70x70 elsewhere (e.g., CollectionModel.cs:746 calling .Save(), CollectionApi.cs:775 calling ConvertImageToStream()) will fail with ObjectDisposedException or GDI+ errors.

Prompt for agents 
The callback in RebuildThumbNailNow disposes the image it receives, but GetThumbNailOfBookCover (lines 59, 66, 100, 106) passes the shared Resources.Error70x70 singleton directly to the callback in several error paths. Disposing this shared resource corrupts it for all future uses across the application.

Possible fixes:
1. In RebuildThumbNailNow, only dispose images that are not Resources.Error70x70 (fragile, not recommended).
2. In GetThumbNailOfBookCover, clone Resources.Error70x70 before passing it to callbacks, so callers can safely dispose the clone. This would be consistent with CreateThumbnailOfCoverImage (line 319) which already clones its image before calling the callback.
3. Have the callback check whether the image is a shared resource before disposing it.

Option 2 is likely the cleanest: change all callback(Resources.Error70x70) calls in GetThumbNailOfBookCover to callback(Resources.Error70x70.Clone() as Image), matching the pattern already used in CreateThumbnailOfCoverImage at line 319.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

(info, ex) =>
{
throw ex;
Expand Down
6 changes: 5 additions & 1 deletion src/BloomExe/CollectionTab/CollectionModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
},
Comment on lines +724 to +728
Copy link
Copy Markdown

@devin-ai-integration devin-ai-integration bot Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Disposing shared Resources.Error70x70 singleton in UpdateThumbnailAsync callback

The new callback wrapping RefreshOneThumbnail calls image?.Dispose() on the image it receives. Through the same error paths in GetThumbNailOfBookCover (BookThumbNailer.cs:66,100,106), the image can be the shared Resources.Error70x70 singleton. The flow is: UpdateThumbnailAsync → RebuildThumbNailAsync → RebuildThumbNail → GetThumbNailOfBookCover → error path passes Resources.Error70x70 to callback → disposed. Note that while ErrorBook is filtered at CollectionModel.cs:713, the HasFatalError, CreateThumbnailOfCoverImage failure, and exception-catch paths in GetThumbNailOfBookCover are still reachable. This is the same root cause as the RebuildThumbNailNow bug but in a different call site.

Prompt for agents 
The callback added in UpdateThumbnailAsync(Book.Book book) disposes the image, but error paths in GetThumbNailOfBookCover (BookThumbNailer.cs:66,100,106) pass the shared Resources.Error70x70 singleton to the callback. Disposing it corrupts the shared resource.

The fix should be coordinated with the same issue in RebuildThumbNailNow. The cleanest approach is to ensure GetThumbNailOfBookCover always passes a clone of Resources.Error70x70 (matching the pattern in CreateThumbnailOfCoverImage at line 319 which already clones). This way all callers can safely dispose the image they receive.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

HandleThumbnailerErrror
);
}
Expand Down
53 changes: 27 additions & 26 deletions src/BloomExe/ImageProcessing/RuntimeImageProcessor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
22 changes: 20 additions & 2 deletions src/BloomExe/web/RequestInfo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
using Bloom.Book;
using Bloom.web;
using Bloom.web.controllers;
using SIL.Code;
using SIL.IO;
using SIL.Reporting;

Expand Down Expand Up @@ -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
Expand All @@ -152,7 +170,7 @@ public void ReplyWithFileContent(string path, string originalPath = null)

try
{
fs = RobustFile.OpenRead(path);
fs = OpenSharedReadStreamWithRetry(path);
}
catch (Exception error)
{
Expand Down Expand Up @@ -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)
{
Expand Down