Skip to content

Shut a FlyingSocks controller's socket down before closing it - #71

Closed
lhoward wants to merge 1 commit into
mainfrom
flyingsocks-close-while-reading
Closed

lhoward wants to merge 1 commit into
mainfrom
flyingsocks-close-while-reading

Conversation

@lhoward

@lhoward lhoward commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Summary

When a FlyingSocks controller's keepalive expires, unlockAndRemove calls controller.close() while the controller's message loop (handle(for:)) is suspended reading its socket. close() closed the descriptor under that read. Closing a descriptor removes it from kqueue or epoll without waking anyone waiting on it:

  • The expired controller's read never finished. Neither did its handle(for:) task.
  • FlyingFox's socket pool still recorded the descriptor as registered. So a controller accepted later and given the same descriptor was never woken, and it expired in turn (SocketPool: keep a descriptor's registration when a closed socket's is reused swhitty/FlyingFox#244). This affects macOS devices, where Ocp1DeviceEndpoint is the FlyingSocks stream endpoint.
  • #244 isn't enough on its own. With it merged, the stale read could be woken by the new controller's data, and consume it.

Changes

  • Ocp1FlyingSocksStreamController.close() now shuts the socket down with shutdown(2), in both directions, instead of closing it. That wakes the suspended read with end of file, so the message loop ends as it does when the peer closes the connection.

  • The descriptor is closed exactly once, by whichever comes last:

    • close();
    • the message stream ending, however it ends, which means no read is suspended.

    A small Mutex-protected lifetime object tracks this. The shutdown and close calls are both made while it holds the lock, so neither can reach a descriptor that was already closed and given to another socket. deinit closes it if neither got that far.

  • Windows uses shutdown(SOCKET, SD_BOTH). Everywhere else it's SHUT_RDWR.

This works with the FlyingFox we resolve (0.27.1), whether or not #244 is merged.

Behaviour change

When a keepalive expires, the message loop now ends. handle(for:) then calls unlockAndRemove for the second time, so onControllerExpiry is signalled twice. The Network.framework and IORing controllers already do this, because closing them wakes their pending receive.

Test

FlyingSocksControllerCloseTests.testClosingAControllerWhileItReadsEndsItsMessageLoop does the following:

  1. Runs a controller's handle(for:) over a socketpair, on the endpoint's running socket pool.
  2. Calls close() while the loop is suspended reading.
  3. Expects the loop to end within 2 s.

On macOS, with the fix it passes in 0.23 s. Without it, it fails after 2 s with "closing the controller should end its message loop".

Test plan

  • macOS: swift build --build-tests clean, full suite passes (290 tests, 0 failures)
  • macOS: the new test fails with the controller change reverted
  • Linux CI (the FlyingSocks controller isn't compiled for non-embedded Linux, which uses IORing)

🤖 Generated with Claude Code

https://claude.ai/code/session_012Fq7ie1LzJERrh9uRuZZWE

When a controller's keepalive expires, unlockAndRemove closes it while its
message loop is suspended reading its socket. Closing the descriptor
under that read drops it from the socket pool's kqueue or epoll set
without waking it:

- The read, and with it the controller's handle(for:) task, never
  finished.
- The pool kept the descriptor registered, so a controller accepted
  later and given the same descriptor was never woken
  (swhitty/FlyingFox#244).
- With that fixed, the stale read could instead be woken by the new
  controller's data.

close() now shuts the socket down in both directions instead. That wakes
the read with end of file, and the message loop ends as it would for a
peer's close. The socket is closed exactly once, by whichever comes last
of close() and the message stream ending, so it is never closed under a
suspended read. Both steps happen under one lock, so neither can reach a
descriptor already closed and reused.

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