Skip to content

fix(fpc): remove keepalive — fphttpserver poll interval causes ~40ms per-request stall - #554

Open
freitasjca wants to merge 7 commits into
HashLoad:masterfrom
freitasjca:fix/fpchttp-keepalive-stall
Open

fix(fpc): remove keepalive — fphttpserver poll interval causes ~40ms per-request stall#554
freitasjca wants to merge 7 commits into
HashLoad:masterfrom
freitasjca:fix/fpchttp-keepalive-stall

Conversation

@freitasjca

Copy link
Copy Markdown
Contributor

Summary

FPC-KEEPALIVE-1 enabled KeepConnections=True on the embedded fphttpserver to restore HTTP/1.1 keep-alive semantics. This produces a ~40 ms stall on every request — a 93× regression on Linux affecting every FPC user running Horse with the default provider.

Root cause

fphttpserver's TFPHTTPConnectionThread keep-alive loop calls select(fd, ~40 ms) between requests to poll for graceful-shutdown signals. Even when the next request is already queued, the loop waits one full interval. This is a fixed constant in fphttpserver — it cannot be configured from outside.

Evidence

Measured with h2load -n 50000 -c 1 on Linux (FPC trunk 3.3.1):

Condition P50 latency req/s
Before (keepalive on) 44 ms 0 (bench timeout)
After (keepalive off) 0.48 ms 1 459
Raw fphttpserver (no Horse) 0.41 ms 1 531

TCP_NODELAY was applied simultaneously and had zero effect, confirming the cause is the poll interval, not Nagle/delayed-ACK.

What this PR does

  1. Removes EnableServerKeepAliveKeepConnections reverts to False (fphttpserver default).
  2. Adds EnableServerNoDelay (PATCH-FPCHTTP-2) — sets TCP_NODELAY on every accepted socket via TSocketServer.OnAllowConnect. Avoids Nagle on non-loopback links. Guard: FPC ≥ 3.3.1, UNIX only.
  3. Adds FPCHttpKeepaliveTest.dpr — standalone FPC regression test: 30 sequential requests must complete in ≤ 35 ms each.

Trade-off

With KeepConnections=False, clients that reuse a connection after the server closes it receive ECONNRESET. Most clients (curl, browsers, System.Net.HttpClient) reconnect transparently. Connection pools that do not retry stale connections should be configured to do so, or to disable keep-alive with this provider.

@regyssilveira

Copy link
Copy Markdown
Contributor

Obrigado pelos dados de desempenho e pela investigação do intervalo de aproximadamente 40 ms. Este PR ainda não está pronto para revisão porque mistura duas linhas de trabalho diferentes e altera a semântica HTTP/1.1 do provider padrão.

Antes de prosseguirmos, por favor:

  1. Rebaseie sobre o master atual e remova todas as alterações relacionadas ao nghttp2. Este PR deve alterar somente Horse.Provider.FPC.HTTPApplication.pas, o teste específico e, se necessário, documentação diretamente relacionada ao keep-alive do fphttpserver.
  2. Remova do histórico/diff as mudanças em Horse.pas, README.md e README.pt-BR.md que pertencem ao antigo PR de nghttp2.
  3. Não desabilite keep-alive de forma incondicional. Isso evita o atraso, mas faz o servidor encerrar a conexão após cada resposta e pode causar ECONNRESET em clientes que reutilizam conexões. Prefira uma solução configurável, preservando a semântica HTTP/1.1, ou uma correção no mecanismo de espera do fphttpserver.
  4. Explique como a resposta informa ao cliente que a conexão será encerrada. Se o servidor fechar sem Connection: close, o cliente pode tentar reutilizar um socket que parece persistente.
  5. Substitua o limite absoluto de 35 ms do teste por uma verificação determinística ou comparativa. Esse limite pode falhar apenas por carga do runner, virtualização ou agendamento do sistema operacional.
  6. Remova código morto após a solução final. No diff atual, EnableServerKeepAlive e DEFAULT_KEEPALIVE_TIMEOUT_MS continuam declarados/implementados apesar de não serem mais usados.
  7. Separe o uso de TCP_NODELAY da correção do keep-alive ou demonstre por teste que ele é necessário. A própria descrição conclui que TCP_NODELAY não resolveu a regressão de 40 ms.

Depois da limpeza, precisamos reavaliar o trade-off entre latência, persistência HTTP/1.1 e compatibilidade dos clientes antes de aprovar.

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.

2 participants