Sanic: Sanic _violates_ HTTP, fails with HTTP 1.1 pipelining

[squeaky-pl]

HTTP 1.1 improves performance upon HTTP 1.0 in two main areas:

1. Keep-alive. Possibility to send many (request, response) pairs on the same connections
2. Pipelining. Possibility to send many requests without waiting for a response after each of them. The responses must be returned in the same order that requests arrived. https://en.wikipedia.org/wiki/HTTP_pipelining

HTTP/1.1 conforming servers are required to support pipelining. This does not mean that servers are required to pipeline responses, but that they are required not to fail if a client chooses to pipeline requests.

Sanic currently doesnt support pipelining, worse it will fail very badly by dropping responses if the client attempts pipelining. This is a HTTP 1.1 protocol violation. If sanic doesn't support pipelining it should at least not consume new requests on the same connection until generating corresponding responses. This way pipelining clients will not break.

At the moment pointing pipelining client at sanic results in a stream of errors in error log:

2017-01-24 01:54:53,266: ERROR: Writing response failed, connection closed 'NoneType' object has no attribute 'should_keep_alive'
2017-01-24 01:54:53,266: ERROR: Writing response failed, connection closed 'NoneType' object has no attribute 'should_keep_alive'
2017-01-24 01:54:53,266: ERROR: Writing response failed, connection closed 'NoneType' object has no attribute 'should_keep_alive'
2017-01-24 01:54:53,267: ERROR: Writing response failed, connection closed 'NoneType' object has no attribute 'should_keep_alive'
...

You can reproduce issue with wrk passing following script into it:

-- pipeline.lua, example script demonstrating HTTP pipelining
-- based on: https://github.com/wg/wrk/blob/master/scripts/pipeline.lua

init = function(args)
   local r = {}
   r[1] = wrk.format(nil, "/")
   r[2] = wrk.format(nil, "/")
   r[3] = wrk.format(nil, "/")
   r[4] = wrk.format(nil, "/")
   r[5] = wrk.format(nil, "/")
   r[6] = wrk.format(nil, "/")   
   r[7] = wrk.format(nil, "/")
   r[8] = wrk.format(nil, "/")

   req = table.concat(r)
end

request = function()
   return req
end

and run it so:

wrk -t 1 -c 100 -d 10 -s pipeline.lua http://localhost:8000
Running 10s test @ http://localhost:8000
  1 threads and 100 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     0.00us    0.00us   0.00us    -nan%
    Req/Sec   530.40    184.91     1.02k    75.00%
  5242 requests in 10.08s, 0.00B read
  Socket errors: connect 0, read 2621, write 0, timeout 0
  Non-2xx or 3xx responses: 2621

Note that half of the traffic failed.

The script instructs wrk to send 8 requests in parallel over the same connection.

Said all that Sanic should support pipelining because it can improve performance (nginx can serve static content twice as fast if client pipelines requests).

[squeaky-pl]

Note that meinheld also does fail this test. Aiohttp, Tornado. Gevent, Twisted work correctly.

[squeaky-pl]

A naïve but effective solution to solve this problem is maintaining some kind of per connection "queue", list or deque might be fine actually because typically it will be a very small "queue". Whilst requests arrive on the open connection each of the created tasks land in the "queue". Also each of the request tasks has an attached on_done_callback. Each time on_done_callback is fired you walk the queue from the start in order checking if each of the tasks has already finished. If it has finished you write back the response bytes, remove the task from the queue and proceed with next task on the queue. Whilst if you find a task that has not yet finished you just return from the callback. Once each of the queued tasks finishes all of the responses are on the connection and written out in the right order.

[seemethere]

Yeah I like that idea, a queue is definitely the right tool for this job. I'll look into after this week.

[squeaky-pl]

Note: I also probed nodejs and go http servers yesterday and they pass this test, they actually perform slightly better (10% faster) under pipelining than under non-pipelined load.

[ubergarm]

Nice article, @squeaky-pl , japronto looks very cool!

Pipelining seems to improve benchmark performance significantly in some go framweworks e.g. fasthttp.

However, it may be prudent to just say "Sanic does not support pipelining", as its questionable whether it gives real performance improvements in modern browsers especially given HTTP/2 multiplexing is probably the way to go in 2017.

It is important to define a project by what is does not do as well as what it does do.

Personally, I'm fine with Sanic explicitly not supporting pipelining so long as it doesn't break pipelining clients.

Though if you want pretty looking benchmark results.... ;)

[1st1]

However, it may be prudent to just say "Sanic does not support pipelining"

IIRC Sanic must support pipelining since it's an HTTP 1.1 requirement for servers.

[amsb]

HTTP/1.1 conforming servers are required to support pipelining (FAQ) but in practice many browsers don't have pipelining enabled although it seems mobile browsers are more likely to have it enabled. According to the spec, clients must be prepared to retry and resend unfulfilled requests which opens the door for the server to just return the first request then close the connection with the client as a fallback mechanism.

Another practical consideration is what happens when you put Sanic behind a load balancer or other proxy. AWS load balancers, for example, support pipelined HTTP on front-end connections but do not support pipelined HTTP on back-end connections which I interpret to mean they terminate pipelining at the load balancer (someone correct me if I'm wrong!).

Indeed, the fragility and general lack of support for HTTP/1.1 pipelining is one of the motivations for multiplexing in HTTP/2 (and also the stack overflow link above).

[1st1]

@amsb Are you advocating for Sanic not supporting pipelining (it's not clear from your comment)? If yes, then:

• What difference does it make that most clients don't use it?
• Why do we discuss load balancers? There is non-zero number of apps deployed behind no balancers.
• Why do we discuss HTTP/2 here?

The discussion is about HTTP 1.1 spec and Sanic not implementing the spec correctly (according to @squeaky-pl )

[amsb]

@1st1 I am not advocating for non-conformance although maybe unintentionally rationalizing it. :) Not conforming to the standard would likely undermine adoption. I just wanted to share some practicalities around the issue of pipelining.

[seemethere]

@1st1 if most clients don't use it then I would say that the feature should be put on the back-burner until we finish features that more users need.

[sjsadowski]

Missed this in initial cleanup. Closed per #423