Use a2wsgi.WSGIMiddleware replace WSGIMiddleware #1303

abersheeran · 2022-01-01T06:52:38Z

Related link: #1049 #1235

Perhaps this PR should be merged after several versions of #1235 have been merged to allow enough buffer time for users.

resolved #371 resolved #708

euri10 · 2022-02-10T12:30:15Z

setup.py

@@ -62,6 +62,7 @@ def get_packages(package):
    "watchgod>=0.6",
    "python-dotenv>=0.13",
    "PyYAML>=5.1",
+    "a2wsgi>=1.4.0,<2.0.0"


My only question here is: should this go in extra_requirements so that people have it when installing via pip install uvicorn[standard] or directly in the minimal_requirements section ?

the idea of uvicorn[standard] vs uvicorn was in summary cpython vs pure-python, or high-speed vs minimal, so I'd thought we better put it in minimal_requirements instead but I'm open on anything really

If we think Uvicorn can be used as a WSGI Server, then it should be added to minimal_requirements.

But in README:

Uvicorn is an ASGI web server implementation for Python.

So I'm not so sure...

What are @tomchristie 's thoughts?

I think we should add it in the docs and then close #177

euri10 · 2022-02-10T12:30:55Z

uvicorn/config.py

@@ -28,12 +28,21 @@
    # enable this functionality.
    pass

+try:


depending on the answer to the comment above @abersheeran the try / catch may not be necessary

tomchristie · 2022-02-10T13:49:52Z

This could be a good idea.
Certainly nice to see neatly targeted packages like https://github.com/abersheeran/a2wsgi 👏

The first thing to do in order for us to make a decision would be to have a clearer user-facing description here.
Which specific currently open issues does this resolve?

abersheeran · 2022-02-11T03:15:49Z

The first thing to do in order for us to make a decision would be to have a clearer user-facing description here. Which specific currently open issues does this resolve?

Some issues: #371 #708

It is worth mentioning that in #371 (comment) , the test results of a2wsgi are almost the same as those of ASGI.

euri10 · 2022-02-11T08:19:29Z

It is worth mentioning that in #371 (comment) , the test results of a2wsgi are almost the same as those of ASGI.

and the from OP's tests the fix in #1329 worsen the situation

euri10 · 2022-02-11T09:08:19Z

some tests, ~~I can confirm #1329 doesn't fix the issue~~, and this branch does:

on master:

❯ curl --form file='@10M.file' -w "@curl-format.txt" http://localhost:8000
Payload size: 10485960     time_namelookup:  0.000838s
        time_connect:  0.001144s
     time_appconnect:  0.000000s
    time_pretransfer:  0.001209s
       time_redirect:  0.000000s
  time_starttransfer:  0.002124s
                     ----------
          time_total:  0.140547s
❯ curl --form file='@100M.file' -w "@curl-format.txt" http://localhost:8000
Payload size: 104857801     time_namelookup:  0.000825s
        time_connect:  0.001101s
     time_appconnect:  0.000000s
    time_pretransfer:  0.001164s
       time_redirect:  0.000000s
  time_starttransfer:  0.002066s
                     ----------
          time_total:  19.151556s
  ~/PycharmProjects/uvicorn on   master +1 !1 ?4 ❯

on this branch:

❯ curl --form file='@10M.file' -w "@curl-format.txt" http://localhost:8000
Payload size: 10485960     time_namelookup:  0.000887s
        time_connect:  0.001225s
     time_appconnect:  0.000000s
    time_pretransfer:  0.001307s
       time_redirect:  0.000000s
  time_starttransfer:  0.003882s
                     ----------
          time_total:  2.030771s
❯ curl --form file='@100M.file' -w "@curl-format.txt" http://localhost:8000
Payload size: 104857801     time_namelookup:  0.000805s
        time_connect:  0.001106s
     time_appconnect:  0.000000s
    time_pretransfer:  0.001185s
       time_redirect:  0.000000s
  time_starttransfer:  0.002596s
                     ----------
          time_total:  1.400091s
❯ curl --form file='@1000M.file' -w "@curl-format.txt" http://localhost:8000
Payload size: 1048576202     time_namelookup:  0.000990s
        time_connect:  0.001306s
     time_appconnect:  0.000000s
    time_pretransfer:  0.001377s
       time_redirect:  0.000000s
  time_starttransfer:  0.003022s
                     ----------
          time_total:  3.512038s
  ~/Py/uvicorn on   abersheeran-use-a2wsgi +1 !1 ?4 ❯

~~on #1329 branch~~ see results below in next comments sorry :

