id summary reporter owner description type status component version severity resolution keywords cc stage has_patch needs_docs needs_tests needs_better_patch easy ui_ux 32472 runserver prematurely closes connection for large response body David Sanders nobody "This was quite a pain to nail down exactly what was going on, but I think I've got my arms around it enough to file a ticket and a suggested fix. It's basically a race condition between the OS and the Python, which makes for real fun when debugging since the debugger pausing Python makes the OS win the race, and that's the go-right case. :-) I don't have time at the moment (already took a full day to debug) to do a full PR with a test case, but anyone is welcome to make one from the recommended fix. **Reproduction** * Create a view which returns a large response body. I haven't tried to find a minimum size where it reproduces, but I was testing with a 50kb `JSONResponse`. The bigger the better for ensuring reproduction. * Serve the project with `runserver` * Request the view with `Connection: close` - this is the important bit * The server will close the connection before the response is fully received by the client. Using Wireshark I can see that the server sends a FIN packet before all of the data has been sent. I believe those to be generic reproduction steps. In my actual testing and debugging the request was a POST, but I don't think that factors in at all. I was also running Django from inside the `python:3.9` Docker image. **Analysis** I believe what is happening here is that the socket is being shut down before the send buffer has been cleared by the OS, and that shut down is causing it to discard the send buffer and send a FIN packet rather than continue to send the remaining contents of the send buffer. If you trace through the code to see where the response is sent, you end up (with CPython 3.9.2 at least) [https://github.com/python/cpython/blob/v3.9.2/Lib/socketserver.py#L798-L801 here] in the `socketserver` module, where it is sent using `socket.sendall`: {{{ #!python def write(self, b): self._sock.sendall(b) with memoryview(b) as view: return view.nbytes }}} That call returns once the data is in the send buffer, but hasn't been sent to the client yet. In Django's code the socket is then shut down [https://github.com/django/django/blob/3.1.7/django/core/servers/basehttp.py#L176 here]: {{{#!python def handle(self): self.close_connection = True self.handle_one_request() while not self.close_connection: self.handle_one_request() try: self.connection.shutdown(socket.SHUT_WR) except (AttributeError, OSError): pass }}} However, that's not the only spot that shut down or close is called on the socket. There's several places in the Python standard library (CPython implementation) such as [https://github.com/python/cpython/blob/v3.9.2/Lib/socketserver.py#L784-L785 here] and [https://github.com/python/cpython/blob/v3.9.2/Lib/socketserver.py#L501-L509 here]. I mention this to point out that it's not just Django's code shutting down the socket, and removing that shut down doesn't change the overall outcome. I'm not sure why there's an explicit shut down in Django's code, but it's not the source of the trouble - the same shutdown occurs very shortly after that in the Python standard library code. I tried setting `SO_LINGER` on the socket (including on the listening socket before a connection is accepted) since in theory it is supposed to fix this sort of situation and cause `close` or `shutdown` to block until all data is sent. I couldn't seem to get this behavior, even with `SO_LINGER` set, a `shutdown` or `close` was immediately causing a FIN on the TCP connection, with the data still to be sent discarded. `SO_LINGER` also wouldn't fully solve the problem and ensure that the client received all the data, since it doesn't wait for confirmation (the client closing the connection), and the client could theoretically need retransmissions of packets. **Proposed Fix** It seems that the best solution to this issue is to wait for the client to close the connection. This ensures that the response was fully received by the client. This is actually more or less the existing behavior for a client using `Connection: keep-alive`, and why the issue only shows up when `Connection: close` is used. The former is going to be the default behavior of any browser, while HTTP clients from scripts or servers (like Node.js) often default to the latter rather than persistent connections. In the request handling loop (shown above) in `django.core.servers.basehttp.WSGIRequestHandler` a connection with `keep-alive` will cause the server to immediately try to read another request from the socket, which blocks and keeps the socket from being shut down on the server until the client closes it. For an HTTP client using `keep-alive`, but only sending a single request before closing the connection, that means the server is handling a single request and ends up waiting for the client to close the connection. I believe the issue would probably also show up with `keep-alive` if multiple requests were sent, the final request included `Connection: close`, and that final request had a large response payload. I haven't tested that, but the way the code reads that would lead to the same situation. Here's a proposed patch to unify the behavior for both cases, and always do a blocking read on the socket before shutting it down on the server: {{{ --- django/core/servers/basehttp.py 2021-02-21 17:31:25.000000000 -0800 +++ django/core/servers/basehttp.py 2021-02-21 17:32:37.000000000 -0800 @@ -7,6 +7,7 @@ been reviewed for security issues. DON'T USE IT FOR PRODUCTION USE! """""" +from http import HTTPStatus import logging import socket import socketserver @@ -172,19 +173,33 @@ self.handle_one_request() while not self.close_connection: self.handle_one_request() + + # Wait for the connection to be closed by the client to ensure + # that all data was received. Shutting down the connection seems + # to flush any data in the send buffer and immediately ends the + # connection, which risks having large responses cut off. + self.rfile.peek() + try: self.connection.shutdown(socket.SHUT_WR) except (AttributeError, OSError): pass def handle_one_request(self): - """"""Copy of WSGIRequestHandler.handle() but with different ServerHandler"""""" + """""" + Copy of WSGIRequestHandler.handle() but with different ServerHandler, + and re-aligned with BaseHTTPRequestHandler.handle() + """""" self.raw_requestline = self.rfile.readline(65537) if len(self.raw_requestline) > 65536: self.requestline = '' self.request_version = '' self.command = '' - self.send_error(414) + self.send_error(HTTPStatus.REQUEST_URI_TOO_LONG) + return + + if not self.raw_requestline: + self.close_connection = True return if not self.parse_request(): # An error code has been sent, just exit @@ -196,6 +211,8 @@ handler.request_handler = self # backpointer for logging & connection closing handler.run(self.server.get_app()) + self.wfile.flush() #actually send the response if not already done. + def run(addr, port, wsgi_handler, ipv6=False, threading=False, server_cls=WSGIServer): server_address = (addr, port) }}} The fix itself is the use of `self.rfile.peek()` before shutting down the socket. The earlier linked parts of the Python standard library which will shutdown/close the connection all occur after `handle()`, so as long as the blocking read occurs in that method it should happen before the connection could be closed. In the case of a connection initially opened with `Connection: close`, or multiple requests on a persistent connection with the final one specifying `Connection: close`, this line will block until the client closes the connection. In the case of a persistent connection that never specifies `Connection: close`, this should be a no-op as it will only be reached once the `close_connection` loop has exited, due to the client closing the connection. So, in every case the server would wait for the client to close the connection. I decided to use `peek()` instead of read since it makes it more clear that the data isn't important, and no arbitrary (and meaningless) size for the read needs to be specified. Any read should work - `read`, `readline`, etc - there just needs to be a blocking call trying to read from the socket. The other part of the patch is not needed for the fix, but I thought it would be good to update that code. Unfortunately `WSGIRequestHandler` in the Python standard library has diverged from `BaseHTTPRequestHandler`, which has some added code, including marking a blank `raw_requestline` as a `close_connection` condition. While `parse_request` also does this, it's probably a bit fragile to rely on that behavior, so explicitly handling that case seems like a worthwhile change. `self.wfile.flush()` and the use of `HTTPStatus.REQUEST_URI_TOO_LONG` are from `BaseHTTPRequestHandler` and also seemed reasonable to pull in at the same time. I don't know if the flush does anything meaningful for this case, but I don't think it hurts to have. ---- If anyone wants to champion fixing this issue, it would be much appreciated, and the above patch is free to use." Bug new HTTP handling 3.1 Normal Florian Apolloner Chris Jerdonek Ülgen Sarıkavak Accepted 0 0 0 0 0 0