Skip to content

Commit 4f52b86

Browse files
ScriptwonderScriptwonder
committed
fix: move EditorPrefs access to main thread for background update checks
Address CodeRabbit review feedback on PR #954: CheckForUpdate was running entirely inside Task.Run, including EditorPrefs reads/writes and MCPServiceLocator.Updates lazy initialization — both unsafe off the main thread. Split CheckForUpdate into three thread-aware methods: - TryGetCachedResult: reads EditorPrefs cache (main thread) - FetchAndCompare: network I/O only (background thread safe) - CacheFetchResult: writes EditorPrefs cache (main thread) The editor window now resolves the service and checks cache on the main thread, runs only network I/O in Task.Run, and caches results back on the main thread via EditorApplication.delayCall.
1 parent b050907 commit 4f52b86

3 files changed

Lines changed: 177 additions & 57 deletions

File tree

‎MCPForUnity/Editor/Services/IPackageUpdateService.cs‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,23 @@ public interface IPackageUpdateService
1212
/// <returns>Update check result containing availability and latest version info</returns>
1313
UpdateCheckResult CheckForUpdate(string currentVersion);
1414

15+
/// <summary>
16+
/// Returns a cached update result if one exists for today, or null if a network fetch is needed.
17+
/// Safe to call from any thread context but designed for main-thread use (reads EditorPrefs).
18+
/// </summary>
19+
UpdateCheckResult TryGetCachedResult(string currentVersion);
20+
21+
/// <summary>
22+
/// Performs only the network fetch and version comparison (no EditorPrefs access).
23+
/// Safe to call from a background thread.
24+
/// </summary>
25+
UpdateCheckResult FetchAndCompare(string currentVersion);
26+
27+
/// <summary>
28+
/// Caches a successful fetch result in EditorPrefs. Must be called from the main thread.
29+
/// </summary>
30+
void CacheFetchResult(string currentVersion, string fetchedVersion);
31+
1532
/// <summary>
1633
/// Compares two version strings to determine if the first is newer than the second
1734
/// </summary>

‎MCPForUnity/Editor/Services/PackageUpdateService.cs‎

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,88 @@ public UpdateCheckResult CheckForUpdate(string currentVersion)
8282
};
8383
}
8484

