fix: Connection issues at high throughput - #25
Conversation
47faf87 to
a43f458
Compare
lujain-aleryani
left a comment
lujain-aleryani
left a comment
There was a problem hiding this comment.
Overall: approve-in-spirit. Moving off per-call new HttpClient() to a pooled, reused handler is the right fix and directly targets the connection-timeout symptom seen under burst. A few things to address before merge (details inline):
Scope vs. claims — the PR description checks GameLift and PlayFab, but the diff only touches Agones, Edgegap, Gameye, and RocketScience. Those two modules still create a per-call HttpClient and keep the bug — either include them or update the checkboxes. Also worth noting CI only builds Agones/GameLift/Multiplay/PlayFab, so three of the four changed modules aren't compiled here.
Testing — the whole premise is behavior under burst, but only "tested locally" is checked. A small harness that drives concurrent allocate/poll and asserts connection reuse (or at least no socket exhaustion) would give us confidence. At minimum, note how the customer/RS will validate in staging.
Framing — this reduces connection cost, not request volume (still one poll per allocation). It's complementary to the poll-dedup/grouped-polling work, not a replacement — worth stating so it isn't mistaken for the batching fix.
Nice-to-have: the SharedHandler block is duplicated across three modules — fine if intentional (standalone templates), but a deliberate call rather than drift.
| var authProvider = new AnonymousAuthenticationProvider(); | ||
|
|
||
| var handler = new HttpClientHandler | ||
| var handler = new SocketsHttpHandler |
There was a problem hiding this comment.
Consistency: here the handler is a non-static local captured by the singleton adapter (effectively singleton-scoped, default disposeHandler: true), whereas Edgegap/Gameye/RocketScience use a static readonly SharedHandler with disposeHandler: false. Both work, but standardizing on one pattern makes it safer to copy into the remaining modules. Suggest the static readonly form everywhere.
Optional: SocketsHttpHandler defaults to HTTP/1.1. If providers support h2, EnableMultipleHttp2Connections = true would multiplex many requests over a few connections — a bigger win against connection exhaustion than raising MaxConnectionsPerServer.
| { | ||
| PooledConnectionLifetime = TimeSpan.FromMinutes(5), | ||
| PooledConnectionIdleTimeout = TimeSpan.FromMinutes(2), | ||
| MaxConnectionsPerServer = 300, |
There was a problem hiding this comment.
MaxConnectionsPerServer = 300 is an unexplained magic number (repeated across all modules). Under a multi-thousand burst this becomes the per-pool concurrency ceiling; requests beyond it queue and can then trip the 10s timeout. Add a one-line comment on why 300, and sanity-check it against the expected concurrent-poll count.
| public class RocketScienceHttpClientFactory : IRocketScienceHttpClientFactory | ||
| { | ||
| // Static so the connection pool outlives a single invocation; a per-call handler re-handshakes every request. | ||
| private static readonly SocketsHttpHandler SharedHandler = new() |
There was a problem hiding this comment.
Key assumption to validate: this only helps if the static handler (and the pool) actually persists across Cloud Code invocations. If CC runs each invocation in a fresh/gated worker, the pool is rebuilt every call and the reuse benefit disappears. Recommend confirming CC worker lifetime, and/or logging once on handler construction to measure reuse in practice.
| HttpClient client = httpClientFactory.Create(edgegapApiToken.Value); | ||
| HttpResponseMessage response = await client.GetAsync($"{EdgegapApiUrl}/v1/status/{requestId}"); | ||
| using HttpClient client = httpClientFactory.Create(edgegapApiToken.Value); | ||
| using HttpResponseMessage response = await client.GetAsync($"{EdgegapApiUrl}/v1/status/{requestId}"); |
There was a problem hiding this comment.
Good — using on both the client and the response. For consistency, apply the same disposal to the Gameye and RocketScience call sites (they weren't updated). Not a connection leak since the shared handler isn't disposed, but disposing the HttpResponseMessage everywhere is the right habit.
2 participants
Pull Request
Description
At high throughputs we can see connection issues where outbound connections are failing with timeouts. A likely cause of this is the lack of connection reuse on the http clients. This means that each allocate or poll requires an expensive and slow https handshake with providers server. This change ensures connections are re-used and provides a sensible timeout of 10s which should fit within the cloud code module's maximum 15s runtime.
Type of Change
Provider Integration
Related Issues
Changes Made
Testing
Security Checklist
Code Provenance
License Agreement
Documentation
Provider Terms Compliance
Additional Notes
Community Support Acknowledgment