Skip to content

Commit 1145598

Browse files
committed
Refactor and fix issues from CodeRabbit review / CodeRabbit 리뷰 반영: 리팩토링 및 버그 수정
- Prevent process deadlocks in WindowsPlatformDetector and ServerEnvironmentSetup - Fix logic inversion and cleanup in ServerEnvironmentSetup - Add file existence check for Python path override - Ensure thread safety in MCPForUnityEditorWindow status check - Refactor Python closure in utils.py - Add missing TryParseVersion in PlatformDetectorBase
1 parent b094ed2 commit 1145598

File tree

7 files changed

+48
-83
lines changed

7 files changed

+48
-83
lines changed

MCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.cs

Lines changed: 11 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -233,83 +233,37 @@ private bool TryValidatePython(string pythonPath, out string version, out string
233233
{
234234
version = null;
235235
fullPath = null;
236-
237-
try
236+
237+
// 5 second timeout for validation
238+
if (TryExecuteProcess(pythonPath, "--version", 5000, out string output) && output.StartsWith("Python "))
238239
{
239-
var psi = new ProcessStartInfo
240-
{
241-
FileName = pythonPath,
242-
Arguments = "--version",
243-
UseShellExecute = false,
244-
RedirectStandardOutput = true,
245-
RedirectStandardError = true,
246-
CreateNoWindow = true
247-
};
240+
version = output.Substring(7); // Remove "Python " prefix
241+
fullPath = pythonPath;
248242

249-
using var process = Process.Start(psi);
250-
if (process == null) return false;
251-
252-
process.WaitForExit(5000);
253-
string output = process.StandardOutput.ReadToEnd().Trim();
254-
255-
if (process.ExitCode == 0 && output.StartsWith("Python "))
243+
// Validate minimum version (Python 4+ or python 3.11+)
244+
if (TryParseVersion(version, out var major, out var minor))
256245
{
257-
version = output.Substring(7); // Remove "Python " prefix
258-
fullPath = pythonPath;
259-
260-
// Validate minimum version (Python 4+ or python 3.11+)
261-
if (TryParseVersion(version, out var major, out var minor))
262-
{
263-
return major > 3 || (major == 3 && minor >= 11);
264-
}
246+
return major > 3 || (major == 3 && minor >= 11);
265247
}
266248
}
267-
catch
268-
{
269-
// Ignore validation errors
270-
}
271-
272249
return false;
273250
}
274251

275252
private bool TryFindInPath(string executable, out string fullPath)
276253
{
277254
fullPath = null;
278-
279-
try
255+
256+
// 3 second timeout for 'where'
257+
if (TryExecuteProcess("where", executable, 3000, out string output) && !string.IsNullOrEmpty(output))
280258
{
281-
var psi = new ProcessStartInfo
282-
{
283-
FileName = "where",
284-
Arguments = executable,
285-
UseShellExecute = false,
286-
RedirectStandardOutput = true,
287-
RedirectStandardError = true,
288-
CreateNoWindow = true
289-
};
290-
291-
using var process = Process.Start(psi);
292-
if (process == null) return false;
293-
294-
process.WaitForExit(3000);
295-
string output = process.StandardOutput.ReadToEnd().Trim();
296-
297-
if (process.ExitCode == 0 && !string.IsNullOrEmpty(output))
298-
{
299259
// Take the first result
300260
var lines = output.Split(new[] { '\r', '\n' }, StringSplitOptions.RemoveEmptyEntries);
301261
if (lines.Length > 0)
302262
{
303263
fullPath = lines[0].Trim();
304264
return File.Exists(fullPath);
305265
}
306-
}
307266
}
308-
catch
309-
{
310-
// Ignore errors
311-
}
312-
313267
return false;
314268
}
315269
}

MCPForUnity/Editor/Helpers/AssetPathUtility.cs

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -139,7 +139,6 @@ public static string GetWrapperJsPath()
139139
return wrapperPath;
140140
}
141141

142-
// Also check without the ~ just in case
143142
// Also check without the ~ just in case
144143
string wrapperPathNoTilde = Path.Combine(Path.GetDirectoryName(wrapperPath), "..", "Server", "wrapper.js");
145144
if (File.Exists(wrapperPathNoTilde))

MCPForUnity/Editor/Services/PathResolverService.cs

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -100,7 +100,7 @@ public string GetPythonPath()
100100
try
101101
{
102102
string overridePath = EditorPrefs.GetString(EditorPrefKeys.PythonPathOverride, string.Empty);
103-
if (!string.IsNullOrEmpty(overridePath))
103+
if (!string.IsNullOrEmpty(overridePath) && File.Exists(overridePath))
104104
{
105105
return overridePath;
106106
}
@@ -143,7 +143,11 @@ public bool IsPythonDetected()
143143
CreateNoWindow = true
144144
};
145145
using var p = Process.Start(psi);
146-
p.WaitForExit(2000);
146+
if (!p.WaitForExit(2000))
147+
{
148+
try { p.Kill(); } catch { /* ignore */ }
149+
return false;
150+
}
147151
return p.ExitCode == 0;
148152
}
149153
catch

MCPForUnity/Editor/Services/Transport/Transports/StdioBridgeHost.cs

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -520,7 +520,9 @@ private static async Task ListenerLoopAsync(CancellationToken token)
520520

