refactor(core): правки по код-ревью (COM-утечки, безопасный shutdown, типобезопасность)
- PartModeler: ResetAsync/Dispose освобождают RCW и чистят реестр эскизов/операций; Editor освобождается после EndEdit (C1) - DocumentTools: сброс реестра при create/open/close — нет устаревших id (C2) - DocumentService: освобождение транзитных IDocuments/IKompasDocument (m9) - KompasSession.Dispose: ограниченное ожидание 5с вместо вечного блока (M3) - PartModeler.ExtrudeAsync: убран dynamic -> конкретные definition-типы; throughAll только для cut (M4) - FeatureTools.extrude_cut: дефолт throughAll=false, без подмены depth (m7) - tests: регрессия на сброс реестра (старый id недействителен) - docs/OPEN_QUESTIONS.md: итоги ревью (исправлено/отложено) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This commit is contained in:
@@ -29,6 +29,30 @@
|
||||
|
||||
---
|
||||
|
||||
## Само-ревью кода (ночная сессия) — итоги
|
||||
|
||||
**Исправлено сразу:**
|
||||
- C1/C2 — утечки COM и устаревшие id: `PartModeler` освобождает RCW и сбрасывает реестр
|
||||
(`ResetAsync`/`Dispose`), `DocumentTools` сбрасывает реестр при create/open/close;
|
||||
`DocumentService` освобождает транзитные `IDocuments`/`IKompasDocument`.
|
||||
- M3 — `KompasSession.Dispose`: ограниченное ожидание (5 c) вместо вечного блока.
|
||||
- M4 — `ExtrudeAsync`: убран `dynamic`, конкретные `ksBossExtrusionDefinition`/
|
||||
`ksCutExtrusionDefinition`; `throughAll` разрешён только для выреза.
|
||||
- m7 — `extrude_cut`: дефолт `throughAll=false`, без молчаливой подмены `depth`.
|
||||
- Регрессионный тест: после `ResetAsync` старый id эскиза недействителен.
|
||||
|
||||
**⚠️ Отложено на проработку (из ревью):**
|
||||
- M5 — отмена/насос сообщений: `CancellationToken` не прерывает уже идущий COM-вызов; зависший
|
||||
вызов (модальный диалог КОМПАС) блокирует всю очередь. Возможное решение — диспетчер с насосом
|
||||
сообщений + подавление диалогов. Связано с [конкурентностью MCP].
|
||||
- m6 — `colorBPP=24` для снимка: для GIF (палитра) может не подойти; снимок по умолчанию PNG.
|
||||
- m8 — приведения enum к `short` для `GetPart/NewEntity/GetDefaultEntity`: на v24 работает
|
||||
(тесты зелёные), но стоит сверить точные типы параметров в interop.
|
||||
- m10 — `_app/_kompas` читаются из `IsConnected/Status()` вне STA-потока без `volatile`
|
||||
(доброкачественное устаревшее чтение).
|
||||
- m11 — трансляция `COMException`/HRESULT в понятные сообщения (сейчас понятны только наши
|
||||
`InvalidOperationException`); добавить текст последней ошибки КОМПАС.
|
||||
|
||||
## Журнал решений по ходу
|
||||
(дополняется автоматически во время работы)
|
||||
|
||||
|
||||
@@ -1,3 +1,4 @@
|
||||
using System.Runtime.InteropServices;
|
||||
using System.Runtime.Versioning;
|
||||
using Kompas.Mcp.Core.Threading;
|
||||
using Kompas6Constants;
|
||||
@@ -22,18 +23,22 @@ public sealed class DocumentService
|
||||
public Task<DocumentInfo> CreateAsync(KompasDocumentType type, bool visible = true, CancellationToken ct = default)
|
||||
=> _dispatcher.InvokeAsync(() =>
|
||||
{
|
||||
var doc = (IKompasDocument)_session.Application.Documents.Add(DocumentTypes.ToEnum(type), visible);
|
||||
return ToInfo(doc);
|
||||
var docs = _session.Application.Documents;
|
||||
var doc = (IKompasDocument)docs.Add(DocumentTypes.ToEnum(type), visible);
|
||||
try { return ToInfo(doc); }
|
||||
finally { ReleaseCom(doc); ReleaseCom(docs); }
|
||||
}, ct);
|
||||
|
||||
/// <summary>Открыть документ с диска.</summary>
|
||||
public Task<DocumentInfo> OpenAsync(string path, bool readOnly = false, bool visible = true, CancellationToken ct = default)
|
||||
=> _dispatcher.InvokeAsync(() =>
|
||||
{
|
||||
var doc = (IKompasDocument)_session.Application.Documents.Open(path, visible, readOnly);
|
||||
var docs = _session.Application.Documents;
|
||||
var doc = (IKompasDocument)docs.Open(path, visible, readOnly);
|
||||
if (doc is null)
|
||||
throw new InvalidOperationException($"Не удалось открыть документ: {path}");
|
||||
return ToInfo(doc);
|
||||
try { return ToInfo(doc); }
|
||||
finally { ReleaseCom(doc); ReleaseCom(docs); }
|
||||
}, ct);
|
||||
|
||||
/// <summary>Сводка по активному документу или <c>null</c>, если документов нет.</summary>
|
||||
@@ -41,25 +46,34 @@ public sealed class DocumentService
|
||||
=> _dispatcher.InvokeAsync(() =>
|
||||
{
|
||||
var doc = _session.Application.ActiveDocument;
|
||||
return doc is null ? null : ToInfo(doc);
|
||||
if (doc is null) return null;
|
||||
try { return ToInfo(doc); }
|
||||
finally { ReleaseCom(doc); }
|
||||
}, ct);
|
||||
|
||||
/// <summary>Сохранить активный документ (для уже сохранённого — без диалога).</summary>
|
||||
public Task SaveAsync(CancellationToken ct = default)
|
||||
=> _dispatcher.InvokeAsync(() => RequireActive().Save(), ct);
|
||||
=> _dispatcher.InvokeAsync(() => WithActive(d => d.Save()), ct);
|
||||
|
||||
/// <summary>Сохранить активный документ по указанному пути (безопасно для новых документов).</summary>
|
||||
public Task SaveAsAsync(string path, CancellationToken ct = default)
|
||||
=> _dispatcher.InvokeAsync(() => RequireActive().SaveAs(path), ct);
|
||||
=> _dispatcher.InvokeAsync(() => WithActive(d => d.SaveAs(path)), ct);
|
||||
|
||||
/// <summary>Закрыть активный документ. <paramref name="save"/> — сохранять ли изменения.</summary>
|
||||
public Task<bool> CloseAsync(bool save = false, CancellationToken ct = default)
|
||||
=> _dispatcher.InvokeAsync(() =>
|
||||
RequireActive().Close(save ? DocumentCloseOptions.kdSaveChanges : DocumentCloseOptions.kdDoNotSaveChanges), ct);
|
||||
=> _dispatcher.InvokeAsync(() => WithActive(d =>
|
||||
d.Close(save ? DocumentCloseOptions.kdSaveChanges : DocumentCloseOptions.kdDoNotSaveChanges)), ct);
|
||||
|
||||
private IKompasDocument RequireActive()
|
||||
=> _session.Application.ActiveDocument
|
||||
?? throw new InvalidOperationException("Нет активного документа.");
|
||||
private void WithActive(Action<IKompasDocument> action)
|
||||
=> WithActive(d => { action(d); return true; });
|
||||
|
||||
private T WithActive<T>(Func<IKompasDocument, T> func)
|
||||
{
|
||||
var doc = _session.Application.ActiveDocument
|
||||
?? throw new InvalidOperationException("Нет активного документа.");
|
||||
try { return func(doc); }
|
||||
finally { ReleaseCom(doc); }
|
||||
}
|
||||
|
||||
private static DocumentInfo ToInfo(IKompasDocument doc) => new()
|
||||
{
|
||||
@@ -67,4 +81,13 @@ public sealed class DocumentService
|
||||
PathName = doc.PathName ?? string.Empty,
|
||||
Type = DocumentTypes.FromEnum(doc.DocumentType),
|
||||
};
|
||||
|
||||
private static void ReleaseCom(object? comObject)
|
||||
{
|
||||
if (comObject is not null && Marshal.IsComObject(comObject))
|
||||
{
|
||||
try { Marshal.ReleaseComObject(comObject); }
|
||||
catch { /* освобождение не критично */ }
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -144,6 +144,7 @@ public sealed class KompasSession : IDisposable
|
||||
// Освобождаем COM-объекты на том же STA-потоке, на котором они созданы.
|
||||
try
|
||||
{
|
||||
// Ограниченное ожидание: не виснем на старте остановки, если STA-поток занят COM-вызовом.
|
||||
_dispatcher.InvokeAsync(() =>
|
||||
{
|
||||
// Quit не вызываем: не «убиваем» КОМПАС пользователя (см. docs/OPEN_QUESTIONS.md).
|
||||
@@ -151,11 +152,11 @@ public sealed class KompasSession : IDisposable
|
||||
if (_kompas is not null && Marshal.IsComObject(_kompas)) Marshal.ReleaseComObject(_kompas);
|
||||
_app = null;
|
||||
_kompas = null;
|
||||
}).GetAwaiter().GetResult();
|
||||
}).Wait(TimeSpan.FromSeconds(5));
|
||||
}
|
||||
catch (ObjectDisposedException)
|
||||
catch
|
||||
{
|
||||
// Диспетчер уже остановлен — COM освободит GC-финализатор.
|
||||
// Диспетчер остановлен / вызов завис — COM освободит GC-финализатор.
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1,3 +1,4 @@
|
||||
using System.Runtime.InteropServices;
|
||||
using System.Runtime.Versioning;
|
||||
using Kompas.Mcp.Core.Threading;
|
||||
using Kompas6API5;
|
||||
@@ -11,7 +12,7 @@ namespace Kompas.Mcp.Core.Modeling;
|
||||
/// ссылаться на них в цикле «эскиз → операция → эскиз».
|
||||
/// </summary>
|
||||
[SupportedOSPlatform("windows")]
|
||||
public sealed class PartModeler
|
||||
public sealed class PartModeler : IDisposable
|
||||
{
|
||||
private const int MainLineStyle = 1; // системный стиль «Основная»
|
||||
|
||||
@@ -122,23 +123,34 @@ public sealed class PartModeler
|
||||
var entity = part.NewEntity((short)type) as ksEntity
|
||||
?? throw new InvalidOperationException("NewEntity операции выдавливания вернул null.");
|
||||
|
||||
// ksBossExtrusionDefinition и ksCutExtrusionDefinition имеют одинаковый набор
|
||||
// нужных членов, но не общий интерфейс — настраиваем через dynamic.
|
||||
dynamic def = entity.GetDefinition()
|
||||
?? throw new InvalidOperationException("GetDefinition() операции вернул null.");
|
||||
|
||||
if (throughAll)
|
||||
// Конкретные типы определений (без dynamic): ошибки связывания — на этапе компиляции.
|
||||
if (cut)
|
||||
{
|
||||
def.directionType = (short)Direction_Type.dtBoth;
|
||||
def.SetSideParam(true, (short)End_Type.etThroughAll, depth, draftAngle, false);
|
||||
def.SetSideParam(false, (short)End_Type.etThroughAll, depth, draftAngle, false);
|
||||
var def = entity.GetDefinition() as ksCutExtrusionDefinition
|
||||
?? throw new InvalidOperationException("GetDefinition() выреза вернул не ksCutExtrusionDefinition.");
|
||||
if (throughAll)
|
||||
{
|
||||
def.directionType = (short)Direction_Type.dtBoth;
|
||||
def.SetSideParam(true, (short)End_Type.etThroughAll, depth, draftAngle, false);
|
||||
def.SetSideParam(false, (short)End_Type.etThroughAll, depth, draftAngle, false);
|
||||
}
|
||||
else
|
||||
{
|
||||
def.directionType = (short)Direction_Type.dtNormal;
|
||||
def.SetSideParam(forward, (short)End_Type.etBlind, depth, draftAngle, false);
|
||||
}
|
||||
def.SetSketch(sketch.Entity);
|
||||
}
|
||||
else
|
||||
{
|
||||
if (throughAll)
|
||||
throw new ArgumentException("throughAll поддерживается только для выреза (cut).", nameof(throughAll));
|
||||
var def = entity.GetDefinition() as ksBossExtrusionDefinition
|
||||
?? throw new InvalidOperationException("GetDefinition() выдавливания вернул не ksBossExtrusionDefinition.");
|
||||
def.directionType = (short)Direction_Type.dtNormal;
|
||||
def.SetSideParam(forward, (short)End_Type.etBlind, depth, draftAngle, false);
|
||||
def.SetSketch(sketch.Entity);
|
||||
}
|
||||
def.SetSketch(sketch.Entity);
|
||||
|
||||
if (!entity.Create())
|
||||
throw new InvalidOperationException("Create() операции выдавливания вернул FALSE.");
|
||||
@@ -156,6 +168,13 @@ public sealed class PartModeler
|
||||
doc3d.RebuildDocument();
|
||||
}, ct);
|
||||
|
||||
/// <summary>
|
||||
/// Сбросить реестр эскизов/операций и освободить их COM-объекты. Вызывать при смене
|
||||
/// активного документа (создание/открытие/закрытие) — id больше не действительны.
|
||||
/// </summary>
|
||||
public Task ResetAsync(CancellationToken ct = default)
|
||||
=> _dispatcher.InvokeAsync(ResetCore, ct);
|
||||
|
||||
// ---- Внутреннее --------------------------------------------------------
|
||||
|
||||
private ksDocument3D ActiveDoc3D()
|
||||
@@ -182,6 +201,38 @@ public sealed class PartModeler
|
||||
{
|
||||
if (s.Editor is null) return;
|
||||
s.Definition.EndEdit();
|
||||
ReleaseCom(s.Editor); // редактор после EndEdit больше не нужен
|
||||
s.Editor = null;
|
||||
}
|
||||
|
||||
private void ResetCore()
|
||||
{
|
||||
foreach (var s in _sketches.Values)
|
||||
{
|
||||
ReleaseCom(s.Editor);
|
||||
ReleaseCom(s.Definition);
|
||||
ReleaseCom(s.Entity);
|
||||
}
|
||||
foreach (var f in _features.Values)
|
||||
ReleaseCom(f);
|
||||
_sketches.Clear();
|
||||
_features.Clear();
|
||||
_nextId = 1;
|
||||
}
|
||||
|
||||
private static void ReleaseCom(object? comObject)
|
||||
{
|
||||
if (comObject is not null && Marshal.IsComObject(comObject))
|
||||
{
|
||||
try { Marshal.ReleaseComObject(comObject); }
|
||||
catch { /* освобождение не критично */ }
|
||||
}
|
||||
}
|
||||
|
||||
public void Dispose()
|
||||
{
|
||||
// Освобождаем COM на STA-потоке; не виснем на старте, если поток занят.
|
||||
try { _dispatcher.InvokeAsync(ResetCore).Wait(TimeSpan.FromSeconds(5)); }
|
||||
catch { /* при остановке диспетчера COM освободит GC */ }
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1,13 +1,14 @@
|
||||
using System.ComponentModel;
|
||||
using Kompas.Mcp.Core;
|
||||
using Kompas.Mcp.Core.Documents;
|
||||
using Kompas.Mcp.Core.Modeling;
|
||||
using ModelContextProtocol.Server;
|
||||
|
||||
namespace Kompas.Mcp.Host.Tools;
|
||||
|
||||
/// <summary>Инструменты управления документами.</summary>
|
||||
[McpServerToolType]
|
||||
public sealed class DocumentTools(KompasSession session, DocumentService docs)
|
||||
public sealed class DocumentTools(KompasSession session, DocumentService docs, PartModeler modeler)
|
||||
{
|
||||
[McpServerTool(Name = "document_create")]
|
||||
[Description("Создать новый документ и сделать его активным. type: part | assembly | drawing | fragment | specification | text.")]
|
||||
@@ -15,6 +16,7 @@ public sealed class DocumentTools(KompasSession session, DocumentService docs)
|
||||
{
|
||||
await session.ConnectAsync();
|
||||
var info = await docs.CreateAsync(DocumentTypes.Parse(type));
|
||||
await modeler.ResetAsync(); // прежние id эскизов/операций недействительны
|
||||
return $"Создан документ типа {info.Type}.";
|
||||
}
|
||||
|
||||
@@ -26,6 +28,7 @@ public sealed class DocumentTools(KompasSession session, DocumentService docs)
|
||||
{
|
||||
await session.ConnectAsync();
|
||||
var info = await docs.OpenAsync(path, readOnly);
|
||||
await modeler.ResetAsync();
|
||||
return $"Открыт документ: {info.Name} ({info.Type}).";
|
||||
}
|
||||
|
||||
@@ -53,6 +56,7 @@ public sealed class DocumentTools(KompasSession session, DocumentService docs)
|
||||
{
|
||||
await session.ConnectAsync();
|
||||
var ok = await docs.CloseAsync(save);
|
||||
await modeler.ResetAsync();
|
||||
return ok ? "Документ закрыт." : "Не удалось закрыть документ.";
|
||||
}
|
||||
|
||||
|
||||
@@ -19,15 +19,15 @@ public sealed class FeatureTools(KompasSession session, PartModeler modeler)
|
||||
}
|
||||
|
||||
[McpServerTool(Name = "extrude_cut")]
|
||||
[Description("Вырезать выдавливанием по эскизу. throughAll=true — насквозь (для отверстий, depth игнорируется); иначе на глубину depth.")]
|
||||
[Description("Вырезать выдавливанием по эскизу. Для сквозного отверстия передайте throughAll=true (depth не нужен). Иначе вырез на глубину depth (мм).")]
|
||||
public async Task<string> ExtrudeCut(
|
||||
int sketchId,
|
||||
[Description("Глубина, мм (если не throughAll)")] double depth = 0,
|
||||
[Description("Вырезать насквозь")] bool throughAll = true,
|
||||
[Description("Глубина, мм (нужна, если throughAll=false)")] double depth = 0,
|
||||
[Description("Вырезать насквозь через всю деталь")] bool throughAll = false,
|
||||
[Description("Прямое направление")] bool forward = true)
|
||||
{
|
||||
await session.ConnectAsync();
|
||||
var id = await modeler.ExtrudeAsync(sketchId, depth <= 0 ? 1 : depth, cut: true, forward: forward, throughAll: throughAll);
|
||||
var id = await modeler.ExtrudeAsync(sketchId, depth, cut: true, forward: forward, throughAll: throughAll);
|
||||
return $"Вырез (cut) создан, id={id}.";
|
||||
}
|
||||
|
||||
|
||||
@@ -52,4 +52,22 @@ public sealed class ModelingTests
|
||||
await _docs.CloseAsync(save: false);
|
||||
}
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task Reset_invalidates_sketch_ids()
|
||||
{
|
||||
await _docs.CreateAsync(KompasDocumentType.Part);
|
||||
try
|
||||
{
|
||||
var s = await _modeler.OpenSketchAsync(BasePlane.XOY);
|
||||
await _modeler.ResetAsync();
|
||||
// После сброса реестра старый id недействителен.
|
||||
await Assert.ThrowsAsync<KeyNotFoundException>(
|
||||
() => _modeler.AddCircleAsync(s, 0, 0, 5));
|
||||
}
|
||||
finally
|
||||
{
|
||||
await _docs.CloseAsync(save: false);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user