Support insertion in StreamingHttpResponse

[sirex]

Since Django 1.5 StreamingHttpResponse is introduced, this response class does not have content attribute, django-debug-toolbar middleware should be aware of that.

Now I get this error:

Traceback (most recent call last):
  File "/usr/lib/python2.7/wsgiref/handlers.py", line 85, in run
    self.result = application(self.environ, self.start_response)
  File "/home/sirex/.buildout/eggs/Django-1.5-py2.7.egg/django/contrib/staticfiles/handlers.py", line 72, in __call__
    return self.application(environ, start_response)
  File "/home/sirex/.buildout/eggs/Django-1.5-py2.7.egg/django/core/handlers/wsgi.py", line 255, in __call__
    response = self.get_response(request)
  File "/home/sirex/.buildout/eggs/Django-1.5-py2.7.egg/django/core/handlers/base.py", line 187, in get_response
    response = middleware_method(request, response)
  File "/home/sirex/.buildout/eggs/django_debug_toolbar-0.9.4-py2.7.egg/debug_toolbar/middleware.py", line 131, in process_response
    smart_unicode(response.content),
  File "/home/sirex/.buildout/eggs/Django-1.5-py2.7.egg/django/http/response.py", line 341, in content
    "Use `streaming_content` instead." % self.__class__.__name__)
AttributeError: This StreamingHttpResponse instance has no `content` attribute. Use `streaming_content` instead.

I use Django 1.5 and django-debug-toolbar 0.9.4.

[lucasicf]

I could reproduce the error and I'm checking this.

[lucasicf]

The issue is a bit hard to fix.
The new StreamingHttpResponse doesn't return a string content. It returns an iterator, and it seems to keep calling the iterator while the content is being sent to the browser, keeping the thread active meanwhile.

The middleware cannot just run the iterator, look for the target tag and change the response. It would break completely the feature, because the iterator is meant to be called while sending the ready content to the browser. Somehow, the middleware should intercept the iterator searching for the target tag in every call of it and "change" the iterator, including the rendered toolbar. I don't know if it's possible, I'll check. That's the only solution I could think about.

[mindsocket]

There is documentation on this at: https://docs.djangoproject.com/en/dev/topics/http/middleware/#dealing-with-streaming-responses

It may be legitimate to simply not process the response. After all, gzipped and non-HTML types are ignored already.

Otherwise, per the URL above, It looks like the streaming_content generator can be wrapped. I haven't been able to test this, but a guess at a solution follows...

Add to middleware.DebugToolbarMiddleware...

def _wrap_streaming_content(self, content)
    """Wrap the stream with a new generator that looks for the replacement tag"""
    for chunk in content:
        yield replace_insensitive(
                smart_unicode(chunk),
                self.tag,
                smart_unicode(toolbar.render_toolbar() + self.tag))

In process_response, add a streaming check:

def process_response(self, request, response):
    ####---snip---####
        if response.streaming:
            # Wrap with a replacement generator
            response.streaming_content = self._wrap_streaming_content(response.streaming_content)
        else:
            # Update response.content as before
            response.content = replace_insensitive(
                smart_unicode(response.content),
                self.tag,
                smart_unicode(toolbar.render_toolbar() + self.tag))
            if response.get('Content-Length', None):
                response['Content-Length'] = len(response.content)

Regarding the Content-Length update, I think it's safe to assume that a streaming response has no such header.

[lucasicf]

Great man, thanks. I've seen the HttpResponse documentation about this but not the middleware documentation.
I'll have more time tonight to work here and I will test your solution. I also will check whether applying this can be a viable solution.

[lucasicf]

This solution worked. However:

1. It performed 305 times slower than with the toolbar disabled. This is a library used only in a development environment not requiring super efficiency, but I guess this is a "remarkable" number. This was tested using the apache-bench tool.
2. It just works if the body tag is contained entirely in a iteration of the generator. If it generates '</', 'b', 'o', 'dy>' (for example), the wrapper isn't able to find the tag. It would require a solution to stack successive iterations into the wrapper and look over them, resulting in even poorer efficiency.

This was the run test:

from django.http import HttpResponse, StreamingHttpResponse

def request_generator():
    yield '<head><body>'
    for i in xrange(1,5000):
        yield '%d<br>' % i
    for i in xrange(1,500):
        yield ('BIGSTRINGBIGSTRINGBIGSTRINGBIGSTRINGBIGSTRINGBIGSTRINGBIGSTRINGBIGSTRING'
               'BIGSTRINGBIGSTRINGBIGSTRINGBIGSTRINGBIGSTRINGBIGSTRINGBIGSTRINGBIGSTRING'
               'BIGSTRINGBIGSTRINGBIGSTRINGBIGSTRINGBIGSTRINGBIGSTRINGBIGSTRINGBIGSTRING'
               'BIGSTRINGBIGSTRINGBIGSTRINGBIGSTRINGBIGSTRINGBIGSTRINGBIGSTRINGBIGSTRING'
               'BIGSTRINGBIGSTRINGBIGSTRINGBIGSTRINGBIGSTRINGBIGSTRINGBIGSTRINGBIGSTRING'
               'BIGSTRINGBIGSTRINGBIGSTRINGBIGSTRINGBIGSTRINGBIGSTRINGBIGSTRINGBIGSTRING'
               'BIGSTRINGBIGSTRINGBIGSTRINGBIGSTRINGBIGSTRINGBIGSTRINGBIGSTRINGBIGSTRING'
               'BIGSTRINGBIGSTRINGBIGSTRINGBIGSTRINGBIGSTRINGBIGSTRINGBIGSTRINGBIGSTRING'
               '%d<br>' % i)
    yield '</body></head>'

