Opened 17 years ago

Closed 12 years ago

#5241 closed Bug (fixed)

Response middleware runs too early for streaming responses

Reported by: bittor Owned by: Aymeric Augustin
Component: HTTP handling Version: dev
Severity: Normal Keywords: internationalization, progresive rendering
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

In order to output very large HTML files or do progresive rendering I let a view return an iterator. The actual HTML file is generated at the last step os request processing. It works fine as long as I don't use gettext to translate any variable/string/template/... during generation. If I do, I always get the default translation.

I hope the following code snippet will clarify what I mean:

def test(request):
    def f():
        print dict(request.session)
        yield "<html><body>\n"
        for lang in ('es', 'en'):
            yield '<a href="/i18n/setlang/?language=%(lang)s">%(lang)s</a><br>'%locals()
        for i in range(10):
            yield (_('Current time:')+' %s<br>\n')%datetime.datetime.now()
            time.sleep(1)
        yield "Done\n</body></html>\n"
    return HttpResponse(f())

In this case 'Current time:' is never translated (it is easy to fix in this case but not in others).

I found the problem and the patch working with Django 0.96 but I believe it also applies to the development version.

Attachments (1)

locale.diff​ (1.1 KB ) - added by bittor 17 years ago.
Patch

Download all attachments as: .zip

Change History (18)

by bittor, 17 years ago

Attachment: locale.diff​ added

Patch

comment:1 by Simon G <dev@…>, 17 years ago

Triage Stage: Unreviewed → Ready for checkin
Version: 0.96

comment:2 by Malcolm Tredinnick, 17 years ago

Resolution: → wontfix
Status: new → closed

Passing iterators to HttpResponse and hoping they won't be entirely pulled into memory is not supported. There are too many pieces of middleware and other code that assume they can do random (or at least repeatable) access to the response.content attribute.

So it's not worth doing this kind of workaround piecemeal, without a clear plan as to how to write iterator-aware middleware (which I suspect is close to impossible, given the need to access things like the length, in many cases) or specify that certain pieces of middleware shouldn't be run.

comment:3 by hendrik@…, 16 years ago

Resolution: wontfix
Status: closed → reopened

I am working on a site (my own) that needs translations for both Dutch and English. I found this patch to work for situations where long translation strings are used in templates in combination with for loops.

For example, in my case, I have some text (not even too long, but it "expands" to multiple lines in the .po file), and a for loop:

{% trans "SmashBits werkt aan software. Voor op het web, of voor op de Mac. Over het laatste binnenkort meer." %}

{% comment %}
{% for lang in LANGUAGES %} 
<div class="lang{% if forloop.first %} first{% endif %}">
<form action="/i18n/setlang/" method="POST" name="Form_{{ lang.0 }}">
	<input name="next" type="hidden" value="/">
	<input type="hidden" name="language" value="{{ lang.0 }}">
	<a href="#" onclick="document.Form_{{ lang.0 }}.submit();">{{ lang.1 }}</a>
</form>
</div>
{% endfor %}
{% endcomment %}

<div class="lang first">
<form action="/i18n/setlang/" method="POST" name="Form_en">
	<input name="next" type="hidden" value="/">
	<input type="hidden" name="language" value="en">
	<a href="#" onclick="document.Form_en.submit();">English</a>
</form>
</div>
<div class="lang">
<form action="/i18n/setlang/" method="POST" name="Form_nl">
	<input name="next" type="hidden" value="/">
	<input type="hidden" name="language" value="nl">
	<a href="#" onclick="document.Form_nl.submit();">Nederlands</a>
</form>
</div>

The text is in Dutch, but will be translated when the user has English as the default language (as can be set by using one of the two forms). The trick is that when I use the code in the {% comment %} block, the text is not translated, but when I simply write it all down, translating works!

I don't have too much Python experience, let alone that I know anything about the Django architecture, but the patch works! I reopened this issue because I think this is an issue with the iterator of the template, i.e. the for tag.

(patch/reproduction done on 1.0.2.)

comment:4 by Malcolm Tredinnick, 16 years ago

Component: Internationalization → HTTP handling
Triage Stage: Ready for checkin → Design decision needed

