Sockets

[adh]

No description provided.

[Copilot]

🟡 Changes recommended

The newly added stream-socket tests assume full-length reads in a single call, which can produce short reads and cause flaky failures.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR introduces a new high-level socket networking subsystem for ListTalk (IP sockets and Unix-domain sockets), exposes it via a native socket module, and adds C + ListTalk-level tests plus Meson build integration.

Changes:

• Add new socket APIs and ListTalk class bindings for UDP/TCP and Unix-domain sockets (including buffered readLine support).
• Introduce a shared ListTalkNetworking library and a socket.ltm native module.
• Add new networking test executable and an eval test that exercises the module from ListTalk code.

File summaries

File	Description
tests/networking_test.c	Adds C-level integration tests for TCP/UDP and Unix stream/server sockets.
tests/eval-socket.lt	Adds ListTalk-level eval test coverage for the new :socket module.
src/networking/UnixSocket.c	Implements Unix-domain datagram/stream/server sockets and ListTalk primitives/classes.
src/networking/SocketBuffer.c	Adds shared buffered read + readLine implementation used by stream sockets.
src/networking/Socket.c	Implements IP (UDP/TCP) sockets and corresponding ListTalk primitives/classes.
src/networking/Socket_internal.h	Declares the internal read-buffer utilities shared by socket implementations.
src/modules/socket.c	Exposes the new socket classes in a ListTalk:Socket package and provides socket.
meson.build	Builds/installs the networking library and socket module; registers new tests.
ListTalk/networking/Socket.h	Public header for the new networking socket APIs.

Review details

Suppressed comments (3)

tests/networking_test.c:155

• This test assumes LT_UnixStreamSocket_read returns all requested bytes in one call; stream reads can be short. Loop until 4 bytes are read (or EOF) to avoid intermittent failures.

    check(
        LT_UnixStreamSocket_read(unix_second, reply, sizeof(reply)) == 4
            && !memcmp(reply, "pair", 4),
        "Unix stream socket pair round trip"
    );

tests/networking_test.c:172

• This check assumes a single stream read returns 2 bytes, but short reads are allowed. Reading in a loop makes the test deterministic across platforms and scheduling.

    check(
        LT_UnixStreamSocket_read(unix_second, reply, 2) == 2
            && !memcmp(reply, "bc", 2),
        "Unix stream read consumes readLine buffered bytes"
    );

tests/networking_test.c:198

• This test assumes LT_UnixStreamSocket_read will return all 4 bytes in one call, but stream reads can be short. Loop until the expected number of bytes is read (or EOF) to prevent flaky failures.

    check(
        LT_UnixStreamSocket_read(unix_second, reply, sizeof(reply)) == 4
            && !memcmp(reply, "path", 4),
        "pathname Unix stream socket round trip"
    );

• Files reviewed: 9/9 changed files
• Comments generated: 2
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

Several connection-setup and shutdown paths don’t robustly handle EINTR/pending-signal processing, which can cause spurious failures under signal delivery.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (3)

Previously missed (1) — in code that hasn't changed since the last review.

ListTalk/networking/Socket.h:7

• Public header Socket.h relies on transitive includes for uint16_t/size_t, which makes it fragile for external consumers. Add the standard headers directly so the header is self-contained.

src/networking/Socket.c:330

• LT_TCPSocket_shutdown_write() does not handle EINTR from shutdown(), so an interrupting signal can surface as an unexpected error. Retry shutdown() on EINTR and call LT_socket_interrupted() to process pending signals.

