add NIO event loop as an argument for execute #79

artemredkin · 2019-08-12T14:26:51Z

Addresses #78

weissi

Looks good but needs a test

artemredkin · 2019-08-12T14:28:45Z

@weissi what do you think we need to test? that we use event loop we pass in?

weissi · 2019-08-12T14:29:38Z

@weissi what do you think we need to test? that we use event loop we pass in?

Two things:

that it uses the correct event loop
every API should at least be used by a test case so that there's at least one caller

ldewailly

@artemredkin, could we do the same for execute(request: Request, deadline: NIODeadline?)? In the case of file upload we wouldn't normally use a HTTPClientResponseDelegate

artemredkin · 2019-08-12T14:49:21Z

@ldewailly done

ldewailly

Looks good. Thanks @artemredkin

t089 · 2019-08-12T20:16:55Z

I think in its current state, in case of redirect the provided EventLoop will be ignored.

In general, is this API really a good idea?

I'm thinking about the future connection pooling. What happens if the pooled connection lives on a different event loop? Should the client then not use this connection and create a new one? Or hop to the event loop of the connection and hop back only in the end? What should happen during the delegate callbacks? On which loop will those be executed?

My understanding was: If you want to tightly control on which event loop the client executes, then initialize it witch a single event loop instead of a event loop group. WDYT?

weissi · 2019-08-12T22:01:52Z

I'm thinking about the future connection pooling. What happens if the pooled connection lives on a different event loop?

The pool could be keyed on the event loop too but that would as you point out mean that we possibly create more connections than necessary to avoid hopping.

My understanding was: If you want to tightly control on which event loop the client executes, then initialize it witch a single event loop instead of a event loop group. WDYT?

Good point. That is achievable today through HTTPClient(eventLoopGroupProvider: .shared(myEventLoop)).execute(...) (because every EventLoop is also an EventLoopGroup where .next() always returns itself)

I think I'm with @t089 and am unsure if we really need this API or if we should just use a new HTTPClient with the EL as the ELG if you want to pin the client to a certain EL. Said that, that also isn't great regarding connection pools because that new HTTPClient would then likely have a new connection pool which gets us back to the initial issue that we might create too many connections... @Lukasa thoughts?

Lukasa · 2019-08-14T10:48:10Z

I'm a bit inclined to want to say that the Client should hide this from users.

Creating new clients per event loop seems like an excessive construction. Why not use the event loop as part of the connection key? Users that don't pass an event loop to execute are expressing that they don't care (that is, nil matches everything), but users that do are requesting a pooled connection on a specific loop. If there isn't one, we just manufacture a new connection for them.

This is strictly no less efficient than creating new clients for every loop.

t089 · 2019-08-14T13:58:51Z

Creating new clients per event loop seems like an excessive construction. Why not use the event loop as part of the connection key? Users that don't pass an event loop to execute are expressing that they don't care (that is, nil matches everything), but users that do are requesting a pooled connection on a specific loop. If there isn't one, we just manufacture a new connection for them.

This sounds doable, although I wonder: if we manufacture a new connection for the explicitly requested event loop, should this connection then also become part of the connection pool? Because in that case, the pool would possibly contain connections that live on event loops that are completely out of the control of the Client. Not sure if this would cause a problem?!

It's a bit unfortunate to discuss the impact of this patch on a feature that does not even exist yet. FWIW, in the current state I would say the eventLoop parameter makes sense.

Lukasa · 2019-08-14T14:01:33Z

Yeah, the connection should be part of the pool. Not sure what you mean by an event loop "out of control of the client" though: can you clarify?

t089 · 2019-08-14T14:08:59Z

Yeah, the connection should be part of the pool. Not sure what you mean by an event loop "out of control of the client" though: can you clarify?

Yeah, maybe that is also a non-issue. But I was thinking of this scenario:

Let's say you create the client with eventLoopGroupProvider: .createNew.
You fire a request with an EventLoop-#8 from another MTELG.
The client will pool the connection with the EventLoop-#8
You shut down the MTELG.
Somebody else fires a request to the same server as above without specifying an event loop.
Would the client now try to use a connection that lives on an event loop that has been shut down?

Lukasa · 2019-08-14T14:09:54Z

Yeah, good call, I think the client should confirm that the event loop for the connection is within its event loop group.

t089 · 2019-08-14T14:15:10Z

Yeah, good call, I think the client should confirm that the event loop for the connection is within its event loop group.

So, should connections on "external" event loops become part of the pool or not?

Lukasa · 2019-08-14T14:16:49Z

The client should refuse to make them, to avoid the question being relevant. 😁

t089 · 2019-08-14T14:24:47Z

Ahhhh ok, I totally missed that use case: You created the client with an ELG and you want to make sure that a subsequent request is executed on a specific EL of this ELG. Yeah, that's a very good use case. 👍

artemredkin · 2019-08-14T14:55:09Z

is there a way to test if EL is part of our ELG?

Lukasa · 2019-08-14T14:56:47Z

Yes, ELGs expose a property that shows you the loops in them.

PopFlamingo · 2019-08-15T17:38:05Z

For the case of a connection pool, I wonder what's the cost of hopping to a different event loop (even repeatedly) versus generating a completely new connection to match the event loop requirement, isn't the former typically much less expensive than the latter since memory is orders of magnitude faster than network IO? In this case, wouldn't it have the risk of causing worse performance when the user actually thinks they are doing an optimisation?

Maybe we could do the following with the eventLoop argument:

It guarantees the futures returned by the client are all hopped to eventLoop.
It is used as a hint for the client / connection pool: if a new connection needs to be created, it will use eventLoop, otherwise, an existing connection will be used no matter its event loop. This way, we ensure that specifying an eventLoop will always have a good impact.

What do you think?

Lukasa · 2019-08-15T17:44:59Z

In general, yes, for a simple request you probably gain nothing by establishing a new TCP connection on your loop instead of reusing one on a different loop.

However, if you are streaming a very large request or response body, you will need to incur way more coordination overhead, and in those cases it is often worth trying to line up the loops.

I think your idea is good, but we should consider extending the API to allow users who know they want it to demand a connection on a specific loop.

artemredkin · 2019-08-15T19:26:47Z

@weissi added a test
@Lukasa > I think your idea is good, but we should consider extending the API to allow users who know they want it to demand a connection on a specific loop.
But still from the clients ELG, right?

PopFlamingo · 2019-08-15T21:39:43Z

@Lukasa I see, so a finer grained preference?

Why not something such as:

enum EventLoopPreference {
    case indifferent
    case prefers(EventLoop)
    case requires(EventLoop)
}

.indifferent is the same as nil in the current PR.
.prefers(EventLoop) guarantees futures callbacks are executed on EventLoop by hopping them, but imposes no further requirement on connection providers implementations, the goal is to enable them to make the right optimizations without over constraining them.
requires(EventLoop) guarantees futures callbacks are executed on EventLoop by ensuring everything is executed on EventLoop.

.prefers(...) would be destined to cases where the user knows using a specific event loop can offer performance benefits, but also knows that forcing the creation of a new connection would probably outweigh the performance gains. On the other hand, requires(...) forces the creation of new connections, better suited for cases such as the long lived streaming @Lukasa describes.

The current implementation would likely treat .prefers(...) and .requires(...) the exact same way, because it creates a new connection each time, but I'm interested to see this already integrated into the API as I'm not sure of how this could be integrated after the 1.0.0 release without being a breaking change?

weissi · 2019-08-16T08:07:17Z

@adtrevor This might be the right idea. I'm just a little worried that right now we're probably not able to express all the things we want to express in the future and the enum is set once we release 1.0.0. What I would probably recommend (is something like):

public struct EventLoopPreferrence {
    private enum Preferrence {
        case .indifferent
        case .prefers(EventLoop)
        /* this is private, we can extend in the future */
    }
    private var preferrence: Preferrence

    public static let indifferent = EventLoopPreferrence(preferrence: .indifferent)
    public static func prefers(_ eventLoop: EventLoop) = EventLoopPreferrence(preferrence: eventLoop)
}

extension EventLoopPreferrence: NilLiteralConvertible {
   /* make `nil` equal to `.indifferent` */
}

if we do this we support the following on day 1: nil (or .indifferent) and .prefers(myEventLoop) and we don't need to implement .requires(...) yet. We can (in a SemVer minor) add any other thing later such as (bad example) .requiresEitherOr(thisEL, thatEL) without breaking the public API.

Obviously if we think .requires is more useful to start with than .prefers, we can also start with that.

