Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

[master] Fixed #18972 -- Refactored chunking algorithm stealing shameles... #921

Closed
wants to merge 3 commits into from

3 participants

@woodm1979

...sly from datagroks code.

@charettes charettes referenced this pull request
Closed

Fixed bug #18972 #782

django/core/servers/basehttp.py
@@ -18,6 +18,7 @@
except ImportError: # Python 2
from urlparse import urljoin
from django.utils.six.moves import socketserver
+from io import BytesIO
@charettes Collaborator

Move that import before os.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
django/core/servers/basehttp.py
@@ -27,6 +28,11 @@
__all__ = ['WSGIServer', 'WSGIRequestHandler']
+# If data is too large, socket will choke, so write chunks no larger than 32MB
+# at a time.
+# FIXME: Document rationale for 32MB and not some smaller or larger value.
@charettes Collaborator

This should be fixed.

I'm not remotely qualified to define the rationale here. But it was provided by the original author of this code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
tests/builtin_server/tests.py
@@ -53,3 +53,22 @@ def test_file_wrapper_no_sendfile(self):
self.assertFalse(handler._used_sendfile)
self.assertEqual(handler.stdout.getvalue().splitlines()[-1], b'Hello World!')
self.assertEqual(handler.stderr.getvalue(), b'')
+
+#
+# Tests for #18972: The logic that performs the math to break data into 32MB
+# chunks was flawed, BUT it didn't actually cause any problems.
+#
+
+def send_big_data_app(environ, start_response):
+ start_response(str('200 OK'), [(str('Content-Type'), str('text/plain'))])
+ # Return a blob of data that slightly exceeds chunk size.
+ return [bytes('x' * (32 * 1024 * 1024 + 5))]
@charettes Collaborator

Use MAX_SOCKET_CHUNK_SIZE here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
tests/builtin_server/tests.py
@@ -53,3 +53,22 @@ def test_file_wrapper_no_sendfile(self):
self.assertFalse(handler._used_sendfile)
self.assertEqual(handler.stdout.getvalue().splitlines()[-1], b'Hello World!')
self.assertEqual(handler.stderr.getvalue(), b'')
+
+#
+# Tests for #18972: The logic that performs the math to break data into 32MB
+# chunks was flawed, BUT it didn't actually cause any problems.
+#
@charettes Collaborator

Move those comments to the ServerHandlerChunksProperly's doc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
django/core/servers/basehttp.py
@@ -25,7 +26,12 @@
from django.core.wsgi import get_wsgi_application
from django.utils.module_loading import import_by_path
-__all__ = ['WSGIServer', 'WSGIRequestHandler']
+__all__ = ['WSGIServer', 'WSGIRequestHandler', 'MAX_SOCKET_CHUNK_SIZE']
+
+# If data is too large, socket will choke, so write chunks no larger than 32MB
+# at a time.
+# FIXME: Document rationale for 32MB and not some smaller or larger value.
@carljm Owner
carljm added a note

I think we should replace this FIXME with the rationale (fuzzy though it is) from the original ticket that added the 32MB chunk size: https://code.djangoproject.com/ticket/5596#comment:4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@charettes
Collaborator

Moved all your work with attribution on Trac, will wait for @carljm RFC go there and commit.

@charettes charettes closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
Showing with 39 additions and 15 deletions.
  1. +16 −14 django/core/servers/basehttp.py
  2. +23 −1 tests/builtin_server/tests.py
View
30 django/core/servers/basehttp.py
@@ -9,6 +9,7 @@
from __future__ import unicode_literals
+from io import BytesIO
import os
import socket
import sys
@@ -25,7 +26,18 @@
from django.core.wsgi import get_wsgi_application
from django.utils.module_loading import import_by_path
-__all__ = ['WSGIServer', 'WSGIRequestHandler']
+__all__ = ['WSGIServer', 'WSGIRequestHandler', 'MAX_SOCKET_CHUNK_SIZE']
+
+# If data is too large, socket will choke, so write chunks no larger than 32MB
+# at a time.
+#
+# eibaan's reasoning for 32MB from ticket 5596:
+# It seems to be a nice value :) 32 MB is large enough so that for typical
+# usage the optimization doesn't kick in and we stay backward compatible.
+# Uploading 32 MB is still fast and doesn't seem to stress the Python garbage
+# collector too much. Using 16 MB or even less as threshold would be fine with
+# me...
+MAX_SOCKET_CHUNK_SIZE = 32 * 1024 * 1024 # 32 MB
def get_internal_wsgi_application():
@@ -77,19 +89,9 @@ def write(self, data):
self.bytes_sent += len(data)
# XXX check Content-Length and truncate if too many bytes written?
-
- # If data is too large, socket will choke, so write chunks no larger
- # than 32MB at a time.
- length = len(data)
- if length > 33554432:
- offset = 0
- while offset < length:
- chunk_size = min(33554432, length)
- self._write(data[offset:offset+chunk_size])
- self._flush()
- offset += chunk_size
- else:
- self._write(data)
+ data = BytesIO(data)
+ for chunk in iter(lambda: data.read(MAX_SOCKET_CHUNK_SIZE), ''):
+ self._write(chunk)
self._flush()
def error_output(self, environ, start_response):
View
24 tests/builtin_server/tests.py
@@ -2,7 +2,7 @@
from io import BytesIO
-from django.core.servers.basehttp import ServerHandler
+from django.core.servers.basehttp import ServerHandler, MAX_SOCKET_CHUNK_SIZE
from django.utils.unittest import TestCase
#
@@ -53,3 +53,25 @@ def test_file_wrapper_no_sendfile(self):
self.assertFalse(handler._used_sendfile)
self.assertEqual(handler.stdout.getvalue().splitlines()[-1], b'Hello World!')
self.assertEqual(handler.stderr.getvalue(), b'')
+
+
+def send_big_data_app(environ, start_response):
+ start_response(str('200 OK'), [(str('Content-Type'), str('text/plain'))])
+ # Return a blob of data that slightly exceeds chunk size.
+ return [bytes('x' * (MAX_SOCKET_CHUNK_SIZE + 5))]
+
+
+class ServerHandlerChunksProperly(TestCase):
+ """
+ Test that the ServerHandler chunks data properly.
+
+ Tests for #18972: The logic that performs the math to break data into
+ 32MB (MAX_SOCKET_CHUNK_SIZE) chunks was flawed, BUT it didn't actually
+ cause any problems.
+ """
+
+ def test_chunked_data(self):
+ env = {'SERVER_PROTOCOL': 'HTTP/1.0'}
+ handler = ServerHandler(None, BytesIO(), BytesIO(), env)
+ handler.request_handler = DummyHandler()
+ handler.run(send_big_data_app)
Something went wrong with that request. Please try again.