def streaming_http_test(request):
    return StreamingHttpResponse(request_generator())

I guess the best thing to do here is not to enable the support for the streaming responses considering the efficiency problems. It could be optimized somehow, however it would require an [maybe] unneccessary effort, since it seems to have a lot of other things to do, with higher priority, migrating the lib to Django 1.5.

What do you think?

[mindsocket]

I had a bit more of a play with this last night.

I think part 2 is ok to ignore by assuming (requiring) </body> appears in one chunk.

Regarding efficiency, I think this can be improved. My example code calls replace_insensitive, smart_unicode (twice) and most importantly toolbar.render_toolbar() for every chunk emitted by the generator. Instead it could just do the search as per replace_insensitive (string.lower().rfind(target.lower()) or similar) before evaluating toolbar.render_toolbar() and applying a replacement. A lazily evalulated replacement arg to replace_insensitive is one way to do this.

A naive example, that checks for the tag before calling replace_insensitive...

for chunk in content:
    if self.tag.lower() in chunk.lower():
        yield replace_insensitive(...)
    else:
        yield chunk

[lucasicf]

What a dumb of me. I didn't notice the render_toolbar() call was redundant inside the loop. It was the main bottleneck, now it's just 4 times slower according to this same test. I'll try some more optimizations. Thanks

[lucasicf]

I could get it much better, as said, following your tips.

1. I couldn't lazily render the toolbar because rendering it inside _wrap_streaming_content causes a bug of get_current() returning None. Thus I render it outside, just once.
2. The same for smart_unicode(rendered_toolbar + self.tag) (evaluated once), except from being rendered inside the wrap function, not outside.
3. Is it safe to search for the tag here: self.tag.lower() in chunk.lower(), without calling the smart_unicode? Since the search is done inside the replace_insensitive() without additional bottlenecks, I call this function in every iteration.

Here is the wrapping function:

def _wrap_streaming_content(self, rendered_toolbar, streaming_content):
    """
    Wrap the stream with a new generator that looks for the replacement
    tag. Used when the response is a generator for StreamingHttpResponse.
    """
    toolbar_and_tag = smart_unicode(rendered_toolbar + self.tag)
    for chunk in streaming_content:
        yield replace_insensitive(smart_unicode(chunk), self.tag, toolbar_and_tag)

[mindsocket]

Looks good to me! If it's fast enough as is (calling replace_insensitive)
I'd leave out the search optimisation. Good idea with the pre-rendering, I
ran into similar threading issues but didn't get a chance to figure out
what was happening.

On 2 April 2013 12:38, Lucas Inojosa notifications@github.com wrote:

I could get it much better, as said, following your tips.

1. I couldn't lazily render the toolbar because rendering it inside
   _wrap_streaming_content causes a bug of get_current() returning None.
   Thus I render it outside, just once.
2. The same for smart_unicode(rendered_toolbar + self.tag) (evaluated
   once), except from being rendered inside the wrap function, not outside.
3. Is it safe to search for the tag here: self.tag.lower() in
   chunk.lower(), without calling the smart_unicode? Since the search is
   done inside the replace_insensitive() without additional bottlenecks,
   I call this function in every iteration.

Here is the wrapping function:

def _wrap_streaming_content(self, rendered_toolbar, streaming_content):
    """        Wrap the stream with a new generator that looks for the replacement        tag. Used when the response is a generator for StreamingHttpResponse.        """
    toolbar_and_tag = smart_unicode(rendered_toolbar + self.tag)
    for chunk in streaming_content:
        yield replace_insensitive(smart_unicode(chunk), self.tag, toolbar_and_tag)

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/359#issuecomment-15748570
.

[lucasicf]

Ok. Here is the current new code...

https://github.com/lucasicf/django-debug-toolbar/commit/a3d3791fb11c55efe797cb3cf3e9d404c8652f49

It doesn't seem to have tests related to check the generated response from the original one (without the panel). May I add test covering it and also covering the streaming case?

[mindsocket]

@lucasicf , you might get the attention of the core devs if you put this in as a pull request. I'm guessing they'd welcome additional tests to accompany the change.

I can see the potential for this being useful with Django ticket 13910 - mindsocket/django@cb4fbc2

[lucasicf]

Thanks @mindsocket, patch sent

[aaugustin]

I stumbled on PR #385 before seeing this discussion. The idea of not inserting the DDT in streaming responses is mentioned several times above, and I really believe that's the way to go at this point.

The proper way to insert the DDT in a streaming response would be with an explicit bit of HTML in the template.

[lucasicf]

@aaugustin
Yeah, I was writing an answer on #385 before seeing you answered back here.
We decided to have a try on processing the streaming response rather than ignoring and that was the result. Maybe there's no reason to do this whatsoever.

What do you mean about an explicit bit of HTML? I couldn't figure out...