Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Support insertion in StreamingHttpResponse #359

Closed
sirex opened this Issue Mar 9, 2013 · 19 comments

Comments

Projects
None yet
5 participants

sirex commented Mar 9, 2013

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.

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

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.

Contributor

mindsocket commented Mar 22, 2013

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.

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.

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?

Contributor

mindsocket commented Mar 24, 2013

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

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 commented Apr 2, 2013

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)
Contributor

mindsocket commented Apr 2, 2013

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/django-debug-toolbar/django-debug-toolbar/issues/359#issuecomment-15748570
.

lucasicf commented Apr 3, 2013

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?

Contributor

mindsocket commented May 1, 2013

@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

Thanks @mindsocket, patch sent

Contributor

aaugustin commented Oct 14, 2013

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.

@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...

Contributor

aaugustin commented Oct 15, 2013

I meant adding {% if debug %}<script src="{% static "debug_toolbar/insert_me.js %}"></script>{% endif %} to the template.

This could be abstracted in a templatetag: {% load debug_toolbar %}{% debug_toolbar %}.

Contributor

aaugustin commented Oct 15, 2013

For the time being, I've disabled insertion of the toolbar in streaming responses. See 142caaa.

I consider this sufficient to make a release that officially supports 1.5. We can decide whether #385 is a good idea later.

Contributor

aaugustin commented Oct 26, 2013

I've updated the title to reflect the status of this issue.

Contributor

aaugustin commented Oct 27, 2013

After taking some time to think about this issue, I still believe ignoring streaming responses is the way to go.

Thanks everyone for your contributions. Sorry for not taking advantage of your work. I wasn't a maintainer yet when this discussion started.

@aaugustin aaugustin closed this Oct 27, 2013

jowolf commented Apr 24, 2015

I'm still getting this on Django 1.7.7 and DjDT 1.3.0:

"This FileResponse instance has no content attribute. Use streaming_content instead."

Traceback:
File "/usr/local/lib/python2.7/dist-packages/django/core/handlers/base.py" in get_response
204.                 response = middleware_method(request, response)
File "/usr/local/lib/python2.7/dist-packages/debug_toolbar/middleware.py" in process_response
94.             new_response = panel.process_response(request, response)
File "/usr/local/lib/python2.7/dist-packages/w3c_validator/panels.py" in process_response
38.         self.validator.validate_source(response.content)
File "/usr/local/lib/python2.7/dist-packages/django/http/response.py" in content
377.             "Use `streaming_content` instead." % self.__class__.__name__)

Exception Type: AttributeError at /assets/css/skin.css
Exception Value: This FileResponse instance has no `content` attribute. 
Use `streaming_content` instead.

Is there a config change I need to make, or a workaround?

JJW

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