Skip to content

SocketPool: throw CancellationError when cancelled while waiting - #242

Open
lhoward wants to merge 2 commits into
swhitty:mainfrom
PADL:pool-run-cancellation
Open

lhoward wants to merge 2 commits into
swhitty:mainfrom
PADL:pool-run-cancellation

Conversation

@lhoward

@lhoward lhoward commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Stacked on #243. This branch is rebased onto #243, which fixes the Linux failures in sendMessage_WithPacketInfo_RoundTripsDatagram that aren't caused by this change. Until #243 is merged, the diff here includes its commit (Socket: send pktinfo with the source address in the right field); after that, only this PR's own commit remains.

Summary

Cancelling SocketPool.run() while the pool waits on its event queue throws the interrupted wait's error instead of CancellationError, and HTTPServer.run() then logs it as a critical server error. The result is that every cancelled HTTPServer on Darwin logs:

server error: ... kqueue kevent ... Bad file descriptor

Cause

getNotifications() runs the blocking wait on its dispatch queue, and its cancellation handler calls stopQueue() to break that wait. For kQueue, stop() closes the kqueue while kevent is still waiting on it. So kevent returns EBADF, getNotifications() throws SocketError.makeFailed("kqueue kevent"), and that becomes run()'s error.

Changes

  • SocketPool.getNotifications(): if the wait fails after the task has been cancelled, throw CancellationError.
  • HTTPServer.run(): don't log a CancellationError as a critical server error. Other errors are still logged.

On Linux, as far as I can tell from reading the code, the ePoll queue already comes out as CancellationError: stop() wakes the wait through the canary eventfd, and the next getNotifications() call's Task.checkCancellation() throws. So only the logging change applies there.

Reproducing

It's a race, and whether it happens depends on how far the pool has got when it's cancelled. The existing taskCanBeCancelled cancels as soon as the server is listening, before the pool is waiting on its queue, so it doesn't see this. I cancelled a server 200 ms after it started listening, three times per build:

build runs ending in SocketError (kqueue kevent, EBADF)
0.26.2 3 of 3
0.27.1 2 of 3
main (bd32d43) 2 of 3

Tests

  • New: HTTPServerTests.taskCanBeCancelled_WhileWaitingForConnections. It does that 5 times and expects CancellationError each time.
    • Without the SocketPool change it failed every time I ran it, with 5 or 6 issues per run, each expected error of type CancellationError, but ".failed(type: "kqueue kevent", errno: 9, ...)" of type SocketError was thrown instead.
    • With the change it passes.
  • Tightened: HTTPServerTests.taskCanBeCancelled now expects CancellationError, not any error.

The full suite passes on macOS (Swift 6.3.3). In 5 runs, one hung in HTTPClientTests.client_sends_request(), which never cancels a server while it runs. Unmodified main hangs the same way, in 1 of 6 runs, there in handlerError_ResponseIncludesDateHeader(). So that's an existing intermittent hang, not something this change introduces. I haven't run the suite on Linux.

We found this in SwiftOCA, whose device endpoints use SocketPool the same way HTTPServer does.

🤖 Generated with Claude Code

https://claude.ai/code/session_012Fq7ie1LzJERrh9uRuZZWE

lhoward added a commit to PADL/SwiftOCA that referenced this pull request Sep 11, 2026
Cancelling a FlyingSocks stream or datagram endpoint's run() cancels its
socket pool, which stops its event queue under the wait in progress. On
Darwin that wait can then fail with EBADF instead of ending as a
cancellation: a race, lost in most runs. When it was lost, the endpoint
logged a critical "server error ... kqueue kevent" and threw that error.
It also skipped removing itself from its device, so it stayed
registered.

When the task has been cancelled, close the socket, remove the endpoint
and throw CancellationError, whatever the pool threw. That covers the
FlyingFox we resolve (0.27.1) as well as swhitty/FlyingFox#242, which
makes the pool itself throw CancellationError.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012Fq7ie1LzJERrh9uRuZZWE
@codecov

codecov Bot commented Sep 11, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.74%. Comparing base (bd32d43) to head (2414491).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #242      +/-   ##
==========================================
+ Coverage   93.72%   93.74%   +0.02%     
==========================================
  Files          72       72              
  Lines        3777     3791      +14     
==========================================
+ Hits         3540     3554      +14     
  Misses        237      237              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

withPacketInfoControl built the in_pktinfo or in6_pktinfo in the element
storage of a ManagedBuffer, which starts out uninitialised, and:

- For IPv4 it wrote the local address to ipi_addr and never set
  ipi_spec_dst, the field ip(7) says sendmsg(2) takes the source address
  from. So the source was whatever was in memory, and sendmsg could fail
  with ENETUNREACH, as sendMessage_WithPacketInfo_RoundTripsDatagram does
  on some Linux builds.
- It read the address by reinterpreting the bytes of the any SocketAddress
  existential. Those bytes are the address only when the value is held
  inline: a sockaddr_in6 or sockaddr_storage is boxed, so what was read
  was the box.

Zero the structure, read the address from makeStorage(), and put an IPv4
source in ipi_spec_dst.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012Fq7ie1LzJERrh9uRuZZWE
@lhoward

lhoward commented Sep 11, 2026

Copy link
Copy Markdown
Contributor Author

The Linux Swift 6.0 and 6.2 failures here are in sendMessage_WithPacketInfo_RoundTripsDatagram, and they aren't caused by this change. withPacketInfoControl never sets ipi_spec_dst, so the IPv4 source address sendmsg uses is whatever happens to be in memory, and it fails with ENETUNREACH on some builds but not others. The tests this PR adds pass on those same jobs. #243 fixes the pktinfo bug; once it's merged, re-running the jobs here should pass.

Cancelling SocketPool.run() while it waits on its event queue made it
throw the error of the interrupted wait instead of CancellationError. On
Darwin, getNotifications()'s cancellation handler stops the queue, which
closes the kqueue under the kevent call in progress. That call fails with
EBADF, and run() threw SocketError.makeFailed("kqueue kevent"). Throw
CancellationError when the wait fails after the task has been cancelled.

HTTPServer.run() logged every error as a critical server error, including
this one, so each cancelled server logged "server error: ...kqueue
kevent...". A cancellation is not a server error; don't log it as one.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012Fq7ie1LzJERrh9uRuZZWE
@lhoward
lhoward force-pushed the pool-run-cancellation branch from c622d6e to 2414491 Compare September 11, 2026 04:41
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.

1 participant