Skip to content

fix(TCrossHttpClient): retry once on stale reused keep-alive connection - #201

Open
freitasjca wants to merge 1 commit into
winddriver:masterfrom
freitasjca:fix/keepalive-retry
Open

fix(TCrossHttpClient): retry once on stale reused keep-alive connection#201
freitasjca wants to merge 1 commit into
winddriver:masterfrom
freitasjca:fix/keepalive-retry

Conversation

@freitasjca

Copy link
Copy Markdown

Problem

TCrossHttpClient surfaces a 400 to the caller when a reused pooled connection goes stale between requests. The stale keep-alive race is inherent in connection pooling: the server closes the connection; the client's write still succeeds locally (TCP send buffer), then the next read returns an error. Every mainstream HTTP client — WinHTTP, curl, Go net/http — retries transparently.

Solution

In TCrossHttpClientConnection.TriggerResponseFailed, detect the stale-connection case and re-dispatch the request once through TServerDock.DoRequest on a fresh connection.

Gates (all required):

  • Not in Destroy teardown (FNoRetry) — avoids touching freed dock state
  • Not already retried (FRetried) — bounded to one retry per request
  • Body replayable: pointer+size or none (TBytes/TCustomMemoryStream qualify; chunk-source TStream closures do not)
  • AND one of:
    • (a) dispatch rode a reused idle connection (FViaReusedConnection) and zero response bytes received (FRespDataReceived=False) — stale keep-alive race, safe for any method including POST
    • (b) method is idempotent (GET/HEAD/PUT/DELETE/OPTIONS) — RFC 7230 §6.3.1 explicitly permits automatic retry

Design choices:

  • Forces a fresh connect on retry (TServerDock.DoRequest skips GetIdleConnection when FRetried): under concurrent bursts every idle pooled connection goes stale together, so retrying on another idle connection fails again.
  • 50 ms pause on a throwaway thread — never blocks the IO thread; an immediate retry dies in the same backend transient that killed the first attempt.

Testing

Validated against fphttpserver (which closes keep-alive connections as a group on server-side restart/reload), eliminating flaky failures on HEAD (test 07) and concurrent requests (tests 18 and 29) in a 101-check suite run with TCrossHttpClient.

When a request dispatched onto a reused pooled connection fails before
any response byte arrives, re-dispatch it once on a fresh connection
instead of surfacing a 400 to the caller.

This is the behaviour WinHTTP and curl implement. The stale keep-alive
race is inherent in connection pooling: the server may close the
connection between requests, the local write still succeeds, then the
next read fails.

Gates (all required):
  - not in Destroy teardown (FNoRetry — avoids touching freed dock state)
  - not already retried (FRetried — bounded to one retry per request)
  - body replayable: pointer+size or none; chunk-source TStream excluded
    (TBytes / TCustomMemoryStream route to pointer+size so they retry)
  - AND one of:
    (a) dispatch rode a reused idle connection (FViaReusedConnection) AND
        zero response bytes received (FRespDataReceived=False) — stale
        keep-alive race, safe for any method including POST
    (b) method is idempotent (GET/HEAD/PUT/DELETE/OPTIONS) per RFC 7230
        §6.3.1 — automatic retry is explicitly permitted regardless of
        how far the failed attempt progressed

Design choices (each validated as necessary):
  - Forces a fresh connect on retry: TServerDock.DoRequest skips
    GetIdleConnection when FRetried. Under concurrent bursts every idle
    pooled connection goes stale together (server closes them as a group;
    MaxConnsPerServer=2 funnels bursts through the pool), so retrying on
    another pooled connection failed again and exhausted the single retry.
  - 50 ms pause on a throwaway thread (TThread.CreateAnonymousThread):
    never blocks the IO thread; an immediate retry was observed to die in
    the same backend transient that killed the first attempt.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant