Client waits during connecting

[t089]

Cross-posted: amzn/smoke-aws#18

I am using DynamoDBClient from inside a Vapor app.

Inside a route I call AWSDynamoDBClient.queryAsync(input:completion:)

this will eventually call HTTPClient.executeAsync<A>()

in there we have:

try bootstrap.connect(host: endpointHostName, port: endpointPort).wait()

this raises a fatal error:

NIO-ELT-#1 (13): Precondition failed: BUG DETECTED: wait() must not be called when on an EventLoop.
Calling wait() on any EventLoop can lead to
- deadlocks
- stalling processing of other connections (Channels) that are handled on the EventLoop that wait was called on

Further information:
- current eventLoop: Optional(SelectableEventLoop { selector = Selector { descriptor = 17 }, scheduledTasks = PriorityQueue(count: 0): [] })
- event loop associated to future: SelectableEventLoop { selector = Selector { descriptor = 7 }, 

Probably in a method called Async you should never .wait() ... ?

[t089]

I did some experimentation and unless I missed something (which is very possible, of course) to me this really seems like a programming error: In an async framework waiting for the connection on the calling thread clearly violates the "we do non-blocking io" promise.

The fix seems pretty straight-forward: instead of waiting for the channel, just return the future.

I am not sure why the HTTPClient.executeAsync<A>() returns the channel in the first place, as none of the clients in smoke-aws ever seem to use it. The good news is, this makes this change relatively painless. If you are interested I can setup a PR. It only requires some changes in smoke-http. But as far as I could tell most of the clients of smoke-http ignore the Channel return parameter. So after the change they would just ignore the EventLoopFuture<Channel> so, no source changes are required in other packages 👍 .

[tachyonics]

The motivation for returning the Channel was incase any other consumers needed access to the channel but it makes sense for it to be a future in the non-blocking case. My only concern is making this change in a non-breaking way or making it as part of the next major version (which will be Swift 5/NIO 2). What version of Swift do you need this for?

[t089]

Yes this for sure is a breaking change unfortunately. However, I think it is probably ok to do it even for Swift4/NIO1. As it stands today this HTTPClient is practically unusable from a NIO app, isn’t it? How are you using it in smoke-framework without running in this issue?

This being said, if you do the change (see here), smoke-aws and smoke-credentials will build just fine, because they don’t use the Channel. Therefore, for these packages it would not even be a major version change.

So, my advice would be: fix it in smoke-http with a major version bump and upgrade the dependency in other packages (I don’t know if this would be minor or bugfix for those).

If this plan is not feasible, I’m also happy to maintain my fork for the time being. For now I’m stuck with NIO1 and Swift4.

Alternatively you could also introduce a new executeTruelyAsync method... 🧐

[tachyonics]

SmokeFramework has a slightly different threading model to Vapor. By default, SmokeFramework only uses NIO event loop threads for actual IO, executing operation handlers on the global DispatchQueue[1]. We chose this default partially for this reason - NIO threads aren't really general purpose and have restrictions on what you can and cannot do on them (calling wait for example). Calling out into unknown (from the framework's perspective) application code is probably best handled on a general DispatchQueue, at least as a default. And so we have services using SmokeFramework happily running in production without running into this issue.

That said, it is definitely a bug that you cannot call into HTTPClient within NIO's event loop if that is what you want to do. I think the best option is probably to introduce parallel methods that return the future of the Channel and move SmokeAWS to use these methods for compatibility with Vapor.

[1] https://github.com/amzn/smoke-framework/blob/master/Sources/SmokeOperations/GlobalDispatchQueueAsyncInvocationStrategy.swift

[tachyonics]

A fix for this issue was committed to master in #37.

This has now been released as 2.0.0.

Resolving this issue. Let us know if there are any further issues.

[t089]

Nice! Thank you!