Cannot cancel inside delegate about async-http-client

[ianpartridge]

Happy to PR this, shouldn't be too hard (famous last words).

[artemredkin]

I'll implement it today-tomorrow and will play with failure modes to see what we can do here

[artemredkin]

I've implemented a prototype, it is indeed potentially dangerous. If one returns .continue when .end or error are received, future will never be completed. Only option is to complete promise with some kind of ReturnedNextOnEnd error if we receive .continue on .end, but this is a runtime error, which is not great in already runtime-error-filled environment of async frameworks (with didReceiveEnd you just cannot not provide a result)... One solution here would be to drop promise from HTTPHandler but in this case users of the library will have to provide a promise, which is not great (you cannot just create a promise, you need access to an EventLoop). And it could make code that handles redirects to a different URL origin more complex (I'm not yet sure about that, will test it today).
Having state is also not super convenient with this type of functional consumer. With delegate its easier to re-use due to state encapsulation, but in case of a function, you have to have state be captured by a closure, and this is messy, imho, especially if you have more than one variable to capture. One can create a class for that state, but by this point you already have a class, so why not just conform to a protocol and use class as a delegate instead.
I will experiment with dropping promise from HTTPHandler to see if it has potential. Also, I will experiment with having only one function in a protocol (similar to RxSwift observer).

[weissi]

thanks @ianpartridge . The delgate should be handled the HTTPTask as the first parameter. That's I think also the usual pattern in Cocoa.

[artemredkin]

I'm thinking about re-structuring streaming API a bit and want to do 'internal pitch' here on in slack about possible improvements to it. Depending on outcome, API could change slightly. What are your general thoughts about this delegate API?

[ianpartridge]

In general it feels more Cocoa-y than Swift-y, if that makes sense. But you only have to use the delegate if you want to do streaming, which is nice. I haven't really got enough experience with the package yet to have views on improvements. What changes do you have in mind?

[artemredkin]

Delegate has a downside of not being able to implement it inline, and that it's exactly as you say - not "swifty". I think it's even more java-like (this is were most of my experience comes from :) ). I was thinking maybe something like a singe method with an enum will work better:

execute(request: request) { event in
    switch event {
        case head(let head):
            // process head
        case body(let part):
            // process body
        case error(let error):
            // handle error
    }
}

[ianpartridge]

I suggested something a bit like that to @weissi but he asked how cancellation could work?

[artemredkin]

You mean inside this closure? we can have two parameters there, event and task for example, or we can return something like .contunue or .abort from it

[ianpartridge]

I like the return .abort idea :) @weissi?

[weissi]

@artemredkin you also need to return a cancellation token for outside the closure...

[artemredkin]

@weissi that part won't change

[weissi]

@artemredkin ah so it'll be

let task = client.get(...)
someQueue.dispach(after: .now() + 2) {
    task.cancel()
}
task.execute { event in
    switch ...
}

[artemredkin]

no, something like this:

let task = client.execute(request) { event in
    switch ...
}
someQueue.dispach(after: .now() + 2) {
    task.cancel()
}

[weissi]

@artemredkin cool, if we also hand the task to the closure, then I'm happy with this too. @Lukasa ?

[artemredkin]

@weissi what about returning .continue, .done(Result) and .abort instead of passing in task?

[weissi]

@artemredkin sure, we can do that. That would also allow us to support back-pressure, what does the .done(Result) do?

[artemredkin]

.done(result) is to return type-safe value as a result of this function, for example:

let count: Int = try client.execute(request) { event in
    var count = 0
    switch event {
        case .body(let part):
            count += part.availableBytes
        case .end:
            return .done(count)
        default:
            break
        return .continue
    }
}.wait()

[weissi]

@artemredkin, so .done == .abort except that it puts a value in the future and abort fails it?

When do you fulfil the future if I do

let count: Int = try client.execute(request) { event in
    return .continue
}.wait()

? Ie. what happens if I never return .done or .abort?

[artemredkin]

hmmm, good point...

[artemredkin]

I'll never be able to provide a value for promise...

[weissi]

I'll never be able to provide a value for promise...

AFAIK the only illegal thing would be .continue on .end and in that case we could just fail the promise maybe?

[weissi]

@artemredkin we could also treat .continue on .end like .done?

[artemredkin]

@weissi the way it is designed right now (not sure if it's the best approach) - we have to fulfill a promise with some kind of value, right now we have promise.succeed(delegate.didFinishRequest())