Needless to say this is irrelevant if we don't think we don't need this kind of feature.

Lukasa · 2019-08-16T09:27:18Z

Yeah, I think a finer-grained preference control is good. I like that idea, and think if we go with it we should adopt @weissi's suggestion. I also agree with @artemredkin's reminder that we should only allow users to ask for an event loop from the group the client is using.

PopFlamingo · 2019-08-16T12:25:50Z

@weissi Thank you, I already saw this pattern in NIO and I like it.
I also agree starting with .prefers because .requires may cause noticeable performance losses if used incorrectly with a pool IMO, and we probably want to be able to make the upgrade to an internal connection pool as much transparent as possible.
I'm a little worried about users not realising .prefers still guarantees the futures are already hopped to the loop they specified? Maybe there could be a better name? The docs might also be enough since EventLoopPreferrence is already a quite "advanced" option one shouldn't use without understanding what it precisely does?

artemredkin · 2019-08-18T10:56:52Z

@weissi @adtrevor great idea, added preferrence

Lukasa · 2019-08-19T11:26:54Z

Sources/AsyncHTTPClient/HTTPClient.swift

+        public static let indifferent = EventLoopPreferrence(preferrence: .indifferent)
+        /// Library will try to use provided event loop if possible.
+        public static func prefers(_ eventLoop: EventLoop) -> EventLoopPreferrence { EventLoopPreferrence(preferrence: .prefers(eventLoop)) }
+    }


Preference only has one 'r'.

oops, fixed, thanks!

t089 · 2019-08-19T11:43:58Z

I also agree with @artemredkin's reminder that we should only allow users to ask for an event loop from the group the client is using.

This part is still missing, isn't it? Should we add a test for it?

weissi · 2019-08-19T14:03:37Z

Sources/AsyncHTTPClient/HTTPClient.swift

