programming model questions

[weissi]

Right now, the delegate's methods are calling out on essentially a random EventLoop.
I think we have the guarantee, that each delegate method will at least be called on the same EventLoop?

I realise that I proposed the change to have task.currentEventLoop (which can change) but I'm not sure anymore that was a good idea. I didn't quite realise that this means that the user has no idea on what EventLoop their delegate's methods will be called on.

Maybe it would be better to go back to having a fixed EventLoop on the task and hop over in case we get a Channel from a different EventLoop from a connection pool.

The other dimension where this matters is the EventLoopPreference, that also kind of means a random EventLoop. Maybe we should just have the user specify the EventLoop they want (if they care, random one otherwise) and use that. If we get a Channel from a different EventLoop then let's just hop(to:) the correct one.

This is just an idea really but I was thinking about the programming model earlier today and 'essentially a random EventLoop' sounds like it might lead to a lot of threading bugs?

Tagging the relevant people @adtrevor / @ianpartridge / @tanner0101 / @Lukasa / @artemredkin

[ianpartridge]

This sounds sensible to me.

[PopFlamingo]

If we was to always call delegate methods on a specified event loop, I am worried about the performance impact this may have. For instance, when a connection pool is added, there are cases where the most efficient solution for the pool will be to return a channel that isn't on the fixed event loop, meaning hopping would have a non-negligible performance impact from what I understand.

Looking at how ResponseAccumulator or the internal CountingDelegate work, there is no risk of threading issue even if methods weren't to be called on the same event loop during the task lifetime, this is because they are tied to a specific task and they don't span any additional asynchronous work on the current event loop.
Obviously that's specific to these delegates, and there are high chances other implementations would be problematic, but this makes me wonder if this isn't something that should still be explored further: AFAICT, it looks like delegate implementations that are tied to a specific task are much less likely to encounter threading issues, what if the client evolved in a way that encouraged making one delegate instance per task?
That's just an idea and maybe there are drawbacks I didn't think about, but since I see ResponseAccumulator works this way, I wonder if having one delegate instance per task isn't something quite common actually?

[weissi]

Thanks for the input @adtrevor & @ianpartridge ! That all makes sense.

@adtrevor So I think in most cases, it's much more important to re-use a Channel than it is to not hop the EventLoop. However it's true that there are use-cases (proxies, uploading massive files, ...) where a new connection might be better than constantly hopping EventLoops.

But, the programming model is also really important.

I struggled to come up with something that would satisfy all use-cases and yet have a usable yet sensible API. Fortunately, I dropped by @Lukasa's desk and I think after talking about lots of that stuff it essentially boils down to the following options:

1. I don't care about the EventLoop at all ➡ no preference.
2. I do care about the delegate's EventLoop but not such much about the Channel's EventLoop ➡ I require a certain EventLoop for the delegate and would prefer the Channel to be colocated
3. I do care about the delegate's EventLoop and the Channel's EventLoop to be the same ➡ I require the EventLoop of the delegate invocations and the Channel to be the same.

We believe those 3 options really cover all reasonable options. Did we miss anything?

The good news here is that with minimal API changes, we could make this work. The required changes would be:

• switch currentEventLoop back to EventLoop and that EventLoop is the one the delegate is invoked on
• change EventLoopPreference from .indifferent and .prefers(EventLoop) to .indifferent, .delegate(on: EventLoop), and .delegateAndChannel(on: EventLoop)

That way we can model all the scenarios above (.indifferent is 1, .delegate(on:) is 2, and delegateAndChannel(on:) is 3) with minimal API changes. I would also shim the current API using deprecated functions to ease the transition from alpha.3 to alpha.4.

How does that sound?

[weissi]

@adtrevor what do you think about #102?

[PopFlamingo]

You can ignore my previous comment if you saw it. I was worried about a use case and did not realize this one was correctly solved by the .indifferent option.
Just wanted to know if .indifferent guarantees that all methods for a given task will be called on the same event loop?

I agree the multiple use cases mean there is no single solution that would fit all and that a finer control is needed to get the best performance, nothing seems to be missing AFAICT and yet it’s indeed a nice to use API 🙂

[Lukasa]

Just wanted to know if .indifferent guarantees that all methods for a given task will be called on the same event loop?

Yes, that would be the guarantee: the client would select an event loop for you.

[t089]

Just wanted to know if .indifferent guarantees that all methods for a given task will be called on the same event loop?

Yes, that would be the guarantee: the client would select an event loop for you.

But, the event loop might still change during the lifetime of the request, correct? For example, if a redirect to a different host would create/reuse a connection/channel that lives on a different event loop.

[Lukasa]

Sorry, we need to clear the language up here. There are two event loops in play. The first is the event loop used by the Channel that is serving the current request. The second is the event loop that is used to provide the implicit mutual exclusion context for the delegate (that is, the event loop on which all delegate callbacks run).

In this case, the loop that belongs to the Channel serving the request may change, but the loop that is used for the mutual exclusion context for the delegate may not.

[t089]

In this case, the loop that belongs to the Channel serving the request may change, but the loop that is used for the mutual exclusion context for the delegate may not.

Got it 👍

[PopFlamingo]

@Lukasa

Yes, that would be the guarantee: the client would select an event loop for you.

Thanks! That's great! I realise the only additional options that could be added would only make a really small difference and they would apply to extremely specific situations, also they could be additive changes so the options as they are right now seem perfect to me 🙂