This isn't "ready for checkin" for so many reasons. Firstly, the patch isn't very good (there shouldn't be any reason to use new.instancemethod; it can be done similarly to other places where methods are created in Django). Secondly, this is only one tiny piece of allowing iterators to work with HttpResponses and piecemeal work on that isn't particularly useful at this point. They are simply unsupported right now, as I noted above.

"The patch works" is only one part of any solution. It has to solve a problem properly, not just make the symptom go away.

comment:5 by ccahoon, 15 years ago

Owner: changed from nobody to ccahoon
Patch needs improvement: set
Status: reopened → new

This looks like it is related to #6527. I am going to work through and see if the changes for #6527 made any changes to this piece.

comment:6 by ccahoon, 15 years ago

Triage Stage: Design decision needed → Fixed on a branch

Fixed in branches/soc2009/http-wsgi-improvements. This was fixed by the same changes that fixed #6527.

comment:7 by Gabriel Hurley, 14 years ago

Severity: → Normal
Type: → Bug

comment:8 by Aymeric Augustin, 13 years ago

UI/UX: unset

Change UI/UX from NULL to False.

comment:9 by Aymeric Augustin, 13 years ago

Easy pickings: unset

Change Easy pickings from NULL to False.

comment:10 by Aymeric Augustin, 12 years ago

Component: HTTP handling → Internationalization
Has patch: unset
Patch needs improvement: unset
Triage Stage: Fixed on a branch → Design decision needed

To be clear, the problem is that LocaleMiddleware.process_response deactivates the current translation, and that everything that runs afterwards uses the default locale.

88b1183425631002a5a8c25631b1b1fad7eb23c5 (the commit on the http-wsgi-improvements branch) isn't acceptable because it removes HttpResponse's current ability to stream. People already use streaming responses (even though they're fragile), and #7581 will add official support for them.

But improvements to streaming responses aren't going to fix this:

  • process_response runs before beginning to send the response;
  • the point of streaming responses is to generate content on the fly while sending the response.

Stupid question — why does LocaleMiddleware need to deactivate the translation in process_response? Couldn't it leave the translation in effect until the next request? This behavior has been here since the merge of the i18n branch: ​https://github.com/django/django/commit/5cf8f684237ab5addaf3549b2347c3adf107c0a7#L48R20

in reply to:  10 comment:11 by Claude Paroz, 12 years ago

Replying to aaugustin:

Stupid question — why does LocaleMiddleware need to deactivate the translation in process_response? Couldn't it leave the translation in effect until the next request? This behavior has been here since the merge of the i18n branch: ​https://github.com/django/django/commit/5cf8f684237ab5addaf3549b2347c3adf107c0a7#L48R20

+1 to remove translation deactivation, unless the test suite proves we're wrong.

comment:12 by Aymeric Augustin, 12 years ago

Owner: changed from ccahoon to Aymeric Augustin

comment:13 by Aymeric Augustin, 12 years ago

Simply removing translation.deactivate() causes some failures, but that's more a lack of isolation in the test suite than anything else.

comment:14 by Aymeric Augustin, 12 years ago

Component: Internationalization → HTTP handling
Summary: i18n not working when the parameter to HttpResponse is an iterator. → Response middleware runs too early for streaming responses
Version: → master

The general problem is that middleware's process_response (and possibly process_exception) run before the content of a streaming response has been generated. This doesn't match the current assumption that these middleware run after the response is generated.

At least the transaction middleware suffers from the same problem. If we can't fix both at the same time, we should open another ticket for that one.

Off the top of my head -- we could:

  • run these middleware methods at a later point,
  • introduce a new middleware method for streaming responses.

#19519 is related, but it's a different problem -- it's about firing the request_finished signal.

comment:15 by Jacob, 12 years ago

Triage Stage: Design decision needed → Accepted

comment:16 by Aymeric Augustin, 12 years ago

After thinking about this problem for some time, I don't think it's feasible to run the middleware after starting a streaming response.

Since you already started sending output:

  • You cannot change headers — the primary task of process_response
  • You cannot handle exceptions gracefully — the primary task of process_exception

So the answer here is to stop deactivating the translation in process_response.

comment:17 by Aymeric Augustin <aymeric.augustin@…>, 12 years ago

Resolution: → fixed
Status: new → closed

In aa089b106b6cfc9a47cd54a0f9eb44bd44811ed9:

Fixed #5241 -- Kept active transalation in LocaleMiddleware.process_response.

Note: See TracTickets for help on using tickets.
Back to Top