Fix hang when theme thumbnail cannot be generated - #699
Open
rmbakker88 wants to merge 1 commit into
Open
Conversation
GetThumbnailImage decodes images with System.Drawing, which throws ArgumentException or ExternalException for formats GDI+ cannot read. LoadThemes only caught OutOfMemoryException, so any other decode failure escaped the loop, faulted the background task started by LoadImportedThemes, and left the import dialog stuck on "Generating thumbnails, please wait..." with no error and nothing logged. The loadSemaphore was leaked at the same time, so later loads blocked for 60 seconds. Catch all exceptions per theme and log them, release the semaphore from a finally block (and only when it was acquired), and close the import dialog from a finally block so importing always completes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
LoadThemes can proceed even when loadSemaphore.Wait(60000) times out, allowing concurrent loads and risking UI state races.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR addresses a hang in the theme import flow when thumbnail generation fails (e.g., unsupported/corrupt image formats causing System.Drawing to throw), ensuring the import dialog can complete and failures are surfaced/logged instead of leaving the UI stuck.
Changes:
- Broaden per-theme exception handling during thumbnail generation and log failures with theme id, while marshaling error UI handling onto the UI thread.
- Ensure
loadSemaphoreis released viafinallyto avoid stranding the semaphore on exceptions. - Ensure the import dialog is closed from a
finallyblock so imports complete even if thumbnail generation throws.
File summaries
| File | Description |
|---|---|
| src/ThemeDialogUtils.cs | Adds robust exception handling and finally cleanup to prevent the import dialog hang when thumbnail generation fails. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+55
to
56
| bool semaphoreAcquired = loadSemaphore.Wait(60000); | ||
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #696
Importing a theme whose images GDI+ cannot decode leaves the import dialog stuck on "Generating thumbnails, please wait..." forever, with no error and nothing in the log.
.webpis the easy way to hit it, but a truncated JPEG does the same thing.GetThumbnailImagedecodes withSystem.Drawing, which throwsExternalException(orArgumentException) for a format it cannot read.LoadThemesonly caughtOutOfMemoryException, so the exception escaped the loop and faulted the background task started byLoadImportedThemes— which closes the dialog on the line afterLoadThemesreturns, sothumbnailsLoadedwas never set andOnImportDialogClosingkept cancelling the close.Three changes, all in
ThemeDialogUtils:OutOfMemoryException, and log it with the theme id, so the underlying exception ends up indebug.logwhen debug logging is on. The existingFailedToCreateThumbnaildialog still shows, now marshalled onto the UI thread rather than raised from the background task.loadSemaphorefrom afinally, and only when the wait actually acquired it. Previously the release was the last statement of the method, so an escaping exception stranded the semaphore and the next load blocked for the full 60s.finallyinLoadImportedThemes, so the import completes even when generating thumbnails throws. This is the part that actually ends the hang.This deliberately does not change which formats are supported — an undecodable image still fails, it just fails visibly and lets the app carry on. Adding WebP support is #690, and a separate PR.
Most of the diff is re-indentation from the new
tryblock;?w=1shows the real change.Testing
dotnet test --filter "type!=system"— 7 passed, unchanged frommain..ddwwhose images are.webp: onmainthe import dialog hangs indefinitely and the app has to be killed; with this change the import completes, the theme is reported as "Failed to generate thumbnail: The image could not be loaded", and the app stays usable..jpgtheme behaves as before.