521521
client.ReceiveTimeout = 60000;
522522

523-
// Create and track the client handler task
523+
// Use TaskCompletionSource to ensure the task is tracked in activeClientTasks
524+
// before the handler starts, preventing a race where the handler could complete
525+
// and remove itself before being added to the dictionary.
524526
var tcs = new TaskCompletionSource<object>(TaskCreationOptions.RunContinuationsAsynchronously);
525527
activeClientTasks.TryAdd(client, tcs.Task);
526528

MCPForUnity/Editor/Setup/ServerEnvironmentSetup.cs

Lines changed: 20 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,7 @@ public static void InstallServerEnvironment()
7676
if (string.IsNullOrEmpty(uvPath))
7777
{
7878
EditorUtility.DisplayProgressBar("MCP Setup", "Creating virtual environment...", 0.5f);
79-
if (!CreateVenv(uvPath))
79+
if (!CreateVenv())
8080
{
8181
EditorUtility.ClearProgressBar();
8282
EditorUtility.DisplayDialog("Error", "Failed to create virtual environment.", "OK");
@@ -188,23 +188,17 @@ private static string GetOrInstallUv()
188188
return null;
189189
}
190190

191-
private static bool CreateVenv(string uvPath)
191+
/// <summary>
192+
/// Creates a Python virtual environment manually. No-op if UV is available (UV handles venv creation).
193+
/// </summary>
194+
private static bool CreateVenv()
192195
{
193196
if (Directory.Exists(VenvPath))
194197
{
195198
McpLog.Info("[MCP Setup] Cleaning existing .venv...");
196199
try { Directory.Delete(VenvPath, true); } catch { /* ignore */ }
197200
}
198201

199-
// If uv is available, let uv sync handle venv creation automatically.
200-
// But if called explicitly, use python venv as fallback.
201-
if (!string.IsNullOrEmpty(uvPath))
202-
{
203-
// We can skip manual venv creation if using uv sync later,
204-
// but returning true here keeps the flow consistent.
205-
return true;
206-
}
207-
208202
McpLog.Info($"[MCP Setup] Creating virtual environment at: {VenvPath}");
209203
return RunCommand(GetPythonCommand(), $"-m venv \"{VenvPath}\"", out string output, workingDirectory: ServerRoot);
210204
}
@@ -264,8 +258,21 @@ private static bool RunCommand(string fileName, string arguments, out string out
264258
return false;
265259
}
266260

267-
output = p.StandardOutput.ReadToEnd();
268-
string error = p.StandardError.ReadToEnd();
261+
// Read streams asynchronously to prevent deadlock on buffer fill
262+
// Note: ReadToEndAsync is not available in .NET Standard 2.0 (Unity's default profile), using wrapper or async delegate
263+
var outputTask = Task.Run(() => p.StandardOutput.ReadToEnd());
264+
var errorTask = Task.Run(() => p.StandardError.ReadToEnd());
265+
266+
// 2 minute timeout to prevent hanging indefinitely
267+
if (!p.WaitForExit(120000))
268+
{
269+
try { p.Kill(); } catch {}
270+
McpLog.Error($"[MCP Setup] Command timed out: {fileName} {arguments}");
271+
return false;
272+
}
273+
274+
output = outputTask.Result;
275+
string error = errorTask.Result;
269276

270277
// 2 minute timeout to prevent hanging indefinitely
271278
if (!p.WaitForExit(120000))

MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -312,13 +312,12 @@ private void ToggleSetup()
312312
setupContainer.style.display = isSetupOpen ? DisplayStyle.Flex : DisplayStyle.None;
313313
}
314314

315-
private bool isChecking = false;
315+
private int _isChecking = 0;
316316

317317
private void CheckSystemStatus()
318318
{
319-
if (isChecking) return;
320-
isChecking = true;
321-
319+
if (System.Threading.Interlocked.CompareExchange(ref _isChecking, 1, 0) != 0) return;
320+
322321
// Show checking state
323322
statusText.text = "Checking...";
324323
statusIndicator.RemoveFromClassList("ready");
@@ -370,7 +369,7 @@ private void CheckSystemStatus()
370369
}
371370
finally
372371
{
373-
isChecking = false;
372+
System.Threading.Interlocked.Exchange(ref _isChecking, 0);
374373
}
375374
}
376375

MCPForUnity/Server~/src/services/tools/utils.py

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -242,10 +242,10 @@ def apply_edits_locally(original_text: str, edits: list[dict[str, Any]]) -> str:
242242
or start_col < 1 or end_col < 1):
243243
raise RuntimeError("replace_range out of bounds")
244244

245-
def index_of(line: int, col: int) -> int:
246-
if line <= len(lines):
247-
return sum(len(ln) for ln in lines[: line - 1]) + (col - 1)
248-
return sum(len(ln) for ln in lines)
245+
def index_of(line: int, col: int, lines_ref: list[str] = lines) -> int:
246+
if line <= len(lines_ref):
247+
return sum(len(ln) for ln in lines_ref[: line - 1]) + (col - 1)
248+
return sum(len(ln) for ln in lines_ref)
249249
a = index_of(start_line, start_col)
250250
b = index_of(end_line, end_col)
251251
text = text[:a] + replacement + text[b:]

0 commit comments

Comments
 (0)