Ah, I think I have an idea.
@johannesweiss I think I have a prototype:
protocol HTTPResponseDelegate: AnyObject {
func didReceivePart(task: HTTPClient.Task<Response>, _ buffer: ByteBuffer) -> EventLoopFuture<Void>?
}
internal class TaskHandler...... {
// Haven't yet figured out how to use BackPressureHandler, reimplemented parts of it for now as an example
var pendingRead = false
var writable = true
public func read(context: ChannelHandlerContext) {
if writable {
context.read()
} else {
pendingRead = true
}
}
private func mayRead(context: ChannelHandlerContext) {
if pendingRead {
pendingRead = false
context.read()
}
}
func channelRead(context: ChannelHandlerContext, data: NIOAny) {
switch unwrapInboundIn(data) {
....
case .body(let body):
self.state = .body
if let future = self.delegate.didReceivePart(task: self.task, body) {
self.writable = false
future.whenComplete { _ in
self.writable = true
self.mayRead(context: context)
}
}
....
}
}
}
class ProxyingDelegate: HTTPClientResponseDelegate {
public typealias Response = Void
let chunkHandler: (ByteBuffer) -> EventLoopFuture<Void>
init(chunkHandler: @escaping (ByteBuffer) -> EventLoopFuture<Void>) {
self.chunkHandler = chunkHandler
}
func didReceivePart(task: HTTPClient.Task<Void>, _ buffer: ByteBuffer) -> EventLoopFuture<Void>? {
return chunkHandler(buffer)
}
func didFinishRequest(task: HTTPClient.Task<Void>) throws -> Void {
return ()
}
}
let body: HTTPClient.Body = .stream(length: 50) { writer in
do {
var download = try Request(url: "http://localhost:\(httpBin.port)/events/10/1")
request.headers.add(name: "Accept", value: "text/event-stream")
let delegate = ProxyingDelegate { part in
writer(.byteBuffer(part))
}
return httpClient.execute(request: download, delegate: delegate).future
} catch {
return httpClient.eventLoopGroup.next().makeFailedFuture(error)
}
}
let upload = try httpClient.post(url: "http://localhost:\(httpBin.port)/post", body: body).wait()
wdyt?
Will this http client supports SNI (Server Name Indication)?
If it doesn't, it should: SwiftNIO has all the hooks for it.
1 Like
Yes, cool, I think that API will work. We should update the proposal with this.
So this seems to gave read on whether the Channel is writable (which is what the BackpressureHandler that comes with NIO does). For the HTTP client, back-pressure will work a little bit differently. The easiest implementation will be to block the reads until the closure returned from didReceivePart is fulfilled. So something like the following pseudo-code:
func channelRead(...) {
self.mayRead = false
self.delegate.didRecievePart(...).whenComplete {
self.mayRead = true
if self.hasReadBeenHeldUp {
context.read()
}
}
}
func read(...) {
if self.mayRead {
self.hadReadBeenHelpUp = false
context.read()
} else {
self.hasReadBeenHeldUp = true
}
}
My main feedback is on the top level APIs for making requests. Depending on the intended audience (backend vs. frontend) for the library, it may or may not all be applicable.
Rather than offer verb-based API like get(...), post(...), or put(...), the library would be better served to focus its efforts on offering API that makes generic composition of requests easy and encourages that use. I base this on my experience as a user of AFNetworking and the developer of Alamofire.
AFNetworking offered a similar top level API to what's described here: HTTP verb-based top level methods with an NSURLRequest based method that the others usually called into. While this made discovery of how to send different types of request rather easy, since it they showed up in autocomplete, it usually made the networking backends based on AFNetworking a hard to maintain mess. This occurred in several ways. First, the different verbs usually bubbled up the API stack to higher level consumers, as the methods called were all separate at the bottom layer. This lead to the exposure of API details to high level consumers. Second, given that this type of API was what the user first saw, it sometimes lead to the creation of higher level APIs that recreated lower level APIs that already existed but which the user was unfamiliar with. For instance, if users wanted to abstract by verb, they may create their own verb enum that wraps these methods, rather than using the types built in. Third, the lack of any request abstraction from the language or library lead to every consumer building their own interface on top of these methods, even if they did find the more general NSURLRequest method instead of the verb specific ones. That was a lot of code that could've been saved if only for the right abstraction.
On the other hand, Alamofire offers both the URLConvertible and URLRequestConvertible protocols that enable users to create their own generic abstraction on top of making requests, in addition to flexible request(...) APIs that allow the passage of individual request components or a URLRequest. While many users rarely venture beyond the "Router" example in our documentation, any sort of flexible abstraction can be built, as long as the end result produced a URLRequest. This does mean that users must be more aware of the verbs they're using when making request, and the structure of requests in general, but we trade that initial complexity for far more flexibility for advanced users.
So personally, what I'd like to see from a general HTTP request API is less focus of verb-specific APIs and more on enabling generic abstractions for composing requests and then performing them. I think this shift would both encourage better practices for code design and use, as well as enabling higher level abstractions out of the box, so that users can integrate their request code into the rest of their system in the way that fits best.
7 Likes
Thanks for the feedback!
I think that intended audience for this library are backend developers. In my experience, people will add those helper methods anyway, so I don't think there is much harm in having them. Also, those methods are really limited, you cannot set headers there, for example, so for anything even a slightly more complicated users will have to use execute(request: HTTPClient.Request) method. I personally am not against the deletion of those methods.
Do you think that execute(request:) method and corresponding Request struct are ok in terms of API?
For those who haven't subscribed to the GitHub repo, there's a PR up adding support for streaming uploads, and improving the streaming downloads API to support propagating backpressure. Comments very welcome.
1 Like