Skip to content

Follow-ups from #800: test ringSendOutstanding's sendBuf and zcNotifPending clauses, one copy of the condition, the comment's locking claim, the over-cap drop on the async path #815

Description

@FumingPower3925

The review of #800 (celeris#751) had no blocking findings. It left these minor findings and nits, and each says what to do. Line numbers are at the PR's head b340cbe. The review's probes and logs are copied to evidence/lanes-20260927/EP-3/review-r1/ (correctness/ and evidence/).

1. The regression test covers one clause of ringSendOutstanding (minor)

ringSendOutstanding (worker.go:4597-4598) is cs.sending || cs.zcNotifPending || len(cs.sendBuf) > 0 || len(cs.bodyBuf) > 0. TestAsyncResponseWaitsForAnInFlightRingSend reaches only the cs.sending clause. The review's mutants M3 (return cs.sending) and M4 (return len(cs.sendBuf) > 0) each pass the test 4 of 4 times (review-r1/evidence/logs/800-mutants-m8.log).

The state that only the sendBuf clause catches is real. completeSend keeps a partial SEND's remainder in sendBuf. When the SQ ring is full, flushSend returns true and leaves the conn with sending=false (worker.go:5166-5171). SQ-full is the common case at CI's one-worker memlock. The review's probe review-r1/evidence/ov/800probe/zz_review_probe_751_sqfull_linux_test.go builds this state: the kernel sends 4096 bytes, the 64-entry SQ ring is filled, the SEND completes partially, and then a pipelined /small arrives. Results:

  • head b340cbe: PASS 2 of 2, early=0 sends=2.
  • head with M3: FAIL 2 of 2, early=103 sends=1, "response 1 corrupted at byte 223253".
  • main dfd044f: fails the same way, 2 of 2.

To do: commit the probe as a third arm of the test. Add a SEND_ZC arm (first completion with F_MORE, then the notification) to cover zcNotifPending.

2. The guarded writeFn keeps its own copy of the condition (nit)

worker.go:2376-2377 tests !cs.fixedFile && !cs.sending && !cs.zcNotifPending && len(cs.sendBuf) == 0 && len(cs.bodyBuf) == 0 && .... That is the same four clauses as the new helper. No test goes beyond cs.sending for either copy, so the two can drift apart without any test noticing.

To do: use !ringSendOutstanding(cs) there, which keeps one source for the condition. Item 1's arms then cover both uses.

3. ringSendOutstanding's comment overstates the locking (nit)

The comment says "The worker mutates all four under cs.detachMu ... so no SEND can start while it does" (worker.go:4594-4595). Two worker paths call flushSend without detachMu:

  • closeConn's deferred-close flush (worker.go:3771);
  • the h2Conns write-queue drain (worker.go:1347).

Neither can overlap the goroutine's direct write:

  • closeConn sets asyncClosed first, and the goroutine either re-checks it under detachMu or has already gone.
  • h2Conns holds only conns whose goroutine has exited.

So the check is correct, but the comment should state this narrower invariant.

To do: reword the comment.

4. Over the send cap, the async path drops whole responses without closing (minor, pre-existing; covered by #805)

makeWriteFn silently drops a write made while writeBuf + sendBuf + bodyBuf exceeds sendCap() (4 MiB, worker.go:4608). The inline path closes the conn in that case (respondAndArm's cap check). runAsyncHandler never checks.

With #800, pipelined responses queue in writeBuf behind an outstanding SEND, including across separate goroutine iterations, where main would have written them directly. So a pipelining client behind a large response can lose whole responses without the conn closing. A later response can then be read as the answer to an earlier request.

The review's probe review-r1/correctness/probes/zz_review_overcap_linux_test.go sends /big (1 MiB) with its tail SEND outstanding, then 6 pipelined 1 MiB responses (-race, unlimited memlock, 2 of 2 runs):

So this is not a regression, but #800's ordering guarantee holds only below the cap.

Open PR #805 (celeris#761) makes a refused write set writeRefused, and it closes the conn on the dispatch goroutine's path too.

To do: once #800 and #805 are both merged, run the probe on main and confirm that the conn is closed and no response is lost silently. If #805 does not cover this, close over the cap on the async path, or stop dispatching new requests while the backlog is over the cap.

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 implementationarea/testTesting infrastructurebugSomething isn't workingengine/iouringio_uring engine specificsplatform/linuxLinux-specific (io_uring, epoll)

    Type

    No type

    Projects

    No projects

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions