fix: NullReferenceException in RemoveLoading (#39) + bump to 7.0.3#41
Merged
Conversation
… removed (#39) When ConcurrentDictionary.TryRemove returns false the out 'future' is null, so logging future.Id in the warning branch threw a NullReferenceException. Log using the in-scope url instead. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
IvanMurzak
commented
May 28, 2026
| else | ||
| { | ||
| if (ImageLoader.settings.debugLevel.IsActive(DebugLevel.Warning)) | ||
| Debug.LogWarning($"[ImageLoader] Future[id={future.Id}] Wasn't able to remove loading registration, not found in loading tasks\n{url}"); |
Owner
Author
There was a problem hiding this comment.
Lets still print the future id if it is known, if not lets replace it with "null"
Future[id={future?.Id}]
IvanMurzak
added a commit
that referenced
this pull request
May 28, 2026
Address PR #41 review: keep the Future id in the "wasn't able to remove loading registration" warning, but use null-safe future?.Id so it no longer throws when the entry was already removed (future is null). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
Summary
Fixes the
NullReferenceExceptionreported in #39 and bumps the package to 7.0.3.Root cause
In
Future.Loading.List.cs,RemoveLoading(string url)callsloadingInProcess.TryRemove(url, out var future). WhenTryRemovereturnsfalse(the entry is not present — e.g. it was already removed by a concurrent completion/cancellation), theoutvariablefutureisnull. Theelse(warning) branch then loggedFuture[id={future.Id}], dereferencing the nullfutureand throwingNullReferenceException.Fix
In the warning branch, log using the in-scope
urlonly, without dereferencing the nullfuture:The
DebugLevel.Warningguard, indentation, and code style are preserved. The success branch (wherefutureis non-null) is unchanged.Review outcome
Ran a high-effort code review over the diff. No additional non-minor findings:
lock (loadingInProcess)shared betweenRegisterLoading/RemoveLoadingmakes the check-then-act atomic and consistent; locking is sound.outfrom a failed lookup —anotherLoadingFutureat theRegisterLoadingcall site — is only used on the path where the value was set by a successfulTryGetValue(non-null), so it is safe.TryRemove/TryGetValuenull-deref sites exist in the file or its immediate call sites.Version bump
Ran
bump-version.ps1 -NewVersion 7.0.3, which updatedpackage.jsonand the Installer.csVersionconstant (README was already dropped from the bump targets upstream).Fixes #39