async server by tanner0101 · Pull Request #106 · swift-server/http

tanner0101 · 2017-12-06T23:23:07Z

An alternative to #96. I ported over Vapor 3's async server.

~/dev/swift-server/http server$ wrk -t 4 -c 128 -d 5 http://localhost:8080
Running 5s test @ http://localhost:8080
  4 threads and 128 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     1.92ms  263.04us  12.15ms   75.57%
    Req/Sec    16.72k     0.98k   19.93k    72.55%
  339444 requests in 5.10s, 28.49MB read
Requests/sec:  66557.73
Transfer/sec:      5.59MB

Performance isn't terrible, but also not great. About the same as the blocking implementation, although that's expected. We've seen >100k req/s in Vapor 3 (even including router, middleware, etc). This could be due to some inefficiencies in the HTTP parsing/serialization or in how I hooked things up.

The keep-alive test wasn't passing (couldn't really figure out what was going on there). It also seems like the streaming parser has some locks in it which will probably need to be removed.

Ideally all of this will be done much better when the Transport module comes out, but perhaps this will at least be a better option than the current blocking server. (Also weighs in at slightly less code)

helje5

It would be cool to know if you could explain what you don't like about #96 that you propose a different implementation. Anything in particular?
But I'm fine to close #96, looks like there is no interest in taking it anyways :-)

But wrt this imp, I see a few issues:
a) apparently handlers cannot actually be async?
~~b) accept congestion~~
c) no back pressure support (we can as well drop it from the API then)
d) quite a lot of copying of buffers
e) no pipelining?

a, c & e are probably the worst parts. I guess b) is not cool but you could probably live with in a simple baseline framework, but d) at least needs to suspend parsing until the first request has been processed (otherwise you get interleaved response output?).

But as far as I see it, a) is completely unsupported aka this is not an async implementation of the API? Probably I'm missing something? How do you do this standard async server example with this implementation?: https://github.com/ZeeZide/http-testserver/blob/master/Sources/http-testserver/main.swift#L111
There seems to be no way to dispatch back to the response writer queue?

helje5 · 2017-12-07T00:07:22Z

+        }
+        let server = TCPServer(socket: socket, eventLoops: eventLoops)
+        print("Starting on http://localhost:\(options.port)/")
+        try server.start(hostname: "0.0.0.0", port: UInt16(options.port), backlog: 4096) { client in


I mean you use a (unreasonably?) huge backlog, but this seems to use a single accept queue. Since accept can take a moment (TCP handshake and all that crap, let alone a TLS driver ;-) ) and is a blocking call, I wonder whether this is viable.

we use O_NONBLOCK so the accept should be nonblocking.

This is of course completely right. 🤦‍♀️ :-)

helje5 · 2017-12-07T00:11:08Z

