mirror of
https://github.com/GoodOlClint/PSProxmoxVE.git
synced 2026-08-12 15:16:52 +00:00
fix: surface HttpClient timeouts as PveApiException(RequestTimeout)
When HttpClient.Timeout elapses, .NET throws TaskCanceledException — not HttpRequestException — so the existing catch in PveHttpClient.SendAsync missed it and callers got a raw stack trace. With -TimeoutSeconds now configurable and documented, this gap became user-visible. In .NET 5+ HttpClient surfaces transport timeouts as TaskCanceledException with a TimeoutException inner; user-driven token cancellation does not. Catch by that inner-type signature and rethrow as PveApiException with HttpStatusCode.RequestTimeout, the resource path, and a message that reports the configured timeout. Adds SendAsync_TimeoutFires_ThrowsPveApiExceptionWithRequestTimeout which swaps in a delaying HttpMessageHandler with a 50ms timeout to exercise the path deterministically. Drops the redundant DefaultSessionTimeoutIs100Seconds test (covered by PveSessionTests.Timeout_DefaultIs100Seconds and the existing flow-through test). Addresses PR #61 review feedback. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -353,6 +353,17 @@ namespace PSProxmoxVE.Core.Client
|
||||
{
|
||||
response = await _httpClient.SendAsync(request).ConfigureAwait(false);
|
||||
}
|
||||
catch (TaskCanceledException ex) when (ex.InnerException is TimeoutException)
|
||||
{
|
||||
// HttpClient.Timeout elapsed. In .NET 5+, HttpClient surfaces transport
|
||||
// timeouts as TaskCanceledException with a TimeoutException inner —
|
||||
// distinguishing them from caller-driven CancellationToken cancellation.
|
||||
var seconds = _httpClient.Timeout == System.Threading.Timeout.InfiniteTimeSpan
|
||||
? "infinite"
|
||||
: _httpClient.Timeout.TotalSeconds.ToString("0", System.Globalization.CultureInfo.InvariantCulture) + "s";
|
||||
throw new PveApiException(HttpStatusCode.RequestTimeout,
|
||||
$"Request timed out after {seconds}.", resource, httpMethod, ex);
|
||||
}
|
||||
catch (HttpRequestException ex)
|
||||
{
|
||||
throw new PveApiException(HttpStatusCode.ServiceUnavailable,
|
||||
|
||||
@@ -1,9 +1,12 @@
|
||||
using System;
|
||||
using System.Net;
|
||||
using System.Net.Http;
|
||||
using System.Reflection;
|
||||
using System.Threading;
|
||||
using System.Threading.Tasks;
|
||||
using PSProxmoxVE.Core.Authentication;
|
||||
using PSProxmoxVE.Core.Client;
|
||||
using PSProxmoxVE.Core.Exceptions;
|
||||
using Xunit;
|
||||
|
||||
namespace PSProxmoxVE.Core.Tests.Client
|
||||
@@ -17,6 +20,14 @@ namespace PSProxmoxVE.Core.Tests.Client
|
||||
return (HttpClient)field.GetValue(client)!;
|
||||
}
|
||||
|
||||
private static void SetInnerHttpClient(PveHttpClient client, HttpClient newInner)
|
||||
{
|
||||
var field = typeof(PveHttpClient).GetField("_httpClient",
|
||||
BindingFlags.Instance | BindingFlags.NonPublic)!;
|
||||
((HttpClient)field.GetValue(client)!).Dispose();
|
||||
field.SetValue(client, newInner);
|
||||
}
|
||||
|
||||
private static PveSession NewSession()
|
||||
{
|
||||
return new PveSession("pve.example.com", 8006, false,
|
||||
@@ -56,13 +67,35 @@ namespace PSProxmoxVE.Core.Tests.Client
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void DefaultSessionTimeoutIs100Seconds()
|
||||
public async Task SendAsync_TimeoutFires_ThrowsPveApiExceptionWithRequestTimeout()
|
||||
{
|
||||
var session = NewSession();
|
||||
|
||||
using var client = new PveHttpClient(session);
|
||||
|
||||
Assert.Equal(TimeSpan.FromSeconds(100), GetInnerHttpClient(client).Timeout);
|
||||
// Swap in an HttpClient with a delaying handler and a 50ms timeout so
|
||||
// HttpClient.Timeout fires reliably without any real network.
|
||||
var delayingClient = new HttpClient(new DelayingHandler(TimeSpan.FromSeconds(30)))
|
||||
{
|
||||
Timeout = TimeSpan.FromMilliseconds(50)
|
||||
};
|
||||
SetInnerHttpClient(client, delayingClient);
|
||||
|
||||
var ex = await Assert.ThrowsAsync<PveApiException>(() => client.GetAsync("version"));
|
||||
Assert.Equal(HttpStatusCode.RequestTimeout, ex.StatusCode);
|
||||
Assert.Contains("timed out", ex.Message, StringComparison.OrdinalIgnoreCase);
|
||||
}
|
||||
|
||||
private sealed class DelayingHandler : HttpMessageHandler
|
||||
{
|
||||
private readonly TimeSpan _delay;
|
||||
public DelayingHandler(TimeSpan delay) { _delay = delay; }
|
||||
|
||||
protected override async Task<HttpResponseMessage> SendAsync(
|
||||
HttpRequestMessage request, CancellationToken cancellationToken)
|
||||
{
|
||||
await Task.Delay(_delay, cancellationToken).ConfigureAwait(false);
|
||||
return new HttpResponseMessage(HttpStatusCode.OK);
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user