85+
/// <inheritdoc/>
86+
public UpdateCheckResult TryGetCachedResult(string currentVersion)
87+
{
88+
bool isGitInstallation = IsGitInstallation();
89+
string gitBranch = isGitInstallation ? GetGitUpdateBranch(currentVersion) : "main";
90+
bool useBetaChannel = isGitInstallation && string.Equals(gitBranch, "beta", StringComparison.OrdinalIgnoreCase);
91+
92+
string lastCheckKey = isGitInstallation
93+
? (useBetaChannel ? LastBetaCheckDateKey : LastCheckDateKey)
94+
: LastAssetStoreCheckDateKey;
95+
string cachedVersionKey = isGitInstallation
96+
? (useBetaChannel ? CachedBetaVersionKey : CachedVersionKey)
97+
: CachedAssetStoreVersionKey;
98+
99+
string lastCheckDate = EditorPrefs.GetString(lastCheckKey, "");
100+
string cachedLatestVersion = EditorPrefs.GetString(cachedVersionKey, "");
101+
102+
if (lastCheckDate == DateTime.Now.ToString("yyyy-MM-dd") && !string.IsNullOrEmpty(cachedLatestVersion))
103+
{
104+
return new UpdateCheckResult
105+
{
106+
CheckSucceeded = true,
107+
LatestVersion = cachedLatestVersion,
108+
UpdateAvailable = IsNewerVersion(cachedLatestVersion, currentVersion),
109+
Message = "Using cached version check"
110+
};
111+
}
112+
113+
return null;
114+
}
115+
116+
/// <inheritdoc/>
117+
public UpdateCheckResult FetchAndCompare(string currentVersion)
118+
{
119+
bool isGitInstallation = IsGitInstallation();
120+
string gitBranch = isGitInstallation ? GetGitUpdateBranch(currentVersion) : "main";
121+
122+
string latestVersion = isGitInstallation
123+
? FetchLatestVersionFromGitHub(gitBranch)
124+
: FetchLatestVersionFromAssetStoreJson();
125+
126+
if (!string.IsNullOrEmpty(latestVersion))
127+
{
128+
return new UpdateCheckResult
129+
{
130+
CheckSucceeded = true,
131+
LatestVersion = latestVersion,
132+
UpdateAvailable = IsNewerVersion(latestVersion, currentVersion),
133+
Message = "Successfully checked for updates"
134+
};
135+
}
136+
137+
return new UpdateCheckResult
138+
{
139+
CheckSucceeded = false,
140+
UpdateAvailable = false,
141+
Message = isGitInstallation
142+
? "Failed to check for updates (network issue or offline)"
143+
: "Failed to check for Asset Store updates (network issue or offline)"
144+
};
145+
}
146+
147+
/// <inheritdoc/>
148+
public void CacheFetchResult(string currentVersion, string fetchedVersion)
149+
{
150+
if (string.IsNullOrEmpty(fetchedVersion)) return;
151+
152+
bool isGitInstallation = IsGitInstallation();
153+
string gitBranch = isGitInstallation ? GetGitUpdateBranch(currentVersion) : "main";
154+
bool useBetaChannel = isGitInstallation && string.Equals(gitBranch, "beta", StringComparison.OrdinalIgnoreCase);
155+
156+
string lastCheckKey = isGitInstallation
157+
? (useBetaChannel ? LastBetaCheckDateKey : LastCheckDateKey)
158+
: LastAssetStoreCheckDateKey;
159+
string cachedVersionKey = isGitInstallation
160+
? (useBetaChannel ? CachedBetaVersionKey : CachedVersionKey)
161+
: CachedAssetStoreVersionKey;
162+
163+
EditorPrefs.SetString(lastCheckKey, DateTime.Now.ToString("yyyy-MM-dd"));
164+
EditorPrefs.SetString(cachedVersionKey, fetchedVersion);
165+
}
166+
85167
/// <inheritdoc/>
86168
public bool IsNewerVersion(string version1, string version2)
87169
{

‎MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs‎

Lines changed: 78 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -50,9 +50,9 @@ public class MCPForUnityEditorWindow : EditorWindow
5050
private bool toolsLoaded = false;
5151
private bool resourcesLoaded = false;
5252
private double lastRefreshTime = 0;
53-
private const double RefreshDebounceSeconds = 0.5;
54-
private bool updateCheckQueued = false;
55-
private bool updateCheckInFlight = false;
53+
private const double RefreshDebounceSeconds = 0.5;
54+
private bool updateCheckQueued = false;
55+
private bool updateCheckInFlight = false;
5656

5757
private enum ActivePanel
5858
{
@@ -351,70 +351,91 @@ private void UpdateVersionLabel()
351351
: $"MCP For Unity v{version}";
352352
}
353353

354-
private void QueueUpdateCheck()
355-
{
356-
if (updateCheckQueued || updateCheckInFlight)
357-
{
358-
return;
359-
}
354+
private void QueueUpdateCheck()
355+
{
356+
if (updateCheckQueued || updateCheckInFlight)
357+
{
358+
return;
359+
}
360360

361361
updateCheckQueued = true;
362362
EditorApplication.delayCall += CheckForPackageUpdates;
363363
}
364364

365-
private void CheckForPackageUpdates()
366-
{
367-
updateCheckQueued = false;
368-
369-
if (updateNotification == null || updateNotificationText == null)
365+
private void CheckForPackageUpdates()
366+
{
367+
updateCheckQueued = false;
368+
369+
if (updateNotification == null || updateNotificationText == null)
370370
{
371371
return;
372372
}
373373

374374
string currentVersion = AssetPathUtility.GetPackageVersion();
375-
if (string.IsNullOrEmpty(currentVersion) || currentVersion == "unknown")
376-
{
377-
updateNotification.RemoveFromClassList("visible");
378-
return;
379-
}
380-
381-
updateCheckInFlight = true;
382-
Task.Run(() =>
383-
{
384-
try
385-
{
386-
return MCPServiceLocator.Updates.CheckForUpdate(currentVersion);
387-
}
388-
catch (Exception ex)
389-
{
390-
McpLog.Info($"Package update check skipped: {ex.Message}");
391-
return null;
392-
}
393-
}).ContinueWith(t =>
394-
{
395-
EditorApplication.delayCall += () =>
396-
{
397-
updateCheckInFlight = false;
398-
399-
if (this == null || updateNotification == null || updateNotificationText == null)
400-
{
401-
return;
402-
}
403-
404-
var result = t.Status == TaskStatus.RanToCompletion ? t.Result : null;
405-
if (result != null && result.CheckSucceeded && result.UpdateAvailable && !string.IsNullOrEmpty(result.LatestVersion))
406-
{
407-
updateNotificationText.text = $"Update available: v{result.LatestVersion} (current: v{currentVersion})";
408-
updateNotificationText.tooltip = $"Latest version: v{result.LatestVersion}\nCurrent version: v{currentVersion}";
409-
updateNotification.AddToClassList("visible");
410-
}
411-
else
412-
{
413-
updateNotification.RemoveFromClassList("visible");
414-
}
415-
};
416-
}, TaskScheduler.Default);
417-
}
375+
if (string.IsNullOrEmpty(currentVersion) || currentVersion == "unknown")
376+
{
377+
updateNotification.RemoveFromClassList("visible");
378+
return;
379+
}
380+
381+
// Main thread: resolve service + read EditorPrefs cache (both require main thread)
382+
var updateService = MCPServiceLocator.Updates;
383+
var cachedResult = updateService.TryGetCachedResult(currentVersion);
384+
if (cachedResult != null)
385+
{
386+
ApplyUpdateCheckResult(cachedResult, currentVersion);
387+
return;
388+
}
389+
390+
// Background thread: network I/O only (no EditorPrefs access)
391+
updateCheckInFlight = true;
392+
Task.Run(() =>
393+
{
394+
try
395+
{
396+
return updateService.FetchAndCompare(currentVersion);
397+
}
398+
catch (Exception ex)
399+
{
400+
McpLog.Info($"Package update check skipped: {ex.Message}");
401+
return null;
402+
}
403+
}).ContinueWith(t =>
404+
{
405+
EditorApplication.delayCall += () =>
406+
{
407+
updateCheckInFlight = false;
408+
409+
// Main thread: cache the result in EditorPrefs
410+
var result = t.Status == TaskStatus.RanToCompletion ? t.Result : null;
411+
if (result != null && result.CheckSucceeded && !string.IsNullOrEmpty(result.LatestVersion))
412+
{
413+
updateService.CacheFetchResult(currentVersion, result.LatestVersion);
414+
}
415+
416+
if (this == null || updateNotification == null || updateNotificationText == null)
417+
{
418+
return;
419+
}
420+
421+
ApplyUpdateCheckResult(result, currentVersion);
422+
};
423+
}, TaskScheduler.Default);
424+
}
425+
426+
private void ApplyUpdateCheckResult(UpdateCheckResult result, string currentVersion)
427+
{
428+
if (result != null && result.CheckSucceeded && result.UpdateAvailable && !string.IsNullOrEmpty(result.LatestVersion))
429+
{
430+
updateNotificationText.text = $"Update available: v{result.LatestVersion} (current: v{currentVersion})";
431+
updateNotificationText.tooltip = $"Latest version: v{result.LatestVersion}\nCurrent version: v{currentVersion}";
432+
updateNotification.AddToClassList("visible");
433+
}
434+
else
435+
{
436+
updateNotification.RemoveFromClassList("visible");
437+
}
438+
}
418439

419440
private void EnsureToolsLoaded()
420441
{

0 commit comments

Comments
 (0)