void LT_TCPSocket_shutdown_write(LT_TCPSocket* socket){
    if (shutdown(socket_fd((LT_IPSocket*)socket), SHUT_WR) != 0){
        LT_system_error("Socket shutdown failed", errno);
    }

src/networking/UnixSocket.c:296

• LT_UnixStreamSocket_shutdown_write() does not handle EINTR from shutdown(), so an interrupting signal can surface as an unexpected error. Retry shutdown() on EINTR and call LT_socket_interrupted() to process pending signals.

void LT_UnixStreamSocket_shutdown_write(LT_UnixStreamSocket* socket){
    if (shutdown(unix_socket_fd((LT_UnixSocket*)socket), SHUT_WR) != 0){
        LT_system_error("Unix socket shutdown failed", errno);
    }

• Files reviewed: 11/11 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

The new Unix-domain socket implementation has confirmed EINTR/EISCONN handling bugs in connect/bind/listen paths that can cause spurious failures under signal delivery.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/networking/UnixSocket.c:142

• unix_bind treats EINTR from bind(2)/listen(2) as an immediate fatal error. Other socket syscalls in this PR explicitly retry on EINTR and call LT_socket_interrupted(); binding/listening should follow the same pattern to avoid spurious failures under signal delivery.

• Files reviewed: 11/11 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

There are concrete portability/reliability bugs in the new socket implementation (EINTR handling for Unix bind/listen and SIGPIPE risk without MSG_NOSIGNAL) that should be fixed before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

src/networking/UnixSocket.c:150

• unix_bind() does not retry bind()/listen() on EINTR, unlike the IP socket path in Socket.c which explicitly loops and calls LT_socket_interrupted(). This makes Unix-domain server creation spuriously fail when the process receives signals during bind/listen.
  src/networking/Socket.c:20
• This file uses IPPROTO_IPV6/IPV6_V6ONLY but does not include <netinet/in.h> directly, relying on transitive includes (e.g. via <netdb.h>). Including <netinet/in.h> explicitly improves portability and avoids build failures on platforms where <netdb.h> doesn't provide those constants.

• Files reviewed: 11/11 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Critical and moderate findings remain in IP and Unix socket handling and tests.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

src/networking/IPSocket.c:186

• The descriptor created at line 138 is still owned only by the local fd; the socket object's finalizer is registered later by set_fd after this helper returns. LT_socket_interrupted() runs a pending ListTalk callable, which can throw and non-locally unwind the constructor, so an interrupted connect/bind (and the analogous listen retry below) can leak this descriptor. Put the address attempt in unwind-protected cleanup or otherwise close/transfer fd before dispatching the pending signal.

            if (errno == EINTR){
                LT_socket_interrupted();
                continue;

src/networking/UnixSocket.c:287

• This fallback has the same SIGPIPE problem as the IP stream implementation: on platforms without MSG_NOSIGNAL, a write after the peer closes can terminate the process before LT_system_error handles the failure. Use a platform-safe suppression mechanism such as SO_NOSIGPIPE when creating the Unix sockets.

#else
        ssize_t count = send(
            unix_socket_fd((LT_UnixSocket*)socket),
            bytes + offset,
            length - offset,
            0
        );

• Files reviewed: 12/12 changed files
• Comments generated: 5
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Multiple moderate issues remain in socket cleanup, signal handling, portability, and test safety.

Review details

Suppressed comments (9)

src/networking/IPSocket.c:162

• This condition prevents the IPv6 dual-stack setup for passive datagram sockets. On hosts whose IPv6 sockets default to IPV6_V6ONLY=1, a wildcard UDPSocket bind can successfully bind the IPv6 result first and stop iterating, so IPv4 datagrams sent to the wildcard endpoint are never received. Apply the same passive IPv6 handling here as for TCP (or keep binding all resolved addresses).

        if (do_listen
                && address->ai_family == AF_INET6
                && setsockopt(

src/networking/IPSocket.c:186

• The descriptor is live when LT_socket_interrupted() is called, but that function can run a pending callable through LT_apply, which may non-locally throw. Since set_fd() is only reached after resolve_socket() returns, such a transfer skips the cleanup at the end of the address loop and leaks the raw descriptor; protect this fd-owning region with an unwind cleanup.

            if (errno == EINTR){
                LT_socket_interrupted();
                continue;

src/networking/IPSocket.c:186

• If connect() is interrupted after the connection has completed, retrying can return EISCONN. This loop treats that as a failed address, closes the already-connected descriptor, and raises an error; handle EISCONN as success here, as the Unix-domain connector does.

            if (errno == EINTR){
                LT_socket_interrupted();
                continue;

src/networking/IPSocket.c:338

• On targets without MSG_NOSIGNAL (notably macOS), this fallback allows SIGPIPE from a write after the peer closes. That terminates the embedding process before LT_system_error can report the broken connection; set SO_NOSIGPIPE when creating the socket (or otherwise suppress SIGPIPE) for this branch.

        ssize_t n = send(
            socket_fd((LT_IPSocket*)socket),
            bytes + offset,
            length - offset,
            0

src/networking/IPSocket.c:356

• shutdown can return EINTR when a signal is delivered. This path neither invokes LT_socket_interrupted nor retries, so a pending ListTalk signal becomes a fatal Socket shutdown failed error instead of being dispatched consistently with the other socket syscalls. Retry EINTR before raising other errors.

    if (shutdown(socket_fd((LT_IPSocket*)socket), SHUT_WR) != 0){
        LT_system_error("Socket shutdown failed", errno);

src/networking/UnixSocket.c:121

• If the pending signal handled here throws, control leaves unix_connect() while fd is still a raw descriptor. unix_stream_from_fd() has not run yet, so no finalizer owns it, and the later close(fd) path is skipped; protect this fd-owning loop with an unwind cleanup before dispatching the signal.

        if (errno == EINTR){
            LT_socket_interrupted();
            continue;

src/networking/UnixSocket.c:286

• On targets without MSG_NOSIGNAL (notably macOS), this fallback allows SIGPIPE from a write after the peer closes. That terminates the embedding process before LT_system_error can report the broken connection; set SO_NOSIGPIPE when creating the socket (or otherwise suppress SIGPIPE) for this branch.

        ssize_t count = send(
            unix_socket_fd((LT_UnixSocket*)socket),
            bytes + offset,
            length - offset,
            0

src/networking/UnixSocket.c:305

• shutdown can return EINTR when a signal is delivered. This path neither invokes LT_socket_interrupted nor retries, so a pending ListTalk signal becomes a fatal Unix socket shutdown failed error instead of being dispatched consistently with the other socket syscalls. Retry EINTR before raising other errors.

    if (shutdown(unix_socket_fd((LT_UnixSocket*)socket), SHUT_WR) != 0){
        LT_system_error("Unix socket shutdown failed", errno);

tests/networking_test.c:95

• If getsockname fails, check only records the failure and execution still reaches this condition, which reads ss_family from the uninitialized wildcard_address and invokes undefined behavior. Initialize the storage before the call or guard the family-specific check on a successful getsockname result.

    if (wildcard_address.ss_family == AF_INET6){

• Files reviewed: 12/12 changed files
• Comments generated: 0 new
• Review effort level: Lite