feat: make SwiftWebServer @MainActor for honest Sendable conformance#5
Conversation
The class is fundamentally main-runloop-coupled — listen() installs CFSocket accept callbacks via CFRunLoopGetCurrent(), which means it already requires the calling thread to be running its own CFRunLoop (in practice: main on iOS/macOS apps). Annotating the class @mainactor matches that runtime reality and makes the type automatically Sendable, so consumers no longer need an @unchecked Sendable wrapper to pass instances across actor boundaries. The CFSocket accept callbacks (which fire on the main run loop) now wrap their @mainactor calls in MainActor.assumeIsolated, asserting at runtime the same isolation the API contract already requires. Per-request work in Connection still runs on a background dispatch queue (so blocking recv() doesn't stall main). The configuration state Connection reads during request handling — routeHandlers, the router instance, the middleware manager, staticDirectories — is annotated nonisolated(unsafe) under the documented contract that it is configured before listen() and treated as read-only thereafter. findStaticFile(for:) and middlewareManagerInternal are nonisolated for the same reason. Package now declares macOS 12 as a platform minimum (MainActor itself is macOS 10.15+, but staying on a clean modern minimum). Test classes get @mainactor since they construct and configure the server. All 43 tests pass. Suggested release: 0.3.0 — minor bump for the Sendable conformance, the new platform minimum, and the @mainactor isolation requirement on consumer call sites that were previously implicitly main-thread.
|
@claude review this PR |
|
Claude finished @atom2ueki's task in 2m 5s —— View job PR Review: feat: make SwiftWebServer @mainactor for honest Sendable conformance
Overall: This is a well-reasoned and correctly implemented change. The motivation is sound — Correctness Issues
|
|
@codex review this PR |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de85d1b302
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex review on PR #5 caught that the new sources use nonisolated(unsafe) (Swift 5.10) and MainActor.assumeIsolated (Swift 5.9), but the manifest still declared swift-tools-version:5.6. Consumers on Swift 5.6-5.9 would have hit a parse error when SPM resolved this package. Raising the manifest tools version to 5.10 — the minimum that supports all the annotations introduced for @mainactor isolation — so dependency resolution honestly reflects the source's compiler requirements.
- Make the "configure before listen()" contract load-bearing: every route registration (get/post/put/delete) and every middleware/static directory mutator now goes through `assertNotRunning()`, which is a precondition that fires with a clear message if the caller mutates config after the server reaches `.running`. The state in question is annotated `nonisolated(unsafe)` because Connection reads it from a background queue; mutating after listen() was undefined behavior, and this turns "undefined" into a deterministic crash. - Add the same explanatory comment to the IPv6 CFSocket accept callback that the IPv4 one already had, so a future reader doesn't have to hunt across the file to understand why MainActor.assumeIsolated is safe in a @convention(c) block. - Promote `@MainActor` from `MockResponse.init()` to the whole class in MiddlewareTests so the isolation declaration matches MockConnection and the parent class context. Pre-existing issues called out by review (out of scope for this PR): - Connection holds a non-weak reference to SwiftWebServer; combined with SwiftWebServer.connections holding strong refs to Connections, this is a retain cycle that prevents server deallocation while connections are alive. - listen() does Unmanaged.passRetained(self) into CFSocketContext but the context's `release` callback is nil, leaking one refcount per listen(). Both worth opening separate issues for; not touching them here to keep the @mainactor PR focused.
|
@claude — review feedback addressed. Fix-up commits:
Pre-existing items called out (out of scope, not touching in this PR):
Worth opening separate issues for both. Build + 43 tests still green. |
|
Claude finished @atom2ueki's task in 1m 8s —— View job Re-review: Verifying fix-up commits
All four findings from the original review are fully addressed. Here's what I verified in the diff: Medium —
|
| Method | Guarded |
|---|---|
use(_ middleware:) |
✅ |
use(_ path:, _ middleware:) |
✅ |
use(_ method:, _ path:, _ middleware:) |
✅ |
get(_:completion:) |
✅ |
post(_:completion:) |
✅ |
put(_:completion:) |
✅ |
delete(_:completion:) |
✅ |
use(staticDirectory:) |
✅ |
Low — IPv6 callback comment (SwiftWebServer.swift) ✅
The IPv6 @convention(c) callback now has the same explanation as IPv4: "Same as the IPv4 callback above: this fires on the run loop the source was added to, which listen() requires to be the main run loop."
Low — MockResponse class-level @MainActor (MiddlewareTests.swift) ✅
@MainActor is now on the class declaration, not just init(). All three test classes (SwiftWebServerTests, MiddlewareTests, BindRequestTests) are properly annotated.
Verdict: All review findings resolved. This PR is ready to merge.
The pre-existing issues (retain cycle on Connection.server, unbalanced passRetained in CFSocketContext) are correctly scoped out — worth filing as separate issues before 0.3.0 ships if any consumers call listen() more than once in a process lifetime.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 59781c1f85
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex P2 review on PR #5 caught that the variadic overloads (get/post/put/delete with `_ middleware: Middleware...`) were inserting middleware into the manager *before* delegating to the base method that held the assertNotRunning() check. A caller registering a route after listen() would mutate shared request-path state on a live server and only then trap, reintroducing the race the precondition was meant to prevent. Move assertNotRunning() to the top of each variadic overload, before any middleware insertion, so the trap fires before any state is touched.
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary
Annotate `SwiftWebServer` with `@MainActor` so it is automatically `Sendable` and the type system reflects what the runtime already requires. The class installs its `CFSocket` accept callbacks via `CFRunLoopGetCurrent()`, so `listen()`/`close()` already had to be called from a thread running its own run loop (in practice: main on iOS/macOS apps). `@MainActor` makes that contract explicit.
This replaces consumer workarounds like `@unchecked Sendable` boxes (which CodingPlanAuthKit's `LocalCallbackServer` had to use against 0.1.0/0.2.0).
Why not `@unchecked Sendable`?
The class has mutable state (`_status`, `_currentPort`, route handlers, middleware manager, sockets, the static `connections` dict) accessed from CFSocket callbacks, route handlers, and configuration calls. Without internal synchronization, declaring `@unchecked Sendable` would over-promise. `@MainActor` is honest: the implementation needs main-runloop isolation, so the type system says so.
Per-request work — still on a background queue
To keep `recv()` from blocking main, `Connection` continues to run on its own dispatch queue. The configuration state it reads during request handling (routes, middleware, static directories) is annotated `nonisolated(unsafe)` under the documented contract that it is configured before `listen()` and treated as read-only after. `middlewareManagerInternal` and `findStaticFile(for:)` are `nonisolated` for the same reason.
If a consumer mutates routes after `listen()`, the behavior is undefined — same as before, just now spelled out.
CFSocket accept callbacks
The `@convention(c)` accept callbacks now wrap their `handleConnect` call in `MainActor.assumeIsolated`. That's a runtime assertion of what the run loop already guarantees — it satisfies the type system without changing behavior.
Migration
Consumer call sites that were already on `@MainActor` (or hopping there via `Task { @mainactor in ... }`) keep working unchanged. Sites that were inadvertently calling from a background context will now get a compiler diagnostic instead of a silent race — strictly an improvement.
The package now declares `macOS(.v12)` as a platform minimum. `MainActor` itself is macOS 10.15+; sticking to a modern minimum keeps the surface clean.
Test plan
Suggested release
`0.3.0` — minor bump for the new `Sendable` conformance, the platform minimum, and the `@MainActor` isolation requirement.