Skip to content

epoll: drainRecvBuffer is unbounded, so a trickling peer can hold the event loop #571

Description

@FumingPower3925

The two engines' close-time drains have diverged

engine/epoll/loop.go:2724

func drainRecvBuffer(fd int) {
	var buf [4096]byte
	for {
		n, _ := unix.Read(fd, buf[:])
		if n <= 0 {
			return
		}
	}
}

engine/iouring/worker.go:4320

func drainRecvBuffer(fd int) {
	var buf [512]byte
	for i := 0; i < drainRecvMaxReads; i++ {
		n, _, err := unix.Recvfrom(fd, buf[:], unix.MSG_DONTWAIT)
		if n <= 0 || err != nil {
			return
		}
	}
}

Same name, same role, one bounded and one not. The io_uring version carries an explicit bound and a comment saying why: drainRecvMaxReads exists "so a peer that keeps trickling bytes" cannot pin the worker. That bound was added in response to a worker wedge. The epoll copy never got it.

Why the loop can spin

The read cannot block — epoll accepts with SOCK_NONBLOCK, so an empty queue returns EAGAIN and the loop exits. The exposure is different: a peer that keeps delivering data as fast as the loop consumes it keeps n > 0 true indefinitely. This runs on the event loop thread, so every other connection on that loop is starved for as long as it continues. It is the same hazard the io_uring bound was introduced to close, on the engine that did not get the fix.

Note the loop also discards the error from unix.Read and relies on n <= 0 alone. That happens to be correct for EAGAIN, which returns -1, but it means a genuine read error is indistinguishable from an empty queue.

Why this is worth fixing even though the drain is right

The drain itself is correct and should stay — see #569, which I filed and then closed after establishing that close(2) discards the receive queue regardless, so draining only removes the reset that would additionally destroy queued outbound data. This issue is not about whether to drain. It is that one engine is bounded against a hostile peer and the other is not, and the release bar asks for identical behaviour across engines.

Suggested fix

Give epoll the same bound as io_uring, and prefer a shared implementation over two copies that can drift again. Worth confirming whether the buffer sizes should also converge — 512 bytes against 4096 is a second unexplained difference between them.

Not urgent for correctness on a well-behaved peer, and deliberately not landing before the pending dual-architecture benchmark, since it touches the close path of every connection on both engines.

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

    area/engineEngine interface or implementation

    Type

    No type

    Projects

    No projects

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions