generic_unix: use a graceful close for TCP server sockets#2376
Conversation
Fix a behavior with init_server_tcp_socket that surfaced in FreeBSD CI failures. Signed-off-by: Paul Guyot <pguyot@kallisys.net>
petermm
left a comment
There was a problem hiding this comment.
PR Review: graceful close for generic Unix TCP server sockets
Commit reviewed: 7e9208c15c73e32e4d6c5df32bcc901e7df167a9 (generic_unix: use a graceful close for TCP server sockets)
Verdict
Approve. No blocking or non-blocking correctness findings.
Removing SO_LINGER = {on, 0} from the listening socket is the smallest correct fix. Accepted sockets inherit the listener's linger policy on the supported Unix implementations, so the old setting made close(2) abort the connection with RST and allowed bytes already accepted by send(2) to be discarded before acknowledgement. Leaving linger at its default disabled state lets close(2) return promptly while the kernel completes the normal queued-data/FIN sequence.
SO_REUSEADDR remains enabled, so listener restart behavior is preserved. Orderly closes may leave accepted connections in TIME_WAIT; that is expected TCP behavior and does not put the listening socket itself in TIME_WAIT.
Findings
No actionable findings.
The relevant end-to-end regression coverage already exists in tests/libs/eavmlib/test_http_server.erl: the server sends a chunked response and immediately closes the accepted socket, while the client asserts that the complete <<"\"hello">> body arrived before observing closed. This is the path that exposed the failure on FreeBSD. Making ACK timing deterministic would require platform-sensitive socket-buffer or packet-control machinery and is not justified for this focused fix.
Optional suggestion: document why the full-body assertion matters
This is not required for merge, but the existing test comment could identify the socket-driver regression it protects. That would make it less likely for a future cleanup to weaken the full-body assertion.
diff --git a/tests/libs/eavmlib/test_http_server.erl b/tests/libs/eavmlib/test_http_server.erl
--- a/tests/libs/eavmlib/test_http_server.erl
+++ b/tests/libs/eavmlib/test_http_server.erl
@@ -49,7 +49,8 @@ handle_req("GET", [], Conn) ->
Body = [34, <<"hello">>],
http_server:reply(200, Body, Conn).
-% http_server doesn't send Content-Length, so ahttp_client doesn't know when it's done and reports closed connection
+% The response is delimited by close because http_server sends no Content-Length.
+% Keep the full-body assertion: abortive close can truncate queued reply bytes.
loop_passive(Conn, AccResp) ->
case ahttp_client:recv(Conn, 0) ofVerification
cmake --build build --target AtomVM -j4— passed.cmake --build build --target test_eavmlib -j4— passed.build/src/AtomVM build/tests/libs/eavmlib/test_eavmlib.avm— passed; all eight test modules, includingtest_http_server, returnedok.clang-format --dry-run --Werror src/platforms/generic_unix/lib/socket_driver.c— passed using the available formatter.git diff --check HEAD^ HEAD— passed.
The test run printed Unable to open test_mdns_tests.beam before running test_mdns's embedded checks; its 21 checks passed and the complete suite returned ok.
Oracle review
Oracle independently reviewed the commit's Unix socket semantics, accepted-socket option inheritance, nonblocking close behavior, TIME_WAIT implications, and existing HTTP regression coverage. It also recommended merging as-is and reported no correctness findings. Oracle's remaining assumptions were that CI exercises this legacy gen_tcp_inet backend on FreeBSD and that no callers intentionally relied on abortive close; both are consistent with the changed path, the observed FreeBSD failure, and the original comment describing the setting as “disable linger.”
Fix a behavior with init_server_tcp_socket that surfaced in FreeBSD CI failures.
These changes are made under both the "Apache 2.0" and the "GNU Lesser General
Public License 2.1 or later" license terms (dual license).
SPDX-License-Identifier: Apache-2.0 OR LGPL-2.1-or-later