Conversation
Sources/HTTPServer/HTTPServer.swift
Outdated
There was a problem hiding this comment.
Are you planning to enable NonisolatedNonsendingByDefault on the whole target, which would make these unnecessary?
There was a problem hiding this comment.
It is already enabled and on closures this is still necessary AFAIK.
There was a problem hiding this comment.
Ah, this is because the closure in RequestResponseContext/withContext was marked as @Sendable and its signature is generic on arguments conforming to ConcludingAsyncReader and ConcludingAsyncWriter. But I actually don't think it should be @Sendable so I've removed this.
| final class ReaderState { | ||
| var trailers: HTTPFields? = nil | ||
| var finishedReading: Bool = false | ||
| } |
There was a problem hiding this comment.
Some idea for us in the future. To avoid this allocation we could use an InlineArray<1, State> potentially that we share into the Reader and Writer. It would require the reader and writer to become ~Escapable but this might make it possible for us to avoid the allocation altogether.
Sources/HTTPServer/HTTPServer.swift
Outdated
There was a problem hiding this comment.
It is already enabled and on closures this is still necessary AFAIK.
Sources/HTTPServer/HTTPServer.swift
Outdated
There was a problem hiding this comment.
That's not really a context right? This is essentially the parameters to the request handler right? Do we really need this type and if we do can we come up with about better name + docs + separate file for it?
There was a problem hiding this comment.
Yeah no, it's a very bad name but I didn't know what to call it :)
Sadly I think this type has to exist because tuples can't contain ~Copyable elements, and I need to be able to somehow spell the Input type of the middleware.
There was a problem hiding this comment.
I've renamed this to RequestResponseMiddlewareBox. I still hate it but open to suggestions.
I do have a question about its current shape though. I need to provide a withContents method so that users can access the stored request/reader/response sender. I could get around this by making the properties public instead, but then I'd have to mark the type as @frozen because, as the properties are consuming, you can't otherwise consume individual properties.
It annoys me a bit that we need a withContents method; but I'm not sure marking the type as frozen is good for API evolution either. So I decided to go with the former but I'd like to hear thoughts here too.
gjcairo
force-pushed
the
response-manager-serve
branch
from
d13e461 to
e6d9933
Compare
| /// This type holds the values passed to the ``HTTPServerRequestHandler`` when handling a request. | ||
| /// It is necessary to box them together so that they can be used with `Middlewares`, as this will be the `Middleware.Input`. | ||
| @available(macOS 26.0, iOS 26.0, watchOS 26.0, tvOS 26.0, visionOS 26.0, *) | ||
| public struct RequestResponseMiddlewareBox< |
There was a problem hiding this comment.
I'm not super happy with that name yet but don't wanna block the PR on naming bike-shedding. In particular, the HTTPServer target should know nothing about middleware. So maybe this is just a HTTPRequestHandlerInput or something along those lines? We should also explore using @frozen instead of the withContents method but can do that in a follow up PR. Since we are not ABI stable in this package we might be able to use it.
3 participants
This PR adds the necessary annotations to make all reader and writer types
~Copyableso that we can enforce call-once behaviour on the reader and writers.It also exposes some additional state from the reader and writer to the server so that we can fail accordingly.