❯ curl --form file='@10M.file' -w "@curl-format.txt" http://localhost:8000
Payload size: 10485960     time_namelookup:  0.001235s
        time_connect:  0.001536s
     time_appconnect:  0.000000s
    time_pretransfer:  0.001602s
       time_redirect:  0.000000s
  time_starttransfer:  0.002817s
                     ----------
          time_total:  0.152660s
❯ curl --form file='@100M.file' -w "@curl-format.txt" http://localhost:8000
Payload size: 104857801     time_namelookup:  0.001145s
        time_connect:  0.001458s
     time_appconnect:  0.000000s
    time_pretransfer:  0.001530s
       time_redirect:  0.000000s
  time_starttransfer:  0.002302s
                     ----------
          time_total:  18.966714s
  ~/Py/uvicorn on   vytas7-708-w…dy-quadratic +1 !1 ?4 ❯

~~

you can reproduce with creating the files:
truncate -s 10M 1000M.file
truncate -s 100M 1000M.file
truncate -s 1000M 1000M.file

having curl-format.txt

     time_namelookup:  %{time_namelookup}s\n
        time_connect:  %{time_connect}s\n
     time_appconnect:  %{time_appconnect}s\n
    time_pretransfer:  %{time_pretransfer}s\n
       time_redirect:  %{time_redirect}s\n
  time_starttransfer:  %{time_starttransfer}s\n
                     ----------\n
          time_total:  %{time_total}s\n

then uvicorn --interface wsgi tests.test_wsgi_large:app for running the app in test_wsgi_large.py

def app(environ, start_response):
    status = '200 OK'  # HTTP Status
    headers = [('Content-type', 'text/plain')]  # HTTP Headers
    start_response(status, headers)

    stream = environ['wsgi.input']
    length = 0
    while True:
        chunk = stream.read(8192)
        if not chunk:
            break
        length += len(chunk)

    resp = "Payload size: {}".format(length)
    return [resp.encode()]

and then curl with curl --form file='@100M.file' -w "@curl-format.txt" http://localhost:8000

tomchristie · 2022-02-11T10:45:13Z

Okay. We've two options...

Fix this in uvicorn directly. Mostly the two implementations are equivalent. The Body implementation in a2wsgi is where the main change here is.
Rely on a2wsgi as a dependancy.

In general I'd prefer we aim towards fewer dependancies.
On the other hand leaning on a2wsgi will get us resolved with less effort.

🤷‍♂️

vytas7 · 2022-02-11T11:12:21Z

Hm, it sounds strange @euri10 as I've verified that my branch does fix the issue, and your results are suspiciously similar to the master branch, making me think you've happened to still test against it.

OTOH, my branch is just a quickfix for quadratic explosion while still buffering into a bytestring; @abersheeran's middleware does it "properly" by streaming request. So it's still much desirable over my PR.

euri10 · 2022-02-11T11:20:57Z

Hm, it sounds strange @euri10 as I've verified that my branch does fix the issue, and your results are suspiciously similar to the master branch, making me think you've happened to still test against it.

let me double check it's entirely possible I fucked up

edit: yes I did fucked up testing your branch, it's fast, sorry for that (reason is git pull https://github.com/vytas7/uvicorn.git 708-wsgi-read-body-quadratic was not ff mergeable and I overlooked it and so I was still on master with just your branch name)

real results:

❯ curl --form file='@10M.file' -w "@curl-format.txt" http://localhost:8000
Payload size: 10485960     time_namelookup:  0.000785s
        time_connect:  0.001085s
     time_appconnect:  0.000000s
    time_pretransfer:  0.001155s
       time_redirect:  0.000000s
  time_starttransfer:  0.001648s
                     ----------
          time_total:  0.016278s
❯ curl --form file='@100M.file' -w "@curl-format.txt" http://localhost:8000
Payload size: 104857801     time_namelookup:  0.001182s
        time_connect:  0.001518s
     time_appconnect:  0.000000s
    time_pretransfer:  0.001588s
       time_redirect:  0.000000s
  time_starttransfer:  0.002256s
                     ----------
          time_total:  0.171567s
  ~/PycharmProjects/uvicorn on   vytas7-708-w…dy-quadratic ?4 ❯

tomchristie · 2022-02-16T12:38:10Z

Okay. Given @euri10's comment above it looks to me like we should close this one off now, given #1329.
Seem reasonable?

abersheeran · 2022-02-16T12:43:40Z

@tomchristie So, #371 should be closed?

euri10 · 2022-02-16T12:44:37Z

OTOH, my branch is just a quickfix for quadratic explosion while still buffering into a bytestring; @abersheeran's middleware does it "properly" by streaming request. So it's still much desirable over my PR.

