Switch to asyncio streams API

[florimondmanca]

Refs #169

Okay, this is an ever more stripped-down alternative to #867 that I think we can consider for merging (at last!).

Description

This PR switches the Server from starting asyncio servers using protocols, to starting asyncio servers using a streams handler (protocols API -> streams API).

The rest is glue code: we pass a handler that defers to our existing HTTP protocol classes. It's a bit of a clever hack, but I've left comments that I hope are sufficiently helpful.

Motivation

Switching to the asyncio streams API is required before abstracting away asyncio specifics, and introducing trio or curio support.

This "reuse existing protocols implementations" is actually nice, because we get to keep the existing asyncio-optimized code, yet this paves the way for async agnostic support.

Next steps

The next steps after this PR would be:

• Make server startup code async-agnostic, keeping a "fast track" for asyncio. Abstract asyncio away in server management #870
• Start adding async-agnostic support for trio/curio through a new implementation. (Eventually we can also consider having asyncio in there, as a new implementation.)

[florimondmanca]

@euri10 What are we thinking about this…?

I'd love for this small bit to get in for 0.14.0. We could also some kind of public beta thing so people try it out in production settings. I could understand that we'd be a bit wary about this "run protocols from streams" thing, so that could help us validate if that's a sensible thing to do / doesn't come with strange edge cases.

(Personally I'm quite confident that it can work just fine, just saying I would understand if we'd want to be more risk-averse.)

[euri10]

I think it's worth taking a shot at it @florimondmanca

It looks like a good 1st step towards introducing trio,

I just have a comment below, I'm not sure at all about its pertinence, but I better look stupid than say nothing, but I'm always kind of careful when we speak about read and write buffers and networking, it reminds me nightmare memory issues 🦞

[euri10]

Interestingly, I played a little with it and get not a bug but something we've been on recently:

❯ uvicorn app:app --loop=asyncio
INFO:     Started server process [1350]
INFO:     Waiting for application startup.
startup lifespan
INFO:     Application startup complete.
INFO:     Uvicorn running on http://127.0.0.1:8000 (Press CTRL+C to quit)
Task was destroyed but it is pending!
task: <Task pending name='Task-5' coro=<Server.startup.<locals>.handler() done, defined at /home/lotso/PycharmProjects/uvicorn/uvicorn/server.py:87> wait_for=<Future pending cb=[<TaskWakeupMethWrapper object at 0x7f462c562df0>()]>>
INFO:     127.0.0.1:45238 - "GET / HTTP/1.1" 200 OK

it's happening just when specifying asyncio loop, putting uvloop and we dont have that

[florimondmanca]

@euri10 Yup, just saw that (I think it surfaced thanks to the upgraded test suite — I wasn't seeing it before). Resolved in the latest (force) push: we used to naively await read(MAX_RECV), which blocked indefinitely if the client didn't send any data. I switched to a hack so we just read what's readily available in the buffer (which is what I had in mind initially). Feel free to take a look at the updated logic. :)

[florimondmanca]

@euri10 How would you go about confirming there's no risk of memory issue with this…? Spawn a server, hit it with random requests for a while, and see how memory usage goes?

[florimondmanca]

@euri10 Hmm nah, my latest attempt isn't working. Try requesting with curl — Uvicorn is now just stalling. We take the "no data in read buffer" branch, and it stalls. Curious why. I'll try something else. :-)

[florimondmanca]

Did some more work on swapping initial read buffers and exception handling, I think this is looking good. :)

[euri10]

@euri10 How would you go about confirming there's no risk of memory issue with this…? Spawn a server, hit it with random requests for a while, and see how memory usage goes?

Having a client like this is a good start (shamelessly taken from here)

import sys, socket
host, port = sys.argv[1], int(sys.argv[2])
with socket.create_connection((host, port)) as sock:
    get = b"GET / HTTP/1.1\r\nHost: " + host.encode("ascii") + b"\r\n\r\n"
    requests_sent = 0
    while True:
        sock.sendall(get)
        requests_sent += 1
        if requests_sent % 1000 == 0:
            print("Sent {} requests", requests_sent)

I just played a little with it, it seems I have same results on master so I doesn't seem specific to this branch, on both I stopped a 1M requests (this is where the plateau begins, way shorter in 2nd graph), but in both cases we can see memory usage growing up:

streams branch:

master branch

I used this profile_me.py file to check memory, ran with mprof run profile_me.py:

from fastapi import FastAPI
from memory_profiler import profile

import uvicorn

app = FastAPI(debug=True)


@app.on_event("startup")
async def startup():
    print('startup lifespan')


@app.on_event("shutdown")
async def shutdown():
    print('shutdown lifespan')


@app.get("/")
async def root():
    return 1114


@profile
def profile_server():
    uvicorn.run("app:app")


if __name__ == '__main__':
    profile_server()

[euri10]

I love this, just have 2 small questions and I feel we gtg
One final remark, dunno if you know a way, it seems we uncovered 2 bugs recently with tasks destroyed while pending: is there a way to catch those with pytest, I searched and couldn't find.
The 2 websockets ones I fixed were showing up already before and it's just the flaky test in the CI that revealed it was an issue, locally I never say those, this could be great to get a warning of some sort ?

[florimondmanca]

@euri10 Thanks for the quick memory benchmarks! About the growing memory — shouldn't the client also be reading the response from the socket? Uvicorn responds anyway so I'm wondering if not reading the response means it gets buffered indefinitely on the server side. Do you get the same behavior if using a full fledged client like HTTPX?

[euri10]

@euri10 Thanks for the quick memory benchmarks! About the growing memory — shouldn't the client also be reading the response from the socket? Uvicorn responds anyway so I'm wondering if not reading the response means it gets buffered indefinitely on the server side. Do you get the same behavior if using a full fledged client like HTTPX?

yep that's the whole point of this stupid client if I understand it correctly. but that's not because of that branch so we can track this as a separate thing !
if I run wrk on this branch I get a nice flat line 👍 :

❯ docker run -v `pwd`:/data  --net=host --rm williamyeh/wrk -c 512 -d 5m -t 16  http://127.0.0.1:8000/
Running 5m test @ http://127.0.0.1:8000/
  16 threads and 512 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     1.07s   113.73ms   2.00s    89.96%
    Req/Sec    35.36     36.99   313.00     96.52%
  142424 requests in 5.00m, 17.39MB read
  Socket errors: connect 0, read 0, write 0, timeout 695
Requests/sec:    474.59
Transfer/sec:     59.32KB
  ~/PycharmProjects/uvicorn   fm/asyncio-use-streams *1 ?14 ❯       

[euri10]

hi @florimondmanca , fancy rebasing this ?
I was looking at pushing a new release over the next week or so, we have many changes in the owrks, since this one would require a minor bump and because we already have merges that are likely to require the same bump maybe we could do everything at the same time ?

[florimondmanca]

@euri10 Putting this on my TODO list. Hope to give it a go today or beginning of next week…

[florimondmanca]

@euri10 Should be ready now. :-)

[euri10]

thanks @florimondmanca !