Repository: encode/uvicorn

Issue:
WSGI interface: reading request body is O(n^2) wrt payload size
Hello!
This is related to https://github.com/encode/uvicorn/issues/371 and potentially a duplicate/complementary information, feel free to close if it is deemed to be the case.

I was fooling around with different WSGI and ASGI servers while hacking on a web framework that would support both in the future.

As noted in https://github.com/encode/uvicorn/issues/371, this is indeed a very inefficient scenario, however, what is worse, I found out the time required to turn around the request is at least O(n^2) or worse wrt the payload size.

`POST`ing some files and multipart forms with `curl`:
```
Payload size: 94171344
real	0m5.763s
user	0m0.033s
sys	0m0.012s
```
```
Payload size: 186054864
real	0m24.183s
user	0m0.018s
sys	0m0.061s
```
```
Payload size: 372178858
real	1m37.760s
user	0m0.044s
sys	0m0.152s
```

So it is not just inefficient loading everything into memory, but something else is going on.

Test case used:
```python
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()]
```

Running `uvicorn==0.11.5` as `uvicorn --interface wsgi simple_reader:app`.

Discussion:
vytas7: I've just realized this might get superseded by https://github.com/encode/uvicorn/pull/1303, so judge accordingly.
tomchristie: Great - thanks!

Given that this clearly resolves an open bug, I'd suggest that we get this in and fixed. I can't see any downside to that.
It's possible that it'll be superseded by https://github.com/encode/uvicorn/pull/1303 - but taking a call on that one is more complex than this.

The test suite currently has a listing failure. I expect it's unrelated to this pull request(?) but it'll needs resolving first, either way.
vytas7: Hi @tomchristie , and thanks for looking at this.
I think these were typing issues and other glitches possibly introduced by _this_ pull request, but I just didn't dig too deep since I didn't know what the plan was. If there is a chance to get this merge, I can try to rectify these.
tomchristie: Rather than using `io.BytesIO` could you use list appends instead, eg...

Change from the existing:

```python
         body = message.get("body", b"")
         more_body = message.get("more_body", False)
         while more_body:
             body_message: HTTPRequestEvent = await receive()  # type: ignore[assignment]
             body += body_message.get("body", b"")
             more_body = body_message.get("more_body", False)
```

to this...

```python
        body_list = [message.get("body", b"")]
        more_body = message.get("more_body", False)
        while more_body:
             body_message: HTTPRequestEvent = await receive()  # type: ignore[assignment]
             body_list.append(body_message.get("body", b""))
             more_body = body_message.get("more_body", False)
        body = b"".join(body_list)
```

That'd probably result in a pull request with a smaller change footprint, right?
vytas7: I've actually considered this; but I think it would in general result in higher overhead.

Writing to a `BytesIO` only very marginally loses against appending, and may even come close or surpass other approaches for many large chunks of data. Furthermore, it is more memory-efficient since a `BytesIO` instance only needs to keep an internal memory buffer that it can reallocate on demand; appending to a list would need twice the memory right after concatenation.
tomchristie: Thanks for you time on this @vytas7.
I think we need a more comprehensive fix, so let's close this given #1303.

(Either we'll rely on `a2wsgi` or we need a fix that doesn't load all the body into memory at once.)
tomchristie: Re-opening given https://github.com/encode/uvicorn/pull/1303#issuecomment-1036102453

Currently blocked on the CI type checking failures.
vytas7: Thanks Tom; if this is still of interest, I'll rectify 'em typing problems later in the evening.
tomchristie: Seems like a good plan to me.
It's a fairly low impact PR, and it'll close an outstanding bug, so... 👍

(Thanks for your time on this)
vytas7: @tomchristie done :heavy_check_mark: (The CI now passes ~~with flying colours~~.)

Patch:
diff --git a/docs/deployment.md b/docs/deployment.md
index 60d1af7..2224f98 100644
--- a/docs/deployment.md
+++ b/docs/deployment.md
@@ -39,12 +39,15 @@ Options:
                                   of using the current working directory.
   --reload-include TEXT           Set glob patterns to include while watching
                                   for files. Includes '*.py' by default; these
-                                  defaults can be overridden in `--reload-
-                                  exclude`.
+                                  defaults can be overridden with `--reload-
+                                  exclude`. This option has no effect unless
+                                  watchgod is installed.
   --reload-exclude TEXT           Set glob patterns to exclude while watching
                                   for files. Includes '.*, .py[cod], .sw.*,
                                   ~*' by default; these defaults can be
-                                  overridden in `--reload-include`.
+                                  overridden with `--reload-include`. This
+                                  option has no effect unless watchgod is
+                                  installed.
   --reload-delay FLOAT            Delay between previous and next check if
                                   application needs to be. Defaults to 0.25s.
                                   [default: 0.25]
diff --git a/docs/index.md b/docs/index.md
index 6f57b09..f9b79d5 100644
--- a/docs/index.md
+++ b/docs/index.md
@@ -108,12 +108,15 @@ Options:
                                   of using the current working directory.
   --reload-include TEXT           Set glob patterns to include while watching
                                   for files. Includes '*.py' by default; these
-                                  defaults can be overridden in `--reload-
-                                  exclude`.
+                                  defaults can be overridden with `--reload-
+                                  exclude`. This option has no effect unless
+                                  watchgod is installed.
   --reload-exclude TEXT           Set glob patterns to exclude while watching
                                   for files. Includes '.*, .py[cod], .sw.*,
                                   ~*' by default; these defaults can be
-                                  overridden in `--reload-include`.
+                                  overridden with `--reload-include`. This
+                                  option has no effect unless watchgod is
+                                  installed.
   --reload-delay FLOAT            Delay between previous and next check if
                                   application needs to be. Defaults to 0.25s.
                                   [default: 0.25]
diff --git a/docs/settings.md b/docs/settings.md
index 5085b9c..25be523 100644
--- a/docs/settings.md
+++ b/docs/settings.md
@@ -29,13 +29,20 @@ For example, in case you want to run the app on port `5000`, just set the enviro
 
 ## Development
 
-* `--reload` - Enable auto-reload.
+* `--reload` - Enable auto-reload. Uvicorn supports two versions of auto-reloading behavior enabled by this option. There are important differences between them.
 * `--reload-dir <path>` - Specify which directories to watch for python file changes. May be used multiple times. If unused, then by default the whole current directory will be watched. If you are running programmatically use `reload_dirs=[]` and pass a list of strings.
+
+### Reloading without watchgod
+
+If Uvicorn _cannot_ load [watchgod](https://pypi.org/project/watchgod/) at runtime, it will periodically look for changes in modification times to all `*.py` files (and only `*.py` files) inside of its monitored directories. See the `--reload-dir` option. Specifying other file extensions is not supported unless watchgod is installed. See the `--reload-include` and `--reload-exclude` options for details.
+
+### Reloading with watchgod
+
+For more nuanced control over which file modifications trigger reloads, install `uvicorn[standard]`, which includes watchgod as a dependency. Alternatively, install [watchgod](https://pypi.org/project/watchgod/) where Unvicorn can see it. This will enable the following options (which are otherwise ignored).
+
 * `--reload-include <glob-pattern>` - Specify a glob pattern to match files or directories which will be watched. May be used multiple times. By default the following patterns are included: `*.py`. These defaults can be overwritten by including them in `--reload-exclude`.
 * `--reload-exclude <glob-pattern>` - Specify a glob pattern to match files or directories which will excluded from watching. May be used multiple times. By default the following patterns are excluded: `.*, .py[cod], .sw.*, ~*`. These defaults can be overwritten by including them in `--reload-include`.
 
-By default Uvicorn uses simple changes detection strategy that compares python files modification times few times a second. If this approach doesn't work for your project (eg. because of its complexity), or you need watching of non python files you can install [watchgod](https://pypi.org/project/watchgod/) or install uvicorn with `uvicorn[standard]`, which will include watchgod.
-
 ## Production
 
 * `--workers <int>` - Use multiple worker processes. Defaults to the `$WEB_CONCURRENCY` environment variable if available, or 1.
diff --git a/uvicorn/main.py b/uvicorn/main.py
index cd853b6..0db1216 100644
--- a/uvicorn/main.py
+++ b/uvicorn/main.py
@@ -87,7 +87,8 @@ def print_version(ctx: click.Context, param: click.Parameter, value: bool) -> No
     "reload_includes",
     multiple=True,
     help="Set glob patterns to include while watching for files. Includes '*.py' "
-    "by default; these defaults can be overridden in `--reload-exclude`.",
+    "by default; these defaults can be overridden with `--reload-exclude`. "
+    "This option has no effect unless watchgod is installed.",
 )
 @click.option(
     "--reload-exclude",
@@ -95,7 +96,8 @@ def print_version(ctx: click.Context, param: click.Parameter, value: bool) -> No
     multiple=True,
     help="Set glob patterns to exclude while watching for files. Includes "
     "'.*, .py[cod], .sw.*, ~*' by default; these defaults can be overridden "
-    "in `--reload-include`.",
+    "with `--reload-include`. This option has no effect unless watchgod is "
+    "installed.",
 )
 @click.option(
     "--reload-delay",
diff --git a/uvicorn/middleware/wsgi.py b/uvicorn/middleware/wsgi.py
index 74bdfad..f58236a 100644
--- a/uvicorn/middleware/wsgi.py
+++ b/uvicorn/middleware/wsgi.py
@@ -19,7 +19,9 @@ from asgiref.typing import (
 from uvicorn._types import Environ, ExcInfo, StartResponse, WSGIApp
 
 
-def build_environ(scope: HTTPScope, message: ASGIReceiveEvent, body: bytes) -> Environ:
+def build_environ(
+    scope: HTTPScope, message: ASGIReceiveEvent, body: io.BytesIO
+) -> Environ:
     """
     Builds a scope and request message into a WSGI environ object.
     """
@@ -31,7 +33,7 @@ def build_environ(scope: HTTPScope, message: ASGIReceiveEvent, body: bytes) -> E
         "SERVER_PROTOCOL": "HTTP/%s" % scope["http_version"],
         "wsgi.version": (1, 0),
         "wsgi.url_scheme": scope.get("scheme", "http"),
-        "wsgi.input": io.BytesIO(body),
+        "wsgi.input": body,
         "wsgi.errors": sys.stdout,
         "wsgi.multithread": True,
         "wsgi.multiprocess": True,
@@ -105,12 +107,17 @@ class WSGIResponder:
         self, receive: ASGIReceiveCallable, send: ASGISendCallable
     ) -> None:
         message: HTTPRequestEvent = await receive()  # type: ignore[assignment]
-        body = message.get("body", b"")
+        body = io.BytesIO(message.get("body", b""))
         more_body = message.get("more_body", False)
-        while more_body:
-            body_message: HTTPRequestEvent = await receive()  # type: ignore[assignment]
-            body += body_message.get("body", b"")
-            more_body = body_message.get("more_body", False)
+        if more_body:
+            body.seek(0, io.SEEK_END)
+            while more_body:
+                body_message: HTTPRequestEvent = (
+                    await receive()  # type: ignore[assignment]
+                )
+                body.write(body_message.get("body", b""))
+                more_body = body_message.get("more_body", False)
+            body.seek(0)
         environ = build_environ(self.scope, message, body)
         self.loop = asyncio.get_event_loop()
         wsgi = self.loop.run_in_executor(
diff --git a/uvicorn/protocols/http/httptools_impl.py b/uvicorn/protocols/http/httptools_impl.py
index 838c177..eb6cc00 100644
--- a/uvicorn/protocols/http/httptools_impl.py
+++ b/uvicorn/protocols/http/httptools_impl.py
@@ -74,7 +74,6 @@ class HttpToolsProtocol(asyncio.Protocol):
         self.pipeline = deque()
 
         # Per-request state
-        self.url = None
         self.scope = None
         self.headers = None
         self.expect_100_continue = False
@@ -182,15 +181,8 @@ class HttpToolsProtocol(asyncio.Protocol):
         self.transport.write(b"".join(content))
         self.transport.close()
 
-    # Parser callbacks
-    def on_url(self, url):
-        method = self.parser.get_method()
-        parsed_url = httptools.parse_url(url)
-        raw_path = parsed_url.path
-        path = raw_path.decode("ascii")
-        if "%" in path:
-            path = urllib.parse.unquote(path)
-        self.url = url
+    def on_message_begin(self):
+        self.url = b""
         self.expect_100_continue = False
         self.headers = []
         self.scope = {
@@ -200,14 +192,14 @@ class HttpToolsProtocol(asyncio.Protocol):
             "server": self.server,
             "client": self.client,
             "scheme": self.scheme,
-            "method": method.decode("ascii"),
             "root_path": self.root_path,
-            "path": path,
-            "raw_path": raw_path,
-            "query_string": parsed_url.query if parsed_url.query else b"",
             "headers": self.headers,
         }
 
+    # Parser callbacks
+    def on_url(self, url):
+        self.url += url
+
     def on_header(self, name: bytes, value: bytes):
         name = name.lower()
         if name == b"expect" and value.lower() == b"100-continue":
@@ -216,10 +208,20 @@ class HttpToolsProtocol(asyncio.Protocol):
 
     def on_headers_complete(self):
         http_version = self.parser.get_http_version()
+        method = self.parser.get_method()
+        self.scope["method"] = method.decode("ascii")
         if http_version != "1.1":
             self.scope["http_version"] = http_version
         if self.parser.should_upgrade():
             return
+        parsed_url = httptools.parse_url(self.url)
+        raw_path = parsed_url.path
+        path = raw_path.decode("ascii")
+        if "%" in path:
+            path = urllib.parse.unquote(path)
+        self.scope["path"] = path
+        self.scope["raw_path"] = raw_path
+        self.scope["query_string"] = parsed_url.query if parsed_url.query else b""
 
         # Handle 503 responses when 'limit_concurrency' is exceeded.
         if self.limit_concurrency is not None and (
diff --git a/uvicorn/supervisors/statreload.py b/uvicorn/supervisors/statreload.py
index 2c9857e..8392e9f 100644
--- a/uvicorn/supervisors/statreload.py
+++ b/uvicorn/supervisors/statreload.py
@@ -20,6 +20,12 @@ class StatReload(BaseReload):
         self.reloader_name = "statreload"
         self.mtimes: Dict[Path, float] = {}
 
+        if config.reload_excludes or config.reload_includes:
+            logger.warning(
+                "--reload-include and --reload-exclude have no effect unless watchgod "
+                "is installed."
+            )
+
     def should_restart(self) -> bool:
         for file in self.iter_py_files():
             try:


Test patch:
diff --git a/tests/middleware/test_wsgi.py b/tests/middleware/test_wsgi.py
index a9673c3..8b35fd1 100644
--- a/tests/middleware/test_wsgi.py
+++ b/tests/middleware/test_wsgi.py
@@ -1,5 +1,6 @@
+import io
 import sys
-from typing import List
+from typing import AsyncGenerator, List
 
 import httpx
 import pytest
@@ -67,6 +68,19 @@ async def test_wsgi_post() -> None:
     assert response.text == '{"example": 123}'
 
 
+@pytest.mark.asyncio
+async def test_wsgi_put_more_body() -> None:
+    async def generate_body() -> AsyncGenerator[bytes, None]:
+        for _ in range(1024):
+            yield b"123456789abcdef\n" * 64
+
+    app = WSGIMiddleware(echo_body)
+    async with httpx.AsyncClient(app=app, base_url="http://testserver") as client:
+        response = await client.put("/", content=generate_body())
+    assert response.status_code == 200
+    assert response.text == "123456789abcdef\n" * 64 * 1024
+
+
 @pytest.mark.asyncio
 async def test_wsgi_exception() -> None:
     # Note that we're testing the WSGI app directly here.
@@ -120,6 +134,6 @@ def test_build_environ_encoding() -> None:
         "body": b"",
         "more_body": False,
     }
-    environ = build_environ(scope, message, b"")
+    environ = build_environ(scope, message, io.BytesIO(b""))
     assert environ["PATH_INFO"] == "/文".encode("utf8").decode("latin-1")
     assert environ["HTTP_KEY"] == "value1,value2"
diff --git a/tests/protocols/test_http.py b/tests/protocols/test_http.py
index d5f3432..a1eacbb 100644
--- a/tests/protocols/test_http.py
+++ b/tests/protocols/test_http.py
@@ -1,10 +1,14 @@
 import asyncio
 import contextlib
 import logging
+import socket
+import threading
+import time
 
 import pytest
 
 from tests.response import Response
+from uvicorn import Server
 from uvicorn.config import Config
 from uvicorn.main import ServerState
 from uvicorn.protocols.http.h11_impl import H11Protocol
@@ -744,3 +748,46 @@ def test_invalid_http_request(request_line, protocol_cls, caplog, event_loop):
         protocol.data_received(request)
         assert b"HTTP/1.1 400 Bad Request" in protocol.transport.buffer
         assert b"Invalid HTTP request received." in protocol.transport.buffer
+
+
+def test_fragmentation():
+    def receive_all(sock):
+        chunks = []
+        while True:
+            chunk = sock.recv(1024)
+            if not chunk:
+                break
+            chunks.append(chunk)
+        return b"".join(chunks)
+
+    app = Response("Hello, world", media_type="text/plain")
+
+    def send_fragmented_req(path):
+
+        sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
+        sock.connect(("127.0.0.1", 8000))
+        d = (
+            f"GET {path} HTTP/1.1\r\n" "Host: localhost\r\n" "Connection: close\r\n\r\n"
+        ).encode()
+        split = len(path) // 2
+        sock.sendall(d[:split])
+        time.sleep(0.01)
+        sock.sendall(d[split:])
+        resp = receive_all(sock)
+        sock.shutdown(socket.SHUT_RDWR)
+        sock.close()
+        return resp
+
+    config = Config(app=app, http="httptools")
+    server = Server(config=config)
+    t = threading.Thread(target=server.run)
+    t.daemon = True
+    t.start()
+    time.sleep(1)  # wait for unicorn to start
+
+    path = "/?param=" + "q" * 10
+    response = send_fragmented_req(path)
+    bad_response = b"HTTP/1.1 400 Bad Request"
+    assert bad_response != response[: len(bad_response)]
+    server.should_exit = True
+    t.join()