Repository navigation
fix: Delphi-POSIX WebSocket WaitReadable uses __FD_SET / Posix.SysSelect - #630
Open
freitasjca wants to merge 28 commits into
Open
freitasjca wants to merge 28 commits into
freitasjca wants to merge 28 commits into
Conversation
Takes upstream's Horse.Provider.FPC.HTTPApplication.pas wholesale. The conflict was entirely our inert PATCH-FPCHTTP-2: it hooked TFPCustomHttpServer's OnAllowConnect, which fphttpserver stores in its setter and reads nowhere, so the handler never fired and no setsockopt ever ran. Upstream's PR HashLoad#563 fixes the same Nagle + delayed-ACK stall with a THorseNoDelaySocketHandler.Accept override wired through OnGetSocketHandler, running once per connection rather than once per request, and additionally sets KeepConnectionTimeout - without which KeepConnections alone leaves the connection thread serving exactly one request. PATCH-FPCHTTP-1 (ListenWithConfig) is unaffected; upstream already carries it.
The test opened a fresh connection per request, because
TFPHTTPClient.KeepConnection defaults to False. The stall only occurs on a
REUSED connection - Linux starts every socket in quickack mode, so the first
ACK is immediate and Nagle never engages. It therefore passed identically with
or without the fix. Setting KeepConnection := True is what makes it a guard.
Adds a compile-time refusal below FPC 3.3.1. The provider's keep-alive and
TCP_NODELAY code sits behind that version guard, so on 3.2.2 the server closes
after each response and the run dies with 'Error reading data from socket' - a
socket error naming nothing relevant.
Rewrites the header, which asserted the disproved cause ('a fixed ~40 ms
select() poll interval'). The real cause is Nagle interacting with the peer's
delayed ACK, established by wire capture: the body segment leaves 11-14 us
after the ACK, across an ACK delay that itself varies 42.5-43.9 ms.
Verified against 3.3.4 on FPC trunk: 30 requests on one connection, max 1 ms.
Dockerfile.tests-lazarus adds fp-units-web, which this test needs for
fphttpclient. .gitignore covers both FPC test binaries: their Linux builds have
no extension, so the existing *.exe rule does not catch them.
Standalone FPC program plus runner, covering the FIX-WS-NONBLOCK path. Horse's own suite is DUnitX and Delphi-only, so the FPC + epoll transport has no way to be tested from it; this was written for the review of PR HashLoad#549 and kept after the maintainer reworked that fix rather than merging ours. Verified against the reworked implementation now in 3.3.4 - errno captured once and passed to both predicates, close deferred via FStateLock plus an FActiveOperations refcount - which is code this test was not written for. 5 checks, all passing on FPC 3.2.2. The load-bearing assertion is SILENCE, not liveness. epoll sets O_NONBLOCK on accepted sockets, so recv returns -1/EAGAIN whenever no frame has arrived yet; the original defect read that as a disconnect, broke the upgrader's read loop on its first iteration and let the HTTP pipeline resume, writing a stray 'HTTP/1.1 200 OK' onto a socket already handed to WebSocket. An earlier draft asserted only that the connection was still open, and passed against the broken code. The runner refuses to build a tree lacking WS_SOCKET_READ_TICK_MS, so it cannot report a pass on a checkout that predates the fix. Exit code is the number of failed checks, so it gates directly.
…s image Our version of Dockerfile.tests-lazarus deleted upstream's WebSocketEpollRegression build-and-run step and replaced it with a build of FPCHttpKeepaliveTest, so the Lazarus test image silently stopped covering the non-blocking WebSocket transport. The replacement comment also restated the disproved cause of the keep-alive stall - a fixed ~40 ms select() poll interval - superseded by the Nagle/delayed-ACK finding in horse#562. Reverted to upstream's version outright rather than patched, because the rest of our change is no longer wanted either: fp-units-web was added only for that keep-alive build step, and nothing else in the image uses fphttpclient. This file is now byte-identical to upstream, which is one less thing to reconcile at the next sync. The keep-alive test is deliberately NOT re-added here. It now refuses to build below FPC 3.3.1, where the provider's keep-alive and TCP_NODELAY code is not compiled at all, and this image installs Ubuntu 22.04's fpc 3.2.2 - so adding it would fail the image build. It runs standalone against trunk instead.
# Conflicts: # .gitignore # tests/fpc-websocket/WebSocketEagainRegression.lpr # tests/fpc-websocket/run-ws-eagain-test.sh
…elect THorseWebSocketSocketTransport.WaitReadable did not compile on Delphi-Linux64: in its Delphi non-Windows branch, FD_SET is (case-insensitively) the type fd_set, so FD_SET(ASocket, LSet) parses as a typecast - E2029. The POSIX fd_set helpers are __FD_ZERO / __FD_SET, and select needs Posix.SysSelect qualification. Fix (Delphi-POSIX branch only): __FD_ZERO / __FD_SET + Posix.SysSelect.select. Matches Delphi's Indy POSIX stack (IdStackVCLPosix). The FPC branch (fpFD_* / fpSelect) and the Delphi-Windows branch (Winsock FD_SET) are untouched. Compile-only fix - no functional test can reach it (the WebSocket tests run on platforms where the old code already compiled); the regression guard is a Delphi-Linux64 compile job in CI. Files: src/Horse.Provider.Socket.WebSocket.pas
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
THorseWebSocketSocketTransport.WaitReadablefails to compile on Delphi-Linux64 (any Delphi POSIX target):E2029in its Delphi-POSIX branch. The WebSocket transport is therefore unbuildable on Delphi-Linux64.Root cause
In the Delphi, non-Windows (
{$ELSE}) branch ofWaitReadable(src/Horse.Provider.Socket.WebSocket.pas, ~line 299):On Delphi-POSIX,
FD_SETis (case-insensitively) the typefd_set, not a macro — soFD_SET(ASocket, LSet)parses as a typecast and the compiler reportsE2029 ')' expected but ',' found. The POSIX fd_set helpers are spelled__FD_ZERO/__FD_SET, andselectneedsPosix.SysSelectqualification to disambiguate.This branch had not been compiled before: the WebSocket CI (PR #549) built FPC-Linux + Delphi-Windows only, and the Delphi-POSIX
{$ELSE}is reached only on a Delphi POSIX target. It surfaced building the Epoll sample for Delphi-Linux64.Fix
This matches Delphi's own Indy POSIX stack (
IdStackVCLPosixuses__FD_SET). The change is isolated to the Delphi-POSIX branch — the FPC branch (fpFD_*/fpSelect) and the Delphi-Windows branch (WinsockFD_SET) are untouched.On testing — an honest note
This is a compile-time fix, and a functional/unit test cannot guard it: the WebSocket integration tests run on Windows and FPC-Linux, where the pre-fix code already compiled — the defect is reachable only when the unit is compiled for a Delphi POSIX target, so a runtime test would pass identically on broken and fixed code.
The appropriate regression guard is a CI job that compiles the WebSocket transport for Delphi-Linux64. If that target isn't in the current matrix, I'm happy to add a compile-only job in a follow-up — it's the only thing that would have caught this, and it would catch the same class of Delphi-POSIX issue elsewhere.
Files
src/Horse.Provider.Socket.WebSocket.pas— Delphi-POSIX branch ofWaitReadable.