Skip to content

Enqueue reply promise after the send, not before#14

Merged
henrikbjorn merged 1 commit into
masterfrom
enqueue-reply-after-send
Jul 16, 2026
Merged

Enqueue reply promise after the send, not before#14
henrikbjorn merged 1 commit into
masterfrom
enqueue-reply-after-send

Conversation

@henrikbjorn

Copy link
Copy Markdown
Member

Summary

Simplifies the cancellation-safe send path merged in #13. That fix kept the reply promise being pushed onto @reply_promises before the write and added a rescue Exception to delete-and-reject it if the send was interrupted. This reorders the two lines instead — send first, then push — so the orphaned-slot state can never exist in the first place, and the rescue block is deleted.

@write_lock.acquire do
  @connection.send_data(msg)   # bytes on the wire first
  @reply_promises << promise   # then reserve the slot
end

Why this is safe in both cancellation windows

  • Cancelled/failed mid-send (the incident: write blocked on backpressure, caller hangs up): send_data raises before the << runs, so no promise is ever enqueued — nothing to clean up, no orphan, no misrouting. Structurally impossible rather than defended.
  • Cancelled while awaiting the reply (the common hangup): the promise is already in the queue and stays there; the reply arrives, shift.resolve fires against a promise nobody is waiting on (a harmless no-op), and the FIFO stays aligned with the wire. Unchanged from before.

The one invariant this relies on: nothing yields between send_data returning and the push (both are sequential statements inside the lock), so no reply for this command can be observed before its promise is queued. The push stays inside the write lock so queue order keeps matching wire order.

Tests

The regression tests from #13 are unchanged in intent and still pass. I verified they still fail (3/3) on the naive push-before-send ordering, so they retain their teeth against this implementation — the reorder, not the tests, is what provides safety. Full suite: 120 runs, 414 assertions, 0 failures.

Note

This supersedes the rescue Exception approach from #13 (already merged); net effect is a deletion of that block. Builds on master.

Replaces the rescue-based guard from the previous commit with a simpler
ordering: send the command first, then push its reply promise onto
@reply_promises — both still under the write lock so queue order keeps
matching wire order.

A sender cancelled (Async::Stop) or failed mid-send now unwinds before it
enqueues anything, so it can never leave an orphaned slot for a later reply
to be misrouted onto. This makes the orphan structurally impossible instead
of defending against it, and deletes the begin/rescue Exception/delete/reject
block entirely. There is no yield between the send and the push, so no reply
for the command can be observed before its promise is queued; and a sender
cancelled at promise.wait was always safe, since its promise stays in the
queue and the matching reply resolves it harmlessly.

Tests unchanged in intent; verified they still fail on the naive
push-before-send ordering.
@henrikbjorn henrikbjorn self-assigned this Jul 16, 2026
@henrikbjorn
henrikbjorn requested a review from bgoRelatel July 16, 2026 07:11
@henrikbjorn
henrikbjorn merged commit c907e54 into master Jul 16, 2026
3 checks passed
@henrikbjorn
henrikbjorn deleted the enqueue-reply-after-send branch July 16, 2026 09:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants