Skip to content

fix: NullReferenceException in RemoveLoading (#39) + bump to 7.0.3#41

Merged
IvanMurzak merged 2 commits into
mainfrom
fix/issue-39-nre-remove-loading
May 28, 2026
Merged

fix: NullReferenceException in RemoveLoading (#39) + bump to 7.0.3#41
IvanMurzak merged 2 commits into
mainfrom
fix/issue-39-nre-remove-loading

Conversation

@IvanMurzak

Copy link
Copy Markdown
Owner

Summary

Fixes the NullReferenceException reported in #39 and bumps the package to 7.0.3.

Root cause

In Future.Loading.List.cs, RemoveLoading(string url) calls loadingInProcess.TryRemove(url, out var future). When TryRemove returns false (the entry is not present — e.g. it was already removed by a concurrent completion/cancellation), the out variable future is null. The else (warning) branch then logged Future[id={future.Id}], dereferencing the null future and throwing NullReferenceException.

Fix

In the warning branch, log using the in-scope url only, without dereferencing the null future:

- Debug.LogWarning($"[ImageLoader] Future[id={future.Id}] Wasn't able to remove loading registration, not found in loading tasks\n{url}");
+ Debug.LogWarning($"[ImageLoader] Wasn't able to remove loading registration, not found in loading tasks\n{url}");

The DebugLevel.Warning guard, indentation, and code style are preserved. The success branch (where future is non-null) is unchanged.

Review outcome

Ran a high-effort code review over the diff. No additional non-minor findings:

  • The surrounding lock (loadingInProcess) shared between RegisterLoading/RemoveLoading makes the check-then-act atomic and consistent; locking is sound.
  • The only other deref of an out from a failed lookup — anotherLoadingFuture at the RegisterLoading call site — is only used on the path where the value was set by a successful TryGetValue (non-null), so it is safe.
  • No other TryRemove/TryGetValue null-deref sites exist in the file or its immediate call sites.

Version bump

Ran bump-version.ps1 -NewVersion 7.0.3, which updated package.json and the Installer.cs Version constant (README was already dropped from the bump targets upstream).

Fixes #39

IvanMurzak and others added 2 commits May 28, 2026 14:51
… 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
IvanMurzak merged commit 2809eb0 into main May 28, 2026
41 of 43 checks passed
@IvanMurzak
IvanMurzak deleted the fix/issue-39-nre-remove-loading branch May 28, 2026 21:53
@IvanMurzak IvanMurzak self-assigned this 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}");

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

A NullReferenceException occurs in Future.Loading.List.cs

1 participant