-        server.stop()
+extension TCPClient: ParserConnecting {
+    func queueSocketWrite(_ from: Data, completion: @escaping (Result) -> Void) {
+        write(from)


Unless write is synchronous (contrary to the goal here), you cannot call completion until the call actually did complete. Otherwise you can't maintain back pressure and you are just overflowing the write buffer until no memory is left?

shouldn't be too hard to add that :)

Yeah, that probably isn't hard by itself (just queue them up and call them when the writes are done). The hard part (IMO) are the synchronization points for the closures in question as discussed on list. Hence the new queue argument to the handler (you can make it free threaded, but that would be pretty expensive).

Also, I'm not 100% but I think you may have the same issue on the reading side? (you need to pause reading until the bodyProcessor calls the done CB, e.g. to make this work reliably and at scale: https://github.com/ZeeZide/http-testserver/blob/master/Sources/http-testserver/main.swift#L61) Right?

Yeah I think you're right. We should definitely be passing the DispatchQueue around w/ the request and response writer.

helje5 · 2017-12-07T00:26:22Z

-        try server.start(port: options.port, handler: handler)
+        let socket = try TCPSocket(isNonBlocking: true, shouldReuseAddress: true)
+        var eventLoops: [DispatchQueue] = []
+        for i in 1...8 {


there are quite a few places i used magic numbers while porting this over. (like the accept queue, and read size as well) those can be easily fixed if people like this

tanner0101 · 2017-12-07T04:28:44Z

@helje5 when did I say I don't like #96? Just trying to contribute back the work we are doing in Vapor to the Swift server group. 👍

helje5 · 2017-12-07T08:25:46Z

+        try server.start(hostname: "0.0.0.0", port: UInt16(options.port), backlog: 4096) { client in
+            self.connectionCount += 1
+            client.onClose = {
+                self.connectionCount -= 1


Isn't this running in a different thread and needs an atomic guard? 🤔

Ah yes. That should be dispatched async to the server's queue. What is the connection count used for btw? Do we need that?

Yeah, I think a count is pretty useless. A connection list could be useful for debugging.

helje5 · 2017-12-07T09:24:13Z

So what about

a) apparently handlers cannot actually be async?
Which I assume is the most important part when doing 'async server' :-)

Even ignoring the pipelining issue (aka multiple handlers running on the same connection), as far as I can see neither StreamingParser nor TCPClient is free threaded (e.g. inputBuffer would be filled from arbitrary queues while it is concurrently being touched by the writer queue?)?
Or do I miss something here?

helje5 · 2017-12-07T09:41:43Z

+                    let buffer = UnsafeBufferPointer<UInt8>(start: pointer, count: data.count)
+
+                    do {
+                        let length = try self.socket.write(from: buffer)


I think you have a low-hanging fruit here. Instead of doing individual writes, use writev w/ the array of buffers.

tanner0101 · 2017-12-07T18:43:27Z

as far as I can see neither StreamingParser nor TCPClient is free threaded (e.g. inputBuffer would be filled from arbitrary queues while it is concurrently being touched by the writer queue?)?

Each client/connection gets its own StreamingParser which in turn is only ever used on that client's queue. There would be problems here if the request handler starts dispatching writes on other queues, but we can remedy that by passing the queue into the handler and requiring that the response writer be called on it as you mentioned.

Since the StreamingParser is dedicated to a connection (and thus a dispatch queue), there should be no problems there with the pipeline as far as I can tell. That seems like something that would have cropped up in the wrk tests I ran at least.

helje5 · 2017-12-07T20:05:02Z

There would be problems here if the request handler starts dispatching writes on other queues

Yet that is the only way you can do anything asynchronous in your current setup, dispatch to a different queue? :-) But yes, either adding the queue to the handler (and requiring API calls on them) or guarding the API methods themselves is the solution to this.

Since the StreamingParser is dedicated to a connection (and thus a dispatch queue), there should be no problems there with the pipeline as far as I can tell.

The queue doesn't help you here because the handlers are asynchronous (their execution can interleave on the same queue).
With pipelining you receive N requests on the socket, in sequence. In your setup all those requests should be immediately parsed and reported to a handler, creating new invocations of the handler for each incoming request(, on the same queue, doesn't matter). Now handler 2 cannot be allowed to write to the actual socket or socket buffer, before handler 1 didn't call done on its response writer.

There are multiple solutions to this, but I think your current thing will just produce corrupted and interleaved responses. In #96 I'm 'corking' response-writers which are not ready yet, another (non-perfect, but okay'ish) option is to essentially disable pipelining by pausing the parser until an in-flight response is done.

(in case this isn't obvious - in the original version this was a non-issue because it was purely synchronous, but that setup isn't suitable here)

That seems like something that would have cropped up in the wrk tests I ran at least.

You didn't show those, are they doing anything asynchronously like the sample I've send? If you are not running an actual async test, you are not going to run into async issues :-) Also, is wrk actually using pipelining here? Probably not, it is trying to measure the requests themselves, but maybe it has an option for that.

But this is a good point, we need actual async tests to test the scalability. Like simulating a database call before returning the response (e.g. using a timer, or a background concurrent queue w/ actual sleeps()), or proxying large requests (would be nice to check out memory consumption).
I created the http-testserver package w/ that goal, but it is not very elaborate yet (in part because interest on #96 seemed to be low)

tanner0101 · 2017-12-07T21:47:59Z

With pipelining you receive N requests on the socket, in sequence. In your setup all those requests should be immediately parsed and reported to a handler, creating new invocations of the handler for each incoming request(, on the same queue, doesn't matter).

Oh wow you're right. TBH I am very surprised to learn that HTTP/1.1 supports pipelining. Looks like very few people actually support it, but obviously Swift Server should. I will take a look at your solution in #96 when I have some time!

helje5 · 2017-12-07T21:58:05Z

Note that I do not suggest that we have to actually support pipelining. As you say it is not usually enabled in web browsers, though it does have some real world relevance with HTTP APIs (and maybe proxies). The proper fix is HTTP/2, though this would still need a pretty similar decoupling to what I do in #96.

But what we should not do is fall apart if a client does pipelining. That should at least suspend the read end of the socket until the active handler is done. I think that would be quite acceptable.

tanner0101 · 2017-12-07T22:18:58Z

Adding back pressure support here could potentially kill two birds with one stone. With back pressure, the streaming parser could wait to accept additional input until it has completed serializing the response.

ianpartridge · 2018-10-29T16:05:12Z

Closing as this project is historical, per the README.

tanner0101 added 7 commits December 6, 2017 18:11

async server

ab0db4d

remove address struct

d415607

uncomment tests

16c47bf

remove runloop

02ffff6

fix linux tcp constants

db61e34

remove vapor comments'

3b47f46

missing . in glib.accept

5bb9454

helje5 reviewed Dec 7, 2017

View reviewed changes

ianpartridge closed this Oct 29, 2018

Conversation

tanner0101 commented Dec 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

helje5 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tanner0101 Dec 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tanner0101 commented Dec 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

helje5 commented Dec 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tanner0101 commented Dec 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

helje5 commented Dec 7, 2017

Uh oh!

tanner0101 commented Dec 7, 2017

Uh oh!

helje5 commented Dec 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tanner0101 commented Dec 7, 2017

Uh oh!

ianpartridge commented Oct 29, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tanner0101 commented Dec 6, 2017 •

edited

Loading

helje5 left a comment •

edited

Loading

tanner0101 Dec 7, 2017 •

edited

Loading

tanner0101 commented Dec 7, 2017 •

edited

Loading

helje5 commented Dec 7, 2017 •

edited

Loading