From bd65ffbc3cca8f4b588df3bf778a0c1931f17fc5 Mon Sep 17 00:00:00 2001 From: neodk2004 Date: Fri, 16 Oct 2026 06:21:40 +0200 Subject: [PATCH] Verbindung: Zeitlimit je Zugriffsart, binaere API strikt nacheinander, Trap/Done-Fix (#40, #41) - RouterOsTimeouts: Lesen 15 s, Schreiben 30 s, Befehle (Backup/Restore/Update) 120 s, Verbindungstest 8 s; vorher global 8 s (Fehlalarme auf schwacher Hardware, Mac v1.3/v1.6) - BinaryRouterOsClient: Zugriffe per SemaphoreSlim serialisiert, nach !trap wird das folgende !done mitgelesen (sonst las der naechste Befehl eine fremde Antwort) - REST-Fehler nennen RouterOS' 'detail' - IRouterOsClient.RunAsync liefert die Ausgabe eines Befehls (Basis fuer Ping/Traceroute) Co-Authored-By: Claude Opus 5.5 --- .../Connectivity/BinaryRouterOsClientTests.cs | 66 +++++++ .../Binary/BinaryRouterOsClient.cs | 163 +++++++++++------- .../Connectivity/Binary/RouterOsReply.cs | 6 +- .../Connectivity/IRouterOsClient.cs | 11 ++ .../Connectivity/Rest/RestRouterOsClient.cs | 70 ++++++-- .../Connectivity/RouterOsTimeouts.cs | 19 ++ .../MikrotikInstaller.Core.csproj | 4 + 7 files changed, 261 insertions(+), 78 deletions(-) create mode 100644 src/MikrotikInstaller.Core.Tests/Connectivity/BinaryRouterOsClientTests.cs create mode 100644 src/MikrotikInstaller.Core/Connectivity/RouterOsTimeouts.cs diff --git a/src/MikrotikInstaller.Core.Tests/Connectivity/BinaryRouterOsClientTests.cs b/src/MikrotikInstaller.Core.Tests/Connectivity/BinaryRouterOsClientTests.cs new file mode 100644 index 0000000..1fb4c4f --- /dev/null +++ b/src/MikrotikInstaller.Core.Tests/Connectivity/BinaryRouterOsClientTests.cs @@ -0,0 +1,66 @@ +using MikrotikInstaller.Core.Connectivity; +using MikrotikInstaller.Core.Connectivity.Binary; + +namespace MikrotikInstaller.Core.Tests.Connectivity; + +public class BinaryRouterOsClientTests +{ + [Fact] + public async Task Trap_ConsumesTrailingDone_SoNextCommandReadsItsOwnReply() + { + var replies = await EncodeAsync( + ["!trap", "=message=failure: already have such entry"], + ["!done"], + ["!done", "=ret=*5"]); + await using var client = new BinaryRouterOsClient(new ReplayStream(replies)); + + var error = await Assert.ThrowsAsync(() => + client.AddAsync("/ip/pool", new Dictionary { ["name"] = "x" })); + Assert.Contains("already have such entry", error.Message); + + var id = await client.AddAsync("/ip/pool", new Dictionary { ["name"] = "y" }); + Assert.Equal("*5", id); + } + + [Fact] + public async Task RunAsync_ReturnsRowsOfCommand() + { + var replies = await EncodeAsync( + ["!re", "=host=1.1.1.1", "=time=12ms"], + ["!re", "=host=1.1.1.1", "=time=11ms"], + ["!done"]); + await using var client = new BinaryRouterOsClient(new ReplayStream(replies)); + + var rows = await client.RunAsync("/ping", new Dictionary { ["address"] = "1.1.1.1", ["count"] = "2" }); + + Assert.Equal(["12ms", "11ms"], rows.Select(r => r["time"])); + } + + private static async Task EncodeAsync(params string[][] sentences) + { + await using var buffer = new MemoryStream(); + foreach (var sentence in sentences) + { + await RouterOsWireFormat.WriteSentenceAsync(buffer, sentence, CancellationToken.None); + } + + return buffer.ToArray(); + } + + /// Liest vorbereitete Antworten, verwirft alles Geschriebene. + private sealed class ReplayStream(byte[] replies) : Stream + { + private readonly MemoryStream _replies = new(replies); + + public override bool CanRead => true; + public override bool CanSeek => false; + public override bool CanWrite => true; + public override long Length => throw new NotSupportedException(); + public override long Position { get => throw new NotSupportedException(); set => throw new NotSupportedException(); } + public override void Flush() { } + public override int Read(byte[] buffer, int offset, int count) => _replies.Read(buffer, offset, count); + public override long Seek(long offset, SeekOrigin origin) => throw new NotSupportedException(); + public override void SetLength(long value) => throw new NotSupportedException(); + public override void Write(byte[] buffer, int offset, int count) { } + } +} diff --git a/src/MikrotikInstaller.Core/Connectivity/Binary/BinaryRouterOsClient.cs b/src/MikrotikInstaller.Core/Connectivity/Binary/BinaryRouterOsClient.cs index b9ffdd3..853e40d 100644 --- a/src/MikrotikInstaller.Core/Connectivity/Binary/BinaryRouterOsClient.cs +++ b/src/MikrotikInstaller.Core/Connectivity/Binary/BinaryRouterOsClient.cs @@ -12,12 +12,23 @@ namespace MikrotikInstaller.Core.Connectivity.Binary; /// public sealed class BinaryRouterOsClient : IRouterOsClient { - private readonly TcpClient _tcpClient; + private readonly TcpClient? _tcpClient; private readonly Stream _stream; + private readonly SemaphoreSlim _gate = new(1, 1); + private bool _isBroken; + + /// Nach Zeitüberschreitung oder Abbruch mitten in einer Antwort ist diese Verbindung nicht mehr nutzbar. + public bool IsBroken => _isBroken; public RouterOsProtocol Protocol => RouterOsProtocol.Binary; - private BinaryRouterOsClient(TcpClient tcpClient, Stream stream) + /// Für Tests: Client über einen bereits geöffneten Datenstrom, ohne Login. + internal BinaryRouterOsClient(Stream stream) + : this(null, stream) + { + } + + private BinaryRouterOsClient(TcpClient? tcpClient, Stream stream) { _tcpClient = tcpClient; _stream = stream; @@ -89,77 +100,51 @@ public sealed class BinaryRouterOsClient : IRouterOsClient ], cancellationToken); - var reply = await ReadReplyAsync(cancellationToken); + using var timeoutCts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); + timeoutCts.CancelAfter(RouterOsTimeouts.ConnectProbe); + RouterOsReply reply; + try + { + reply = await ReadReplyAsync(timeoutCts.Token); + } + catch (Exception ex) when (ex is OperationCanceledException or IOException) + { + throw new RouterOsException("Das Gerät hat auf die Anmeldung nicht geantwortet.", ex); + } + if (reply.Status == RouterOsReplyStatus.Trap) { throw new RouterOsException("Benutzername oder Passwort ist falsch."); } } - public async Task>> GetAsync(string path, CancellationToken cancellationToken = default) - { - await RouterOsWireFormat.WriteSentenceAsync(_stream, [NormalizeCommand(path, "print")], cancellationToken); - - var rows = new List>(); - while (true) - { - var reply = await ReadReplyAsync(cancellationToken); - switch (reply.Status) - { - case RouterOsReplyStatus.Row: - rows.Add(reply.Attributes); - break; - case RouterOsReplyStatus.Done: - return rows; - case RouterOsReplyStatus.Trap: - throw new RouterOsException(DescribeError(reply)); - } - } - } + public Task>> GetAsync(string path, CancellationToken cancellationToken = default) => + SendAsync([NormalizeCommand(path, "print")], RouterOsTimeouts.Read, cancellationToken); public async Task AddAsync(string path, IReadOnlyDictionary parameters, CancellationToken cancellationToken = default) { var words = new List { NormalizeCommand(path, "add") }; words.AddRange(parameters.Select(kv => $"={kv.Key}={kv.Value}")); - await RouterOsWireFormat.WriteSentenceAsync(_stream, words, cancellationToken); - var reply = await ReadReplyAsync(cancellationToken); - if (reply.Status == RouterOsReplyStatus.Trap) - { - throw new RouterOsException(DescribeError(reply)); - } - - return reply.Attributes.GetValueOrDefault("ret", string.Empty); + var rows = await SendAsync(words, RouterOsTimeouts.Write, cancellationToken); + return rows.Count > 0 ? rows[^1].GetValueOrDefault("ret", string.Empty) : string.Empty; } public async Task SetAsync(string path, string id, IReadOnlyDictionary parameters, CancellationToken cancellationToken = default) { var words = new List { NormalizeCommand(path, "set"), $"=.id={id}" }; words.AddRange(parameters.Select(kv => $"={kv.Key}={kv.Value}")); - - await RouterOsWireFormat.WriteSentenceAsync(_stream, words, cancellationToken); - var reply = await ReadReplyAsync(cancellationToken); - if (reply.Status == RouterOsReplyStatus.Trap) - { - throw new RouterOsException(DescribeError(reply)); - } + await SendAsync(words, RouterOsTimeouts.Write, cancellationToken); } - public async Task RemoveAsync(string path, string id, CancellationToken cancellationToken = default) - { - await RouterOsWireFormat.WriteSentenceAsync( - _stream, - [NormalizeCommand(path, "remove"), $"=.id={id}"], - cancellationToken); + public async Task RemoveAsync(string path, string id, CancellationToken cancellationToken = default) => + await SendAsync([NormalizeCommand(path, "remove"), $"=.id={id}"], RouterOsTimeouts.Write, cancellationToken); - var reply = await ReadReplyAsync(cancellationToken); - if (reply.Status == RouterOsReplyStatus.Trap) - { - throw new RouterOsException(DescribeError(reply)); - } - } + public async Task ExecuteAsync(string path, IReadOnlyDictionary? parameters = null, CancellationToken cancellationToken = default) => + await RunAsync(path, parameters, cancellationToken); - public async Task ExecuteAsync(string path, IReadOnlyDictionary? parameters = null, CancellationToken cancellationToken = default) + public Task>> RunAsync( + string path, IReadOnlyDictionary? parameters = null, CancellationToken cancellationToken = default) { var words = new List { path.StartsWith('/') ? path : "/" + path }; if (parameters is not null) @@ -167,21 +152,75 @@ public sealed class BinaryRouterOsClient : IRouterOsClient words.AddRange(parameters.Select(kv => $"={kv.Key}={kv.Value}")); } - await RouterOsWireFormat.WriteSentenceAsync(_stream, words, cancellationToken); + return SendAsync(words, RouterOsTimeouts.Command, cancellationToken); + } - while (true) + /// + /// Schickt einen Satz und liest bis einschließlich "!done" — auch nach einem "!trap", denn RouterOS + /// schickt danach noch ein "!done", das sonst als Antwort des nächsten Befehls gelesen würde. + /// Zugriffe laufen strikt nacheinander: die binäre API ordnet Antworten nur über die Reihenfolge + /// zu, ein paralleler Zugriff (z. B. Update-Prüfung im Hintergrund) würde sonst fremde Antworten lesen. + /// "!done" mit Attributen (z. B. "ret" bei "add") wird als letzte Zeile mitgeliefert. + /// + private async Task>> SendAsync( + IReadOnlyList words, TimeSpan timeout, CancellationToken cancellationToken) + { + using var timeoutCts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); + timeoutCts.CancelAfter(timeout); + + await _gate.WaitAsync(cancellationToken); + try { - var reply = await ReadReplyAsync(cancellationToken); - if (reply.Status == RouterOsReplyStatus.Trap) - { - throw new RouterOsException(DescribeError(reply)); - } + await RouterOsWireFormat.WriteSentenceAsync(_stream, words, timeoutCts.Token); - if (reply.Status == RouterOsReplyStatus.Done) + var rows = new List>(); + string? error = null; + while (true) { - return; + var reply = await ReadReplyAsync(timeoutCts.Token); + switch (reply.Status) + { + case RouterOsReplyStatus.Row: + rows.Add(reply.Attributes); + break; + case RouterOsReplyStatus.Trap: + error ??= DescribeError(reply); + if (reply.IsFatal) + { + throw new RouterOsException(error); + } + + break; + case RouterOsReplyStatus.Done: + if (error is not null) + { + throw new RouterOsException(error); + } + + if (reply.Attributes.Count > 0) + { + rows.Add(reply.Attributes); + } + + return rows; + } } } + catch (OperationCanceledException) when (!cancellationToken.IsCancellationRequested) + { + // Nach einem Zeitlimit mitten in einer Antwort ist die Reihenfolge nicht mehr verlässlich. + _isBroken = true; + throw new RouterOsException("Das Gerät hat nicht rechtzeitig geantwortet."); + } + catch (IOException ex) + { + _isBroken = true; + throw new RouterOsException("Die Verbindung zum Gerät wurde unterbrochen.", ex); + } + finally + { + _gate.Release(); + } } private static string NormalizeCommand(string path, string action) @@ -224,7 +263,7 @@ public sealed class BinaryRouterOsClient : IRouterOsClient _ => RouterOsReplyStatus.Done, }; - return new RouterOsReply(status, attributes); + return new RouterOsReply(status, attributes) { IsFatal = replyWord == "!fatal" }; } private static string DescribeError(RouterOsReply reply) => @@ -235,7 +274,7 @@ public sealed class BinaryRouterOsClient : IRouterOsClient public ValueTask DisposeAsync() { _stream.Dispose(); - _tcpClient.Dispose(); + _tcpClient?.Dispose(); return ValueTask.CompletedTask; } } diff --git a/src/MikrotikInstaller.Core/Connectivity/Binary/RouterOsReply.cs b/src/MikrotikInstaller.Core/Connectivity/Binary/RouterOsReply.cs index 58bfa49..5c52d73 100644 --- a/src/MikrotikInstaller.Core/Connectivity/Binary/RouterOsReply.cs +++ b/src/MikrotikInstaller.Core/Connectivity/Binary/RouterOsReply.cs @@ -12,4 +12,8 @@ internal enum RouterOsReplyStatus Trap, } -internal sealed record RouterOsReply(RouterOsReplyStatus Status, IReadOnlyDictionary Attributes); +internal sealed record RouterOsReply(RouterOsReplyStatus Status, IReadOnlyDictionary Attributes) +{ + /// "!fatal": RouterOS schließt die Verbindung danach, es folgt kein "!done" mehr. + public bool IsFatal { get; init; } +} diff --git a/src/MikrotikInstaller.Core/Connectivity/IRouterOsClient.cs b/src/MikrotikInstaller.Core/Connectivity/IRouterOsClient.cs index 745b529..b784bfe 100644 --- a/src/MikrotikInstaller.Core/Connectivity/IRouterOsClient.cs +++ b/src/MikrotikInstaller.Core/Connectivity/IRouterOsClient.cs @@ -23,4 +23,15 @@ public interface IRouterOsClient : IAsyncDisposable /// Führt einen eigenständigen Befehl aus, der keinem Listen-Eintrag entspricht (z. B. "/system/backup/save"). Task ExecuteAsync(string path, IReadOnlyDictionary? parameters = null, CancellationToken cancellationToken = default); + + /// + /// Wie , liefert aber die Ausgabezeilen des Befehls zurück (z. B. "/ping" mit + /// "count", "/tool/traceroute", "/execute"). Standard für Test-Doubles: ausführen, keine Ausgabe. + /// + async Task>> RunAsync( + string path, IReadOnlyDictionary? parameters = null, CancellationToken cancellationToken = default) + { + await ExecuteAsync(path, parameters, cancellationToken); + return []; + } } diff --git a/src/MikrotikInstaller.Core/Connectivity/Rest/RestRouterOsClient.cs b/src/MikrotikInstaller.Core/Connectivity/Rest/RestRouterOsClient.cs index 6832cf2..1bd5d57 100644 --- a/src/MikrotikInstaller.Core/Connectivity/Rest/RestRouterOsClient.cs +++ b/src/MikrotikInstaller.Core/Connectivity/Rest/RestRouterOsClient.cs @@ -37,7 +37,8 @@ public sealed class RestRouterOsClient : IRouterOsClient var httpClient = new HttpClient(handler) { BaseAddress = new Uri($"{scheme}://{credentials.Host}:{port}/rest/"), - Timeout = TimeSpan.FromSeconds(8), + // Zeitlimit je Aufruf in SendAsync (siehe RouterOsTimeouts), nicht global. + Timeout = Timeout.InfiniteTimeSpan, }; httpClient.DefaultRequestHeaders.Authorization = new AuthenticationHeaderValue( "Basic", @@ -47,7 +48,7 @@ public sealed class RestRouterOsClient : IRouterOsClient try { // Dient gleichzeitig als Erreichbarkeits- und Login-Test. - await client.GetAsync("/system/resource", cancellationToken); + using var probe = await client.SendAsync(HttpMethod.Get, "/system/resource", null, RouterOsTimeouts.ConnectProbe, cancellationToken); } catch (RouterOsException) { @@ -65,9 +66,19 @@ public sealed class RestRouterOsClient : IRouterOsClient public async Task>> GetAsync(string path, CancellationToken cancellationToken = default) { - using var response = await SendAsync(HttpMethod.Get, path, body: null, cancellationToken); - await using var stream = await response.Content.ReadAsStreamAsync(cancellationToken); - using var document = await JsonDocument.ParseAsync(stream, cancellationToken: cancellationToken); + using var response = await SendAsync(HttpMethod.Get, path, body: null, RouterOsTimeouts.Read, cancellationToken); + return await ReadRowsAsync(response, cancellationToken); + } + + private static async Task>> ReadRowsAsync(HttpResponseMessage response, CancellationToken cancellationToken) + { + var content = await response.Content.ReadAsStringAsync(cancellationToken); + if (string.IsNullOrWhiteSpace(content)) + { + return []; + } + + using var document = JsonDocument.Parse(content); var results = new List>(); switch (document.RootElement.ValueKind) @@ -89,7 +100,7 @@ public sealed class RestRouterOsClient : IRouterOsClient public async Task AddAsync(string path, IReadOnlyDictionary parameters, CancellationToken cancellationToken = default) { - using var response = await SendAsync(HttpMethod.Put, path, parameters, cancellationToken); + using var response = await SendAsync(HttpMethod.Put, path, parameters, RouterOsTimeouts.Write, cancellationToken); await using var stream = await response.Content.ReadAsStreamAsync(cancellationToken); using var document = await JsonDocument.ParseAsync(stream, cancellationToken: cancellationToken); return document.RootElement.TryGetProperty(".id", out var id) ? id.GetString() ?? string.Empty : string.Empty; @@ -97,22 +108,31 @@ public sealed class RestRouterOsClient : IRouterOsClient public async Task SetAsync(string path, string id, IReadOnlyDictionary parameters, CancellationToken cancellationToken = default) { - using var response = await SendAsync(new HttpMethod("PATCH"), $"{path.TrimEnd('/')}/{id}", parameters, cancellationToken); + using var response = await SendAsync(new HttpMethod("PATCH"), $"{path.TrimEnd('/')}/{id}", parameters, RouterOsTimeouts.Write, cancellationToken); } public async Task RemoveAsync(string path, string id, CancellationToken cancellationToken = default) { - using var response = await SendAsync(HttpMethod.Delete, $"{path.TrimEnd('/')}/{id}", body: null, cancellationToken); + using var response = await SendAsync(HttpMethod.Delete, $"{path.TrimEnd('/')}/{id}", body: null, RouterOsTimeouts.Write, cancellationToken); } - public async Task ExecuteAsync(string path, IReadOnlyDictionary? parameters = null, CancellationToken cancellationToken = default) + public async Task ExecuteAsync(string path, IReadOnlyDictionary? parameters = null, CancellationToken cancellationToken = default) => + await RunAsync(path, parameters, cancellationToken); + + public async Task>> RunAsync( + string path, IReadOnlyDictionary? parameters = null, CancellationToken cancellationToken = default) { - using var response = await SendAsync(HttpMethod.Post, path, parameters, cancellationToken); + // RouterOS erwartet bei POST immer einen JSON-Körper, auch ohne Parameter. + using var response = await SendAsync(HttpMethod.Post, path, parameters ?? new Dictionary(), RouterOsTimeouts.Command, cancellationToken); + return await ReadRowsAsync(response, cancellationToken); } private async Task SendAsync( - HttpMethod method, string path, IReadOnlyDictionary? body, CancellationToken cancellationToken) + HttpMethod method, string path, IReadOnlyDictionary? body, TimeSpan timeout, CancellationToken cancellationToken) { + using var timeoutCts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); + timeoutCts.CancelAfter(timeout); + var request = new HttpRequestMessage(method, path.TrimStart('/')); if (body is not null) { @@ -122,17 +142,22 @@ public sealed class RestRouterOsClient : IRouterOsClient HttpResponseMessage response; try { - response = await _httpClient.SendAsync(request, cancellationToken); + response = await _httpClient.SendAsync(request, timeoutCts.Token); } - catch (Exception ex) + catch (OperationCanceledException ex) when (!cancellationToken.IsCancellationRequested) + { + throw new RouterOsException("Das Gerät hat nicht rechtzeitig geantwortet.", ex); + } + catch (Exception ex) when (ex is not OperationCanceledException) { throw new RouterOsException("Die Verbindung zum Gerät wurde unterbrochen.", ex); } if (!response.IsSuccessStatusCode) { + var detail = await TryReadErrorDetailAsync(response); response.Dispose(); - throw new RouterOsException(DescribeError(response.StatusCode)); + throw new RouterOsException(DescribeError(response.StatusCode, detail)); } return response; @@ -151,10 +176,25 @@ public sealed class RestRouterOsClient : IRouterOsClient return dict; } - private static string DescribeError(HttpStatusCode statusCode) => statusCode switch + /// RouterOS liefert bei Fehlern {"error":400,"message":"Bad Request","detail":"…"} — "detail" nennt den eigentlichen Grund. + private static async Task TryReadErrorDetailAsync(HttpResponseMessage response) + { + try + { + using var document = JsonDocument.Parse(await response.Content.ReadAsStringAsync()); + return document.RootElement.TryGetProperty("detail", out var detail) ? detail.GetString() : null; + } + catch (Exception ex) when (ex is JsonException or InvalidOperationException or HttpRequestException) + { + return null; + } + } + + private static string DescribeError(HttpStatusCode statusCode, string? detail) => statusCode switch { HttpStatusCode.Unauthorized or HttpStatusCode.Forbidden => "Benutzername oder Passwort ist falsch.", HttpStatusCode.NotFound => "Diese Einstellung wird von deinem Gerät nicht unterstützt.", + _ when !string.IsNullOrWhiteSpace(detail) => $"Das Gerät hat die Anfrage abgelehnt: {detail}", _ => "Das Gerät hat die Anfrage abgelehnt.", }; diff --git a/src/MikrotikInstaller.Core/Connectivity/RouterOsTimeouts.cs b/src/MikrotikInstaller.Core/Connectivity/RouterOsTimeouts.cs new file mode 100644 index 0000000..4dac8c8 --- /dev/null +++ b/src/MikrotikInstaller.Core/Connectivity/RouterOsTimeouts.cs @@ -0,0 +1,19 @@ +namespace MikrotikInstaller.Core.Connectivity; + +/// +/// Zeitlimits je Art des Zugriffs. Schwache Hardware (z. B. hAP lite) braucht für Schreibvorgänge und +/// erst recht für Backup/Restore/Update deutlich länger als für ein Auslesen — ein gemeinsames, knappes +/// Limit meldete dort Fehler, obwohl der Router den Befehl ausgeführt hatte (Mac v1.3.0/v1.6.0). +/// +public static class RouterOsTimeouts +{ + /// Erster Erreichbarkeits-/Login-Test beim Verbinden — kurz, damit ein Fallback zügig greift. + public static readonly TimeSpan ConnectProbe = TimeSpan.FromSeconds(8); + + public static readonly TimeSpan Read = TimeSpan.FromSeconds(15); + + public static readonly TimeSpan Write = TimeSpan.FromSeconds(30); + + /// Eigenständige Befehle wie Backup, Restore, Update, Ping. + public static readonly TimeSpan Command = TimeSpan.FromSeconds(120); +} diff --git a/src/MikrotikInstaller.Core/MikrotikInstaller.Core.csproj b/src/MikrotikInstaller.Core/MikrotikInstaller.Core.csproj index 0bc6e59..aecd413 100644 --- a/src/MikrotikInstaller.Core/MikrotikInstaller.Core.csproj +++ b/src/MikrotikInstaller.Core/MikrotikInstaller.Core.csproj @@ -9,6 +9,10 @@ embedded + + + +