Opened 8 years ago

Closed 2 years ago

#5241 closed Bug (fixed)

Response middleware runs too early for streaming responses

Reported by: bittor Owned by: aaugustin
Component: HTTP handling Version: master
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 8 years ago.
Patch

Download all attachments as: .zip

Change History (18)

Changed 8 years ago by bittor

Patch

comment:1 Changed 8 years ago by Simon G <dev@…>

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Ready for checkin
  • Version 0.96 deleted

comment:2 Changed 8 years ago by mtredinnick

  • Resolution set to wontfix
  • Status changed from new to 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 Changed 6 years ago by hendrik@…

  • Resolution wontfix deleted
  • Status changed from closed to 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 Changed 6 years ago by mtredinnick

  • Component changed from Internationalization to HTTP handling
  • Triage Stage changed from Ready for checkin to 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 Changed 6 years ago by ccahoon

  • Owner changed from nobody to ccahoon
  • Patch needs improvement set
  • Status changed from reopened to 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 Changed 6 years ago by ccahoon

  • Triage Stage changed from Design decision needed to Fixed on a branch

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

comment:7 Changed 4 years ago by gabrielhurley

  • Severity set to Normal
  • Type set to Bug

comment:8 Changed 3 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:9 Changed 3 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

comment:10 follow-up: Changed 3 years ago by aaugustin

  • Component changed from HTTP handling to Internationalization
  • Has patch unset
  • Patch needs improvement unset
  • Triage Stage changed from Fixed on a branch to 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

comment:11 in reply to: ↑ 10 Changed 3 years ago by claudep

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 Changed 3 years ago by aaugustin

  • Owner changed from ccahoon to aaugustin

comment:13 Changed 3 years ago by aaugustin

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

comment:14 Changed 3 years ago by aaugustin

  • Component changed from Internationalization to HTTP handling
  • Summary changed from i18n not working when the parameter to HttpResponse is an iterator. to Response middleware runs too early for streaming responses
  • Version set to 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 Changed 2 years ago by jacob

  • Triage Stage changed from Design decision needed to Accepted

comment:16 Changed 2 years ago by aaugustin

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 Changed 2 years ago by Aymeric Augustin <aymeric.augustin@…>

  • Resolution set to fixed
  • Status changed from new to closed

In aa089b106b6cfc9a47cd54a0f9eb44bd44811ed9:

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

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