yup, that said I think @vytas7 is right in saying

OTOH, my branch is just a quickfix for quadratic explosion while still buffering into a bytestring; @abersheeran's middleware does it "properly" by streaming request. So it's still much desirable over my PR.

so let's close the bug and reopen this once settled ?

Kludex · 2022-05-09T06:14:13Z

We're back on this!

Kludex · 2022-10-19T12:30:45Z

Since Starlette has moved to the same middleware proposed here, I think uvicorn should move as well.

@abersheeran Do you have time for this PR? If not, I would like to close and make it available for someone else to work on it. 🙏

abersheeran · 2022-10-19T14:29:33Z

@Kludex Sorry, I recently moved to a new job and don't have time to revise this PR.
I'm closing it for now.

Kludex · 2022-10-19T14:42:32Z

Don't worry. Hope you have a great experience on the new job! 😁

I'll open an issue for this, and see if someone wants to contribute. Thanks! 🙏

Use a2wsgi.WSGIMiddleware replace WSGIMiddleware

7c2ddb7

vytas7 mentioned this pull request Jan 16, 2022

Fix WSGI middleware not to explode quadratically in the case of a larger body #1329

Merged

euri10 added the wsgi label Feb 10, 2022

euri10 reviewed Feb 10, 2022

View reviewed changes

euri10 mentioned this pull request Feb 10, 2022

Deprecate WSGIMiddleware #1235

Closed

abersheeran closed this Feb 16, 2022

Kludex reopened this May 9, 2022

Kludex added this to the Version 0.19.0 milestone Jul 5, 2022

Kludex requested a review from tomchristie July 7, 2022 19:48

abersheeran closed this Oct 19, 2022

Kludex mentioned this pull request Oct 27, 2022

WSGI middleware should stream request body, rather than loading it all at once. #371

Closed

humrochagf mentioned this pull request Dec 31, 2022

Replace current WSGIMiddleware implementation by a2wsgi one #1825

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use a2wsgi.WSGIMiddleware replace WSGIMiddleware #1303

Use a2wsgi.WSGIMiddleware replace WSGIMiddleware #1303

abersheeran commented Jan 1, 2022 •

edited

Loading

euri10 Feb 10, 2022

abersheeran Feb 11, 2022

euri10 Feb 11, 2022

abersheeran Feb 11, 2022

euri10 Feb 10, 2022

tomchristie commented Feb 10, 2022

abersheeran commented Feb 11, 2022 •

edited

Loading

euri10 commented Feb 11, 2022

euri10 commented Feb 11, 2022 •

edited

Loading

tomchristie commented Feb 11, 2022

vytas7 commented Feb 11, 2022 •

edited

Loading

euri10 commented Feb 11, 2022 •

edited

Loading

tomchristie commented Feb 16, 2022

abersheeran commented Feb 16, 2022

euri10 commented Feb 16, 2022

Kludex commented May 9, 2022

Kludex commented Oct 19, 2022

abersheeran commented Oct 19, 2022

Kludex commented Oct 19, 2022

Use a2wsgi.WSGIMiddleware replace WSGIMiddleware #1303

Use a2wsgi.WSGIMiddleware replace WSGIMiddleware #1303

Conversation

abersheeran commented Jan 1, 2022 • edited Loading

euri10 Feb 10, 2022

Choose a reason for hiding this comment

abersheeran Feb 11, 2022

Choose a reason for hiding this comment

euri10 Feb 11, 2022

Choose a reason for hiding this comment

abersheeran Feb 11, 2022

Choose a reason for hiding this comment

euri10 Feb 10, 2022

Choose a reason for hiding this comment

tomchristie commented Feb 10, 2022

abersheeran commented Feb 11, 2022 • edited Loading

euri10 commented Feb 11, 2022

euri10 commented Feb 11, 2022 • edited Loading

tomchristie commented Feb 11, 2022

vytas7 commented Feb 11, 2022 • edited Loading

euri10 commented Feb 11, 2022 • edited Loading

tomchristie commented Feb 16, 2022

abersheeran commented Feb 16, 2022

euri10 commented Feb 16, 2022

Kludex commented May 9, 2022

Kludex commented Oct 19, 2022

abersheeran commented Oct 19, 2022

Kludex commented Oct 19, 2022

abersheeran commented Jan 1, 2022 •

edited

Loading

abersheeran commented Feb 11, 2022 •

edited

Loading

euri10 commented Feb 11, 2022 •

edited

Loading

vytas7 commented Feb 11, 2022 •

edited

Loading

euri10 commented Feb 11, 2022 •

edited

Loading