+        public static let indifferent = EventLoopPreference(preference: .indifferent)
+        /// Library will try to use provided event loop if possible.
+        public static func prefers(_ eventLoop: EventLoop) -> EventLoopPreference {
+            return EventLoopPreference(preference: .prefers(eventLoop))


the parameter shouldn't have a name here, "preference" is duplicated. Should be EventLoopPreference(.prefers(eventLoop))

good idea, fixed!

PopFlamingo · 2019-08-19T14:18:11Z

@artemredkin Thanks, that's great!

…ion that EL must be part of ELG

artemredkin · 2019-08-20T09:24:57Z

@t089 good catch, thank you! updated the PR

weissi

lgtm

t089 · 2019-08-20T15:17:41Z

Note, that redirects will ignore the preference parameter, won't they?

async-http-client/Sources/AsyncHTTPClient/HTTPClient.swift

Line 218 in 8fa354d

self.execute(request: newRequest, delegate: delegate, deadline: deadline)

t089 · 2019-08-20T15:27:26Z

Also in the current implementation .prefers(EventLoop) does what @adtrevor described as .requires(EventLoop): It creates a connection on the requested EventLoop. So shouldn't this be called .requires(EventLoop), then?

PopFlamingo · 2019-08-20T15:33:12Z

@t089 The issue if we use .requires(...) is that when the lib evolves to include pooling, there might be big performance losses for people who still have the .requires(...) option set. That's because it imposes big constraints causing the spin of sometimes useless new connections.

t089 · 2019-08-20T15:39:40Z

Ah got it, thanks!

PopFlamingo · 2019-08-20T16:51:15Z

@t089

Note, that redirects will ignore the preference parameter, won't they?

async-http-client/Sources/AsyncHTTPClient/HTTPClient.swift

Line 218 in 8fa354d

self.execute(request: newRequest, delegate: delegate, deadline: deadline)

Indeed looks like this part is missing?

weissi · 2019-08-20T16:52:55Z

@t089

Note, that redirects will ignore the preference parameter, won't they?

async-http-client/Sources/AsyncHTTPClient/HTTPClient.swift

Line 218 in 8fa354d

self.execute(request: newRequest, delegate: delegate, deadline: deadline)

Indeed looks like this part is missing?

Sorry, I merged because I didn't see this: Created issue #88

add NIO event loop as an argument for execute

5ebd2bd

artemredkin requested review from weissi, ldewailly and Lukasa August 12, 2019 14:26

t pMerge branch 'master' into add_event_loop_in_execute

377b4ef

weissi requested changes Aug 12, 2019

View reviewed changes

ldewailly suggested changes Aug 12, 2019

View reviewed changes

review fix: add to np-delegate method as well

99fed14

ldewailly approved these changes Aug 12, 2019

View reviewed changes

Merge branch 'master' into add_event_loop_in_execute

2eefbc7

artemredkin added 2 commits August 15, 2019 20:00

Merge branch 'master' into add_event_loop_in_execute

61935d7

Resolve confict

cd81083

missing self

b7078e8

weissi mentioned this pull request Aug 16, 2019

Remove eventLoop property from Task #82

Closed

artemredkin added 2 commits August 18, 2019 11:55

review fix: add event loop preference argument instead of eventloop

db26532

Merge branch 'master' into add_event_loop_in_execute

912b5fe

formatting

082cfeb

Lukasa requested changes Aug 19, 2019

View reviewed changes

review fix: spelling

3b30918

Lukasa approved these changes Aug 19, 2019

View reviewed changes

fix compilation error

1036075

weissi reviewed Aug 19, 2019

View reviewed changes

review fixes: make preference argument not explicit and add precondit…

1b7e97e

…ion that EL must be part of ELG

Merge branch 'master' into add_event_loop_in_execute

8fa354d

weissi approved these changes Aug 20, 2019

View reviewed changes

weissi merged commit 64851a1 into master Aug 20, 2019

weissi deleted the add_event_loop_in_execute branch August 20, 2019 16:50

weissi mentioned this pull request Aug 20, 2019

redirects ignore EventLoop preference #88

Closed

add NIO event loop as an argument for execute #79

add NIO event loop as an argument for execute #79

Conversation

artemredkin commented Aug 12, 2019

weissi left a comment

Choose a reason for hiding this comment

artemredkin commented Aug 12, 2019

weissi commented Aug 12, 2019

ldewailly left a comment

Choose a reason for hiding this comment

artemredkin commented Aug 12, 2019

ldewailly left a comment

Choose a reason for hiding this comment

t089 commented Aug 12, 2019

weissi commented Aug 12, 2019

Lukasa commented Aug 14, 2019

t089 commented Aug 14, 2019 • edited Loading

Lukasa commented Aug 14, 2019

t089 commented Aug 14, 2019

Lukasa commented Aug 14, 2019

t089 commented Aug 14, 2019

Lukasa commented Aug 14, 2019

t089 commented Aug 14, 2019 • edited Loading

artemredkin commented Aug 14, 2019

Lukasa commented Aug 14, 2019

PopFlamingo commented Aug 15, 2019 • edited Loading

Lukasa commented Aug 15, 2019

artemredkin commented Aug 15, 2019

PopFlamingo commented Aug 15, 2019 • edited Loading

weissi commented Aug 16, 2019 • edited Loading

Lukasa commented Aug 16, 2019

PopFlamingo commented Aug 16, 2019 • edited Loading

artemredkin commented Aug 18, 2019

Lukasa Aug 19, 2019

Choose a reason for hiding this comment

artemredkin Aug 19, 2019

Choose a reason for hiding this comment

t089 commented Aug 19, 2019

weissi Aug 19, 2019

Choose a reason for hiding this comment

artemredkin Aug 20, 2019

Choose a reason for hiding this comment

PopFlamingo commented Aug 19, 2019

artemredkin commented Aug 20, 2019

weissi left a comment

Choose a reason for hiding this comment

t089 commented Aug 20, 2019

t089 commented Aug 20, 2019

PopFlamingo commented Aug 20, 2019 • edited Loading

t089 commented Aug 20, 2019

PopFlamingo commented Aug 20, 2019 • edited Loading

weissi commented Aug 20, 2019

t089 commented Aug 14, 2019 •

edited

Loading

t089 commented Aug 14, 2019 •

edited

Loading

PopFlamingo commented Aug 15, 2019 •

edited

Loading

PopFlamingo commented Aug 15, 2019 •

edited

Loading

weissi commented Aug 16, 2019 •

edited

Loading

PopFlamingo commented Aug 16, 2019 •

edited

Loading

PopFlamingo commented Aug 20, 2019 •

edited

Loading

PopFlamingo commented Aug 20, 2019 •

edited

Loading