Добавлен параметр завершения дочерних процессов в метод Завершить()#1660
Добавлен параметр завершения дочерних процессов в метод Завершить()#1660yukon39 wants to merge 1 commit intoEvilBeaver:developfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
yukon39
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/OneScript.StandardLibrary/Processes/ProcessContext.cs`:
- Around line 199-207: Update the XML documentation for ProcessContext.Stop (the
method wrapping _p.Kill) to explicitly state that calling Stop(true) /
Завершить(Истина) kills the root process but WaitForExit and the HasExited
property do not wait for or reflect termination of descendant processes, so
callers using ОжидатьЗавершения() / WaitForExit after Stop(true) may still
observe running child processes and should handle descendant termination
separately; include this note in the <summary> or as an additional <remarks>
sentence so it's clearly visible in generated docs.
- Around line 199-207: The Stop method calls _p.Kill(entireProcessTree) which
can throw AggregateException when Kill(true) cannot terminate some descendant
processes (e.g., due to permissions); update Stop to catch AggregateException
from the call to Kill and rethrow a clearer exception (e.g., throw new
InvalidOperationException("Failed to terminate one or more child processes.",
ex)) preserving the original AggregateException as InnerException, and also add
an XML <exception> entry to Stop's doc comment that documents this
AggregateException/InvalidOperationException scenario when entireProcessTree is
true; reference the Stop method and the call to Kill(bool) when making the
changes.
| /// <summary> | ||
| /// Завершить процесс и, опционально, все его дочерние процессы. | ||
| /// </summary> | ||
| /// <param name="entireProcessTree">Булево. Завершить все дочерние процессы.</param> | ||
| [ContextMethod("Завершить", "Stop")] | ||
| public void Stop(bool entireProcessTree = false) | ||
| { | ||
| _p.Kill(); | ||
| _p.Kill(entireProcessTree); | ||
| } |
There was a problem hiding this comment.
WaitForExit/HasExited do not track descendant termination after Kill(true)
The WaitForExit method and the HasExited property do not reflect the status of descendant processes; when Kill(entireProcessTree: true) is used, they will indicate completion after the root process exits, even if all descendants have not yet exited.
This is a subtle behavioral difference worth noting in the method's XML documentation so that callers who call ОжидатьЗавершения() / WaitForExit after Завершить(Истина) are not surprised.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/OneScript.StandardLibrary/Processes/ProcessContext.cs` around lines 199 -
207, Update the XML documentation for ProcessContext.Stop (the method wrapping
_p.Kill) to explicitly state that calling Stop(true) / Завершить(Истина) kills
the root process but WaitForExit and the HasExited property do not wait for or
reflect termination of descendant processes, so callers using
ОжидатьЗавершения() / WaitForExit after Stop(true) may still observe running
child processes and should handle descendant termination separately; include
this note in the <summary> or as an additional <remarks> sentence so it's
clearly visible in generated docs.
AggregateException from Kill(true) is unhandled and undocumented
Kill(bool) can throw an AggregateException when not all processes in the associated process's descendant tree could be terminated. This occurs when Kill(true) tries to terminate child processes but lacks permissions for some of them — for example, processes owned by different users.
The XML <summary> and <param> doc do not mention this, so script callers have no warning. Consider either wrapping it or at minimum adding a <exception> doc tag.
📝 Suggested doc addition
/// <summary>
/// Завершить процесс и, опционально, все его дочерние процессы.
/// </summary>
/// <param name="entireProcessTree">Булево. Завершить все дочерние процессы.</param>
+/// <exception cref="AggregateException">Не все дочерние процессы удалось завершить (только при entireProcessTree = true).</exception>
[ContextMethod("Завершить", "Stop")]
public void Stop(bool entireProcessTree = false)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// <summary> | |
| /// Завершить процесс и, опционально, все его дочерние процессы. | |
| /// </summary> | |
| /// <param name="entireProcessTree">Булево. Завершить все дочерние процессы.</param> | |
| [ContextMethod("Завершить", "Stop")] | |
| public void Stop(bool entireProcessTree = false) | |
| { | |
| _p.Kill(); | |
| _p.Kill(entireProcessTree); | |
| } | |
| /// <summary> | |
| /// Завершить процесс и, опционально, все его дочерние процессы. | |
| /// </summary> | |
| /// <param name="entireProcessTree">Булево. Завершить все дочерние процессы.</param> | |
| /// <exception cref="AggregateException">Не все дочерние процессы удалось завершить (только при entireProcessTree = true).</exception> | |
| [ContextMethod("Завершить", "Stop")] | |
| public void Stop(bool entireProcessTree = false) | |
| { | |
| _p.Kill(entireProcessTree); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/OneScript.StandardLibrary/Processes/ProcessContext.cs` around lines 199 -
207, The Stop method calls _p.Kill(entireProcessTree) which can throw
AggregateException when Kill(true) cannot terminate some descendant processes
(e.g., due to permissions); update Stop to catch AggregateException from the
call to Kill and rethrow a clearer exception (e.g., throw new
InvalidOperationException("Failed to terminate one or more child processes.",
ex)) preserving the original AggregateException as InnerException, and also add
an XML <exception> entry to Stop's doc comment that documents this
AggregateException/InvalidOperationException scenario when entireProcessTree is
true; reference the Stop method and the call to Kill(bool) when making the
changes.
1 participant
Описание изменений:
ProcessContext.Stop/Завершить: добавлен опциональный параметрentireProcessTreeдля завершения дерева процесса.Stop()завершает только текущий процесс (entireProcessTree = false)..NETчерезProcess.Kill(entireProcessTree).closes #1659
Summary by CodeRabbit