Remove eventLoop property from Task

[PopFlamingo]

Currently, Task has an eventLoop stored property, and its channel must be on this same event loop.
In practice, eventLoop can be used by the http client delegate in the following ways:

1. Return an immediately succeeded future in the appropriate methods when they don’t want to apply back pressure.
2. Schedule backpressure related work on this event loop and return the backpressure control future.
3. In case backpressure work is done on another event loop (which might be more often the case than Update readme with simple tutorial #2), hop the resulting backpressure future to eventLoop.

In these three cases, the goal to guarantee to the http client that it can assume all futures it gets from its delegate are already on the right event loop.

Potential shortcomings

Errors on user side

When applying back pressure, users getting futures from different event loops might forget to hop them back to the right event loop. On the other hand, if the client stopped relying on the assumption that futures returned from the delegate are already on the right event loop, and instead always took care of hopping them, this source of bugs would disappear entirely for a minimal performance cost (hopTo isn’t too heavy, and in many cases the user will need to use it either way, so why force them to do it when we could do this on the library side?).

Hard to compose with future evolutions

The eventLoop property might not compose very well with future evolutions of the client such as pooling (tldr: the associated event loop is hard to determine synchronously like Task needs, and Task might change of event loop during its lifetime):

• It complicates the connection generation logic on the pool side. For instance a connection pool would likely contain a method such as getConnection(for request: HTTPClient.Request -> EventLoopFuture<Connection>, this is logical for it to return a future because a connection might not be instantly available, but it couldn’t be used as such with the current AsyncClient public API: execute() synchronously returns Task, but Task needs an eventLoop on initialization, which couldn’t be instantaneously determined due to getConnection being an async method. There would of be workarounds for this, but they would complicate the connection pool logic (maybe not a really good sign when two isolated components start to interact this way) and may also force creating new connections (instead of using released ones) just to satisfy the event loop identity constraint.
• Pooling will also need a retry logic for when a connection dies too soon, this means associating the Task with a new channel that will probably not be on the same event loop as before except if we always made a new connection for retries, but it would be a ressource waste.

Proposed solution

Remove eventLoop property entirely, always hop futures provided by the delegate, additionally, update HTTPClientResponseDelegate backpressure controlling methods to make them return EventLoopFuture<Void>? instead of a non-optional EventLoopFuture<Void>.

This change would enable the following:

• Reduces the risk of threading issue in delegate implementations
• Makes Task less dependent on a specific event loop, making it easier to evolve in the future.
• Delegate implementations that want to schedule work on the same event loop as channel can still make use of task.channel.eventLoop knowing it might evolve during the Task lifetime.
• Delegate implementations that don't wish to exerce backpressure can express it more clearly/easily by returning nil instead of having to reference eventLoop just to return an immediately completed void future.

What do you think about this? Am I missing some important part?

Thank you

[artemredkin]

One problem I see is that channel property in Task is optional, so in case we want to return a future and don't have our own we cannot guarantee (compile-time wise) that we have a channel. Also, first iteration of back pressure did return optional EventLoopFuture, but it was decided to opt for non-optional variant: #37 (comment)

On the topic of connection generation code, why not add eventLoop property to connection? That way we can create Task on the same eventLoop as connection.

[PopFlamingo]

@artemredkin About channel being optional I would see two options:

• Make eventLoop a public internal(set) var and update it as needed (ie: when the Task changes of event loop)
• Remove eventLoop, pass a Channel to delegate methods (IIRC someone had already proposed it), this shouldn't require to unwrap optionals either.

I have a preference for the second approach because it makes it a little clearer that the Channel can change along Task lifecycle, whereas users might be more tempted to keep long lived references to properties of a Task object, which could lead to bugs due to having a reference to an outdated event loop / channel.

Ok, makes sense for not returning an optional.

About the connection pool, you did mean adding it to the Connection type, right? Actually, the problem isn't to retrieve the event loop associated with a connection, but more to retrieve the connection itself: getConnection(...) would return an EventLoopFuture<Connection>, but since the client execute(...) method synchronously returns a Task, it means we can't init Task with an argument that requires a call to the async getConnection(...).

We could return a EventLoopFuture<Task>, but it seems suboptimal (at the syntax level, at least) since some of its properties are themselves futures. Returning a non-async connection isn't possible either except if we set excessive constraints on the event loop it can belong to: if we already have active connections to server A, we have no way to know in advance the event loop the next released connection will belong to, so there is no way to synchronously return a Connection that would already contain an event loop, except if we specified we only accepted connections from a certain event loop, but it would waste ressources because some available connections couldn't be reused.

[PopFlamingo]

I gave my second idea a shot, here is the diff.

[weissi]

also this discussion here because it's related: #79

[PopFlamingo]

@weissi Indeed, I realise its not a good solution to add Channel to delegates. Do you think the base issue is valid though? TBH I'm not entirely sure myself, but I don't see how to write something clean when Task needs to be returned immediately and needs an event loop to be initialised.

[weissi]

Ok, we need to definitely think about this some more before 1.0.0. I don't have time right this second but will think about it soon. Adding other relevant folks @ianpartridge @tanner0101 @Lukasa to help the discussion.

[weissi]

Some comments

When applying back pressure, users getting futures from different event loops might forget to hop them back to the right event loop. On the other hand, if the client stopped relying on the assumption that futures returned from the delegate are already on the right event loop, and instead always took care of hopping them, this source of bugs would disappear entirely for a minimal performance cost (hopTo isn’t too heavy, and in many cases the user will need to use it either way, so why force them to do it when we could do this on the library side?)

The client should definitely be robust against that, this assumption is flawed. Back-pressure most of the time is exerted from another Channel which might be on a totally different EL.

From what I can tell, having .eventloop on the Task is mostly for convenience. Here's my proposal: We leave eventLoop on Task but we document clearly that it may change. Possibly we could even name it currentEventLoop. And we leave it non-optional as it's useless as a convenience if it's optional.

Because now it can change, we simply do this:

• if we already have a Channel, we return channel.eventLoop from task.eventLoop
• if we do not have a Channel yet, we return group.next() from the HTTPClient's task.

The HTTP client (I'll file a bug about this in a second) anyway needs to tolerate back-pressure futures from random ELs so there's no more room for user error. But in almost all cases, the user would use task.currentEventLoop to make new futures and those would happen to be on the right EventLoop so hop(to:) is pretty much free anyway. If the user really took the future before a Channel got available, sure, then it's from a different EL and hop(to:) will solve this issue.

So tl;dr: Rename eventLoop to currentEventLoop and document very clearly that it may change during the lifetime.

[PopFlamingo]

Thank you! I like this solution, it provides the right mix between correctness and usability in my opinion. Having the freedom of using any event loop (from group.next()) when the connection pool isn't able to synchronously provide this information solves the issue entirely, especially with good documentation.

On the same subject, I realise that Task has a public init(eventLoop: EventLoop), do we actually want to make it public? Even with the current version of the package I don't know what the use case would be?

[weissi]

On the same subject, I realise that Task has a public init(eventLoop: EventLoop), do we actually want to make it public?

Thanks for catching that. I think it's a bug that Task can even be instantiated from outside the package (but obvs I might be wrong ;) ).

[weissi]

CC @artemredkin do you know why the constructor is public?

[PopFlamingo]

The main issues that were presented in this thread have been fixed by #96 and additional discussion is also present in #100.