Always let the OS auto-tune the socket send/receive buffer size - #1821
Always let the OS auto-tune the socket send/receive buffer size#1821cloud-hai-vo wants to merge 4 commits into
Conversation
SocketConnect/SocketConnectAsync hardcode the underlying socket's send/receive buffer size as a small multiple of the SSH packet size constant, unrelated to network conditions. On links with a non-trivial bandwidth-delay product this caps throughput well below the link's actual capacity, and prevents the OS from auto-tuning the buffer. Add ConnectionInfo.SocketBufferSize (default null, unchanged behavior) and the AutoTuneSocketBufferSize sentinel to opt out of setting the buffer explicitly and let the OS auto-tune it.
The OS may clamp and/or adjust an explicitly requested socket buffer size (e.g. against net.core.wmem_max/rmem_max on Linux, which also doubles the value for internal bookkeeping), so asserting an exact byte value after connecting is not portable across platforms - this failed CI on Linux while passing on Windows. Assert the resulting buffer size is larger than an untouched control socket's default instead, which holds on both platforms and still proves the code path took effect.
|
I did not realise the async setup differs to the sync path. I would take a quick fix to align async with sync.
If this is true then perhaps best is to delete this stuff, but it will take longer to merge since it needs some testing Either way I don't want to add a setting to control it since it locks us in to this approach. Eventually I would like to allow a more general way to set up the transport, like HttpClient does. But "eventually" takes some emphasis here |
Per review feedback, drop the ConnectionInfo.SocketBufferSize opt-in property in favor of simply removing the explicit SendBufferSize/ ReceiveBufferSize assignment from both SocketConnect and SocketConnectAsync entirely. This lets the operating system auto-tune the buffer for every connection by default, rather than requiring callers to opt in - and resolves the sync/async multiplier mismatch Rob flagged, since neither path sets an explicit value anymore. The evidence in the PR description (auto-tuning outperforming both manual tuning and a reference client) already supports this as the right default, not just an opt-in.
|
@Rob-Hague Makes sense on all three points — updated:
I also have a follow-up idea along the |
Problem
ConnectorBase.SocketConnect/SocketConnectAsynchardcode the socket's send/receive buffer size as a small multiple ofSession.MaximumSshPacketSize— a protocol framing constant with no relationship to network throughput, and the two paths even disagree with each other (10×sync vs2×async):On any connection with a non-trivial bandwidth-delay product (BDP = bandwidth × RTT larger than the buffer), this caps throughput at
buffer_size / RTT, regardless of the link's actual capacity. Setting an explicit buffer size also disables the OS's own TCP auto-tuning for that connection, which would otherwise size the buffer based on observed conditions.Measured on a real link (AWS
ap-southeast-1→ AWS Transfer Family inus-west-2/eu-west-2, ~168–174 ms RTT per AWS's own published inter-region latency):SftpClientwas capped at ~5–7 Mbps regardless of the underlying connection's actual capacity, confirmed independently via a reference SFTP client on the same link reaching 50+ Mbps. This math also happens to land almost exactly on the observed number:134 KB / 0.17s ≈ 6.3 Mbps.Change
Simply removes the explicit
SendBufferSize/ReceiveBufferSizeassignment from bothSocketConnectandSocketConnectAsync. No new configuration surface — this always lets the OS auto-tune, for every connection, and resolves the sync/async mismatch as a side effect since neither path sets an explicit value anymore.Evidence
Live A/B test on the same link/session, uploading a 140 MB file:
develop(unchanged before this PR)Auto-tuning outperformed both manually-tuned fixed values and the reference client — this isn't just "as good as" picking a bigger number, letting the OS decide is measurably the better default.
Testing
Diff is now 6 lines removed from
ConnectorBase.cs, nothing else. Full existing suite passes locally (one unrelated pre-existing flaky test,SessionTest_Connecting_ServerNotResetSequenceNumberAfterNewKeys_StrictKex.ShouldThrowSshConnectionException, fails identically on unmodifieddevelopwhen run as part of the full suite — confirmed not related to this change).Previous revision of this PR added an opt-in
ConnectionInfo.SocketBufferSizeproperty — per @Rob-Hague's feedback that a narrow setting locks the library into this approach ahead of a more general transport configuration mechanism, this revision drops that entirely and just removes the hardcoded override, which also resolves the sync/async alignment point raised.