fix(net): bound every request with a timeout
A server that stops answering without closing the socket left a running sync waiting forever. Requests now fail with TimeoutException after 30 s (10 s for the handshake, both configurable in ConnectAsync) and the connection is marked dead so the app reconnects. See ADR 0034.
This commit is contained in:
1 parent
cb551efce4
commit
3be761d97e
5 files changed
+147
-6
No files matched your search
@@ -9,6 +9,10 @@ release and covers the full history from the initial commit.
|
||||
|
||||
### Fixed
|
||||
|
||||
- `ElectrumClient`: every request is now bounded (30 s, 10 s for the handshake; configurable in
|
||||
`ConnectAsync`). A server that stops answering without closing the socket made a running sync
|
||||
wait forever; now the request fails with `TimeoutException`, the connection is marked dead
|
||||
(`Disconnected` is raised, pending and later requests fail fast) and the app reconnects.
|
||||
- Fee rate parsing no longer depends on the system culture: on Italian/German systems `1.5`
|
||||
was read as `15` sat/vB (10x the intended fee). The Send form, the Donate form and the CLI
|
||||
`--feerate` now share `CoinAmount.TryParseFeeRate` (invariant culture, `.` or `,` as decimal
|
||||
|
||||
@@ -0,0 +1,28 @@
|
||||
# 0034. Bound every network request with a timeout
|
||||
|
||||
- **Status:** Accepted
|
||||
- **Area:** Network
|
||||
- **Recorded:** 2026-11-03
|
||||
|
||||
## Context
|
||||
|
||||
`ElectrumClient.RequestAsync` waited until the server answered or the caller cancelled. The only timeout in the stack was the 8 s ping of the ViewModel keep-alive, which does not run while a sync is in progress. A server that stops answering without closing the socket (half-open TCP, Android Doze, a stalled or hostile server) left a running sync waiting forever, with no error and no automatic recovery.
|
||||
|
||||
## Decision
|
||||
|
||||
- Every request has a timeout, `ElectrumClient.DefaultRequestTimeout` (30 s). The `server.version` handshake has its own, shorter `DefaultHandshakeTimeout` (10 s). Both are parameters of `ConnectAsync` (`requestTimeout`, `handshakeTimeout`), so tests use a few hundred milliseconds.
|
||||
- The timer starts when the request obtains its in-flight slot: waiting behind the cap (ADR 0009) does not count.
|
||||
- On expiry the request fails with `TimeoutException` (naming the method and server) and the **whole connection is marked dead**: the other pending requests fail, `IsConnected` becomes false, `Disconnected` is raised and later requests fail fast with `IOException`. After a missed answer the socket cannot be trusted, and the ViewModel reconnects through the existing path. A late reply is dropped because the request is no longer pending.
|
||||
- The keep-alive is left as it is (it skips ticks while syncing): the request timeout now bounds a stalled sync, so a second mechanism is not needed.
|
||||
|
||||
## Consequences
|
||||
|
||||
- A stalled server makes a sync fail within the timeout instead of hanging; the existing retry and reconnect logic takes over.
|
||||
- A legitimately slow reply above 30 s is treated as a dead connection. The largest realistic replies (header batches, large transactions, busy-address histories) are far below that on a working link; the value is a constructor parameter if a deployment needs more.
|
||||
- One slow request tears down the shared connection and the requests in flight with it. This is deliberate: it is cheaper than a half-open socket that keeps accepting writes.
|
||||
|
||||
## References
|
||||
|
||||
- `src/Core/Net/ElectrumClient.cs`
|
||||
- `tests/PalladiumWallet.Tests/Net/ElectrumClientTests.cs`
|
||||
- Related: 0009
|
||||
@@ -37,6 +37,7 @@ One ADR per feature or significant design decision. Format: `template.md`. Workf
|
||||
| 0031 | [Explicitly defer multisig, Lightning, Tor and hardware wallets](0031-defer-multisig-lightning-and-hardware-wallets.md) | Scope | Accepted |
|
||||
| 0032 | [Run tests, fuzz smoke and docs checks in GitHub Actions](0032-github-actions-ci.md) | Testing | Accepted |
|
||||
| 0033 | [Test the UI layer and CLI headlessly, gate coverage](0033-test-strategy-beyond-core.md) | Testing | Accepted |
|
||||
| 0034 | [Bound every network request with a timeout](0034-bound-every-network-request.md) | Network | Accepted |
|
||||
|
||||
ADRs `0002` onwards were written retroactively on 2026-11-03 from the code, comments and changelog.
|
||||
If one misstates the original reasoning, fix it in a pull request.
|
||||
@@ -41,6 +41,17 @@ public sealed class ElectrumClient : IAsyncDisposable
|
||||
// concurrent requests before throttling — no single constant is right for
|
||||
// every server, so callers may raise/lower it instead of recompiling.
|
||||
public const int DefaultMaxInFlight = 32;
|
||||
|
||||
// Every request is bounded: a server that stops answering without closing the socket
|
||||
// (half-open TCP, Android doze, a stalled or hostile server) would otherwise leave the
|
||||
// caller waiting forever. The timer starts once the request holds an in-flight slot, so
|
||||
// queueing behind the cap does not count. On expiry the request fails with
|
||||
// TimeoutException and the whole connection is marked dead (pending requests fail,
|
||||
// Disconnected is raised): after a missed answer the socket cannot be trusted, and a
|
||||
// late reply is dropped because the request is no longer pending.
|
||||
public static readonly TimeSpan DefaultRequestTimeout = TimeSpan.FromSeconds(30);
|
||||
public static readonly TimeSpan DefaultHandshakeTimeout = TimeSpan.FromSeconds(10);
|
||||
private readonly TimeSpan _requestTimeout;
|
||||
private readonly SemaphoreSlim _inFlight;
|
||||
|
||||
private long _nextId;
|
||||
@@ -54,8 +65,9 @@ public sealed class ElectrumClient : IAsyncDisposable
|
||||
public event Action<Exception?>? Disconnected;
|
||||
|
||||
private ElectrumClient(TcpClient tcp, Stream stream, string host, int port, bool useSsl,
|
||||
int maxInFlight)
|
||||
int maxInFlight, TimeSpan requestTimeout)
|
||||
{
|
||||
_requestTimeout = requestTimeout;
|
||||
_tcp = tcp;
|
||||
_stream = stream;
|
||||
Host = host;
|
||||
@@ -68,7 +80,8 @@ public sealed class ElectrumClient : IAsyncDisposable
|
||||
|
||||
public static async Task<ElectrumClient> ConnectAsync(string host, int port, bool useSsl,
|
||||
CertificatePinStore? pins = null, CancellationToken ct = default,
|
||||
int maxInFlight = DefaultMaxInFlight)
|
||||
int maxInFlight = DefaultMaxInFlight, TimeSpan? requestTimeout = null,
|
||||
TimeSpan? handshakeTimeout = null)
|
||||
{
|
||||
var tcp = new TcpClient { NoDelay = true };
|
||||
try
|
||||
@@ -97,8 +110,10 @@ public sealed class ElectrumClient : IAsyncDisposable
|
||||
stream = ssl;
|
||||
}
|
||||
|
||||
var client = new ElectrumClient(tcp, stream, host, port, useSsl, maxInFlight);
|
||||
await client.RequestAsync("server.version", ct, ClientName, ProtocolVersion);
|
||||
var client = new ElectrumClient(tcp, stream, host, port, useSsl, maxInFlight,
|
||||
requestTimeout ?? DefaultRequestTimeout);
|
||||
await client.RequestCoreAsync(handshakeTimeout ?? DefaultHandshakeTimeout,
|
||||
"server.version", ct, [ClientName, ProtocolVersion]);
|
||||
return client;
|
||||
}
|
||||
catch
|
||||
@@ -108,8 +123,12 @@ public sealed class ElectrumClient : IAsyncDisposable
|
||||
}
|
||||
}
|
||||
|
||||
public async Task<JsonElement> RequestAsync(string method, CancellationToken ct = default,
|
||||
params object?[] parameters)
|
||||
public Task<JsonElement> RequestAsync(string method, CancellationToken ct = default,
|
||||
params object?[] parameters) =>
|
||||
RequestCoreAsync(_requestTimeout, method, ct, parameters);
|
||||
|
||||
private async Task<JsonElement> RequestCoreAsync(TimeSpan timeout, string method,
|
||||
CancellationToken ct, object?[] parameters)
|
||||
{
|
||||
// Gate before going in-flight: beyond MaxInFlight pending requests
|
||||
// we wait for a response to free a slot, instead of flooding the server.
|
||||
@@ -144,6 +163,15 @@ public sealed class ElectrumClient : IAsyncDisposable
|
||||
_pending.TryRemove(id, out _);
|
||||
tcs.TrySetCanceled(ct);
|
||||
});
|
||||
|
||||
using var timeoutCts = new CancellationTokenSource(timeout);
|
||||
await using var timeoutRegistration = timeoutCts.Token.Register(() =>
|
||||
{
|
||||
if (!_pending.TryRemove(id, out _)) return;
|
||||
tcs.TrySetException(new TimeoutException(
|
||||
$"No response to '{method}' from {Host}:{Port} within {timeout.TotalSeconds:0.#}s."));
|
||||
try { _cts.Cancel(); } catch (ObjectDisposedException) { }
|
||||
});
|
||||
return await tcs.Task;
|
||||
}
|
||||
finally
|
||||
|
||||
@@ -147,6 +147,86 @@ public class ElectrumClientTests
|
||||
Assert.Equal(big, result);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task A_request_without_an_answer_fails_with_TimeoutException()
|
||||
{
|
||||
await using var server = new FakeElectrumServer();
|
||||
server.Handle("server.banner", _ => FakeElectrumServer.NoResponse);
|
||||
await using var client = await ElectrumClient.ConnectAsync(server.Host, server.Port,
|
||||
useSsl: false, requestTimeout: TimeSpan.FromMilliseconds(300));
|
||||
|
||||
var ex = await Assert.ThrowsAsync<TimeoutException>(() => client.BannerAsync().WaitAsync(Timeout));
|
||||
Assert.Contains("server.banner", ex.Message);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task After_a_timeout_the_client_is_disconnected_and_later_requests_fail_fast()
|
||||
{
|
||||
await using var server = new FakeElectrumServer();
|
||||
server.Handle("server.banner", _ => FakeElectrumServer.NoResponse);
|
||||
await using var client = await ElectrumClient.ConnectAsync(server.Host, server.Port,
|
||||
useSsl: false, requestTimeout: TimeSpan.FromMilliseconds(300));
|
||||
var disconnected = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously);
|
||||
client.Disconnected += _ => disconnected.TrySetResult();
|
||||
|
||||
await Assert.ThrowsAsync<TimeoutException>(() => client.BannerAsync().WaitAsync(Timeout));
|
||||
await disconnected.Task.WaitAsync(Timeout);
|
||||
|
||||
Assert.False(client.IsConnected);
|
||||
await Assert.ThrowsAsync<IOException>(() => client.BannerAsync().WaitAsync(Timeout));
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task A_timeout_fails_the_other_pending_requests_too()
|
||||
{
|
||||
await using var server = new FakeElectrumServer();
|
||||
server.Handle("server.banner", _ => FakeElectrumServer.NoResponse);
|
||||
await using var client = await ElectrumClient.ConnectAsync(server.Host, server.Port,
|
||||
useSsl: false, requestTimeout: TimeSpan.FromMilliseconds(300));
|
||||
|
||||
var a = client.BannerAsync();
|
||||
var b = client.BannerAsync();
|
||||
await Assert.ThrowsAnyAsync<Exception>(() => a.WaitAsync(Timeout));
|
||||
await Assert.ThrowsAnyAsync<Exception>(() => b.WaitAsync(Timeout));
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task A_slow_answer_within_the_timeout_still_succeeds()
|
||||
{
|
||||
await using var server = new FakeElectrumServer();
|
||||
server.Handle("server.banner", _ => { Thread.Sleep(200); return "hello"; });
|
||||
await using var client = await ElectrumClient.ConnectAsync(server.Host, server.Port,
|
||||
useSsl: false, requestTimeout: TimeSpan.FromSeconds(10));
|
||||
|
||||
Assert.Equal("hello", await client.BannerAsync().WaitAsync(Timeout));
|
||||
Assert.True(client.IsConnected);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task A_handshake_that_never_completes_times_out()
|
||||
{
|
||||
await using var server = new FakeElectrumServer();
|
||||
server.Handle("server.version", _ => FakeElectrumServer.NoResponse);
|
||||
|
||||
await Assert.ThrowsAsync<TimeoutException>(() => ElectrumClient.ConnectAsync(
|
||||
server.Host, server.Port, useSsl: false,
|
||||
handshakeTimeout: TimeSpan.FromMilliseconds(300)).WaitAsync(Timeout));
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task Waiting_for_an_in_flight_slot_does_not_count_towards_the_timeout()
|
||||
{
|
||||
await using var server = new FakeElectrumServer();
|
||||
server.Handle("server.banner", _ => { Thread.Sleep(250); return "ok"; });
|
||||
await using var client = await ElectrumClient.ConnectAsync(server.Host, server.Port,
|
||||
useSsl: false, maxInFlight: 1, requestTimeout: TimeSpan.FromMilliseconds(600));
|
||||
|
||||
// Three serial requests of ~250 ms each: the last one waits ~500 ms for the slot.
|
||||
var all = await Task.WhenAll(client.BannerAsync(), client.BannerAsync(), client.BannerAsync())
|
||||
.WaitAsync(Timeout);
|
||||
Assert.All(all, b => Assert.Equal("ok", b));
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task La_cancellazione_del_token_annulla_la_richiesta_pendente()
|
||||
{
|
||||
|
||||
Reference in new issue
Block a user