Skip to content

Engines drop a peer's final line on RST: SO_ERROR pre-check runs before reading (PR #101 finding 4 remainder) #104

Description

@MrIron-no

All event engines check the socket error state before reading. When a peer's final line and an RST are both pending by the time the event loop looks at the socket, the engine generates ET_ERROR from the SO_ERROR/EPOLLERR pre-check without ever reading, and the line is dropped: a server link's closing ERROR/SQUIT (or a client's QUIT reason) is lost and the teardown is reported as Read error: Connection reset by peer with no reason from the peer.

This is the deferred remainder of PR #101 finding 4. The kqueue FIN case was fixed in c122bab (EV_EOF with data > 0 now drains before ET_EOF), and the epoll FIN case needs no fix — but the RST case is engine-independent and still present everywhere, including kqueue, whose SO_ERROR pre-check bypasses the new ordering.

Reproduced on Linux/epoll

Deterministic repro in the docker test harness: freeze the ircd with docker pause (the shared kernel keeps ACKing and buffering), have a channel member send QUIT :final-words followed by its connection teardown, unpause, and observe the quit reason broadcast to another member:

teardown reason seen by the other client verdict
FIN (orderly close()) Quit: final-words pending line delivered — epoll needs no FIN fix, as argued in #101
RST (SO_LINGER(1,0) + close()) Read error: Connection reset by peer pending line dropped

engine_epoll.c sees EPOLLERR|EPOLLIN|EPOLLHUP, takes the EPOLLERR branch at the top of the dispatch, reads SO_ERROR, and generates ET_ERROR — the readable data is never touched.

Two harness caveats for anyone re-testing this:

  • The repro must connect to the container IP, not the published port on 127.0.0.1: localhost published ports go through docker-proxy, which terminates the TCP connection on the host and relays — an RST sent by the test is laundered into a clean close and the bug does not reproduce.
  • Without the pause, delivery races the event loop and the result is nondeterministic.

The repro exists as a pytest file (FIN variant as a passing control, RST variant as a strict=True xfail that will flag itself when the fix lands) and can be contributed alongside the fix.

Fix sketch (the "recv-before-error" design from the #101 discussion)

In each engine's dispatch, when the error/HUP condition coincides with readable data on a connected stream socket, generate ET_READ instead and let the level-triggered engine refire; once the buffer is drained, the next pass takes the error/EOF branch for real.

Two constraints that make this more than a mechanical reorder:

  1. getsockopt(SO_ERROR) consumes the error. The pre-check must not run while data is pending, or the eventual teardown degrades into a fake-clean EOF with no ECONNRESET detail. Deferring the check entirely and letting recv() surface the error after the drain avoids stashing errcode state per socket.
  2. Deferred errors must not busy-loop on owners that don't read. A dead-socket client (FLAG_DEADSOCKET) whose ET_READ arm is a no-op would spin — the guard added for Drive the inbound TLS handshake by socket events; kqueue: deliver unread data before EOF #101 finding 2 (exit dead clients from the ET_READ arm) has to be in place wherever this lands.

Per-engine scope:

  • epoll (engine_epoll.c): EPOLLERR and EPOLLHUP branches defer to ET_READ while EPOLLIN is set. Small, and verifiable in the existing Linux harness with the repro above. SS_CONNECTING/SS_LISTENING keep the current error short-circuit.
  • kqueue (engine_kqueue.c): gate the SO_ERROR pre-check on no pending read data, folding into the ordering c122bab introduced. Needs a BSD build to validate.
  • poll / devpoll / select: same treatment for the unconditional SO_ERROR pre-check and the POLLHUP short-circuits; devpoll is Solaris-only and hardest to validate.

A first increment covering epoll + kqueue would cover the engines in production use; the remaining engines can follow the same pattern.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions