Repository: encode/uvicorn

Issue:
httptools.parser.errors.HttpParserInvalidURLError on fragmented first line of the HTTP request.
### Checklist

<!-- Please make sure you check all these items before submitting your bug report. -->

- [x] The bug is reproducible against the latest release and/or `master`.
- [x] There are no similar issues or pull requests to fix it yet.

### Describe the bug

I've ran into issue with uvicorn+httptools, HttpParserInvalidURLError is raised and the server closes connection when the first line of the http/1.* request is split into several tcp packets.
It also seems like everything is fine if h11 used instead of httptools.

### To reproduce
```
import socket
import threading as th
from time import sleep

import uvicorn

########## HELPERS ##########
def receive_all(sock):
    chunks = []
    while True:
        chunk = sock.recv(1024)
        if not chunk:
            break

        chunks.append(chunk)

    return b''.join(chunks)


async def stub_app(scope, receive, send):
    event = await receive()
    d = b'whatever'
    await send({
        'type': 'http.response.start',
        'status': 200,
        'headers': [
            [b'Content-Length', str(len(d)).encode()],
            [b'Content-Type', b'text/plain'],
        ]
    })
    await send({'type': 'http.response.body', 'body': d, 'more_body': False})
#############################


def send_fragmented_req(path):

    sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
    sock.connect(('127.0.0.1', 5000))
    d = (
        f'GET {path} HTTP/1.1\r\n'
        'Host: localhost\r\n'
        'Connection: close\r\n\r\n'
    ).encode()

    split = len(path) // 2
    # send first part
    sock.sendall(d[:split])
    sleep(0.01)  # important, ~100% error, without this the chances of reproducing the error are pretty low

    # send second part
    sock.sendall(d[split:])
    
    # read response
    resp = receive_all(sock)
    sock.shutdown(socket.SHUT_RDWR)
    sock.close()
    return resp


def test():
    t = th.Thread(target=lambda: uvicorn.run(stub_app, host='127.0.0.1', port=5000, http='httptools'))
    t.daemon = True
    t.start()
    sleep(1)    # wait for unicorn to start
    
    path = '/?param=' + 'q' * 10
    print(send_fragmented_req(path).decode())


if __name__ == '__main__':
    test()


# Console output:
# INFO:     Started server process [748603]
# INFO:     Waiting for application startup.
# INFO:     ASGI 'lifespan' protocol appears unsupported.
# INFO:     Application startup complete.
# INFO:     Uvicorn running on http://127.0.0.1:5000 (Press CTRL+C to quit)
# WARNING:  Invalid HTTP request received.
# Traceback (most recent call last):
#   File "httptools/parser/parser.pyx", line 258, in httptools.parser.parser.cb_on_url
#   File "/_/lib/python3.8/site-packages/uvicorn/protocols/http/httptools_impl.py", line 193, in on_url
#     parsed_url = httptools.parse_url(url)
#   File "httptools/parser/url_parser.pyx", line 105, in httptools.parser.url_parser.parse_url
# httptools.parser.errors.HttpParserInvalidURLError: invalid url b'am=qqqqqqqqqq'
#
# During handling of the above exception, another exception occurred:
#
# Traceback (most recent call last):
#   File "/_/lib/python3.8/site-packages/uvicorn/protocols/http/httptools_impl.py", line 131, in data_received
#     self.parser.feed_data(data)
#   File "httptools/parser/parser.pyx", line 212, in httptools.parser.parser.HttpParser.feed_data
# httptools.parser.errors.HttpParserCallbackError: `on_url` callback error

```

### Expected behavior

HTTP response:

```
HTTP/1.1 200 OK
date: Tue, 23 Nov 2021 09:36:10 GMT
server: uvicorn
content-length: 8
content-type: text/plain

whatever
```

### Actual behavior

Server closes connection without a response.

### Environment

Running uvicorn 0.15.0 with CPython 3.8.2 on Linux
Dependencies:
    uvicorn==0.15.0
    h11==0.12.0
    httptools==0.3.0
   

Discussion:

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/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/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()