Skip to content

fix: Delphi-POSIX WebSocket WaitReadable uses __FD_SET / Posix.SysSelect - #630

Open
freitasjca wants to merge 28 commits into
HashLoad:masterfrom
freitasjca:fix/websocket-posix-fdset
Open

freitasjca wants to merge 28 commits into
HashLoad:masterfrom
freitasjca:fix/websocket-posix-fdset

Conversation

@freitasjca

Copy link
Copy Markdown
Contributor

Summary

THorseWebSocketSocketTransport.WaitReadable fails to compile on Delphi-Linux64 (any Delphi POSIX target): E2029 in its Delphi-POSIX branch. The WebSocket transport is therefore unbuildable on Delphi-Linux64.

Root cause

In the Delphi, non-Windows ({$ELSE}) branch of WaitReadable (src/Horse.Provider.Socket.WebSocket.pas, ~line 299):

FD_ZERO(LSet);
FD_SET(ASocket, LSet);            // E2029 here
...
select(ASocket + 1, @LSet, nil, nil, @LTimeout)

On Delphi-POSIX, FD_SET is (case-insensitively) the type fd_set, not a macro — so FD_SET(ASocket, LSet) parses as a typecast and the compiler reports E2029 ')' expected but ',' found. The POSIX fd_set helpers are spelled __FD_ZERO / __FD_SET, and select needs Posix.SysSelect qualification 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

__FD_ZERO(LSet);
__FD_SET(ASocket, LSet);
...
Result := Posix.SysSelect.select(ASocket + 1, @LSet, nil, nil, @LTimeout) > 0;

This matches Delphi's own Indy POSIX stack (IdStackVCLPosix uses __FD_SET). The change is isolated to the Delphi-POSIX branch — the FPC branch (fpFD_* / fpSelect) and the Delphi-Windows branch (Winsock FD_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 of WaitReadable.

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

No deployments
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