Backport for ticket 34063?
262 views
Skip to first unread message
James Bennett
Dec 29, 2022, 7:44:43 PM12/29/22
to Django Developers
https://code.djangoproject.com/ticket/34063
The short summary of that ticket is that there was a significant
behavioral difference between django.test.Client and
django.test.AsyncClient: with AsyncClient, any view or middleware
which attempted to access request.POST without first accessing
request.body would raise an exception with an obscure message about
trying to read too much data from the request body. This was fixed
last month:
https://github.com/django/django/commit/c4eaa67e2b880db778c9fe6d9854fbdfcc16ecd2
However, it does not explicitly fall under any of the criteria for
backporting to older releases
(https://docs.djangoproject.com/en/dev/internals/release-process/#supported-versions)
so at the moment, the fix will only be available from Django 4.2
onward, and no previous Django versions will receive a backported
version of the fix (though the bug is present back to at least Django
3.2, and I suspect but haven't confirmed all the way back to the
introduction of AsyncClient in Django 3.1).
I'd like to argue for an exception to the normal backport policy, and
for #34063 to be backported to Django 4.1, 4.0, and 3.2 LTS.
My reasoning here is that async support is an incredibly important
part of the Python web ecosystem right now, and there is no good
alternative to django.test.AsyncClient for testing some kinds of async
code -- although the normal django.test.Client can hit URLs served by
async views, it does not always trigger async code paths inside Django
when doing so. For example, I came across #34063 while trying to test
a middleware that supports both sync and async usage; only AsyncClient
reliably exercises the async code path of such a middleware, but that
triggers the bug unless I add throwaway accesses of request.body.
Especially for authors of reusable applications/libraries that try to
support a broad range of Django versions, many of whom (like me!) are
finally getting around to async support, this is an unpleasant
situation, since it will require the request.body workaround to be
left in place in all such code for at least another 16 months or so
(which is when Django 3.2 LTS will finally drop out of upstream
support). And that's presuming such authors even know to put in the
workaround -- it took me quite a bit of debugging and following up on
irrelevant search results before I finally found #34063, and that's as
someone who has deep familiarity with Django -- someone less
experienced at delving into Django's internals to figure out root
causes and likely search terms would probably have had an even worse
time of it.
Finally, if it had been discovered prior to any of the releases in
which AsyncClient has been present, this would have been a clear
release blocker; it's quite a serious bug that seems to have gone
unnoticed for so long largely by accident (and, ironically, by
under-testing of the testing tools).
So I think the right thing to do, to help with Django's transition
into async support and to avoid imposing unnecessary burdens of
debugging on application and library authors, is to backport the fix
from #34063 to the full set of currently-supported Django releases:
4.1, 4.0, and 3.2 LTS.
The short summary of that ticket is that there was a significant
behavioral difference between django.test.Client and
django.test.AsyncClient: with AsyncClient, any view or middleware
which attempted to access request.POST without first accessing
request.body would raise an exception with an obscure message about
trying to read too much data from the request body. This was fixed
last month:
https://github.com/django/django/commit/c4eaa67e2b880db778c9fe6d9854fbdfcc16ecd2
However, it does not explicitly fall under any of the criteria for
backporting to older releases
(https://docs.djangoproject.com/en/dev/internals/release-process/#supported-versions)
so at the moment, the fix will only be available from Django 4.2
onward, and no previous Django versions will receive a backported
version of the fix (though the bug is present back to at least Django
3.2, and I suspect but haven't confirmed all the way back to the
introduction of AsyncClient in Django 3.1).
I'd like to argue for an exception to the normal backport policy, and
for #34063 to be backported to Django 4.1, 4.0, and 3.2 LTS.
My reasoning here is that async support is an incredibly important
part of the Python web ecosystem right now, and there is no good
alternative to django.test.AsyncClient for testing some kinds of async
code -- although the normal django.test.Client can hit URLs served by
async views, it does not always trigger async code paths inside Django
when doing so. For example, I came across #34063 while trying to test
a middleware that supports both sync and async usage; only AsyncClient
reliably exercises the async code path of such a middleware, but that
triggers the bug unless I add throwaway accesses of request.body.
Especially for authors of reusable applications/libraries that try to
support a broad range of Django versions, many of whom (like me!) are
finally getting around to async support, this is an unpleasant
situation, since it will require the request.body workaround to be
left in place in all such code for at least another 16 months or so
(which is when Django 3.2 LTS will finally drop out of upstream
support). And that's presuming such authors even know to put in the
workaround -- it took me quite a bit of debugging and following up on
irrelevant search results before I finally found #34063, and that's as
someone who has deep familiarity with Django -- someone less
experienced at delving into Django's internals to figure out root
causes and likely search terms would probably have had an even worse
time of it.
Finally, if it had been discovered prior to any of the releases in
which AsyncClient has been present, this would have been a clear
release blocker; it's quite a serious bug that seems to have gone
unnoticed for so long largely by accident (and, ironically, by
under-testing of the testing tools).
So I think the right thing to do, to help with Django's transition
into async support and to avoid imposing unnecessary burdens of
debugging on application and library authors, is to backport the fix
from #34063 to the full set of currently-supported Django releases:
4.1, 4.0, and 3.2 LTS.
Carlton Gibson
Dec 30, 2022, 3:02:04 AM12/30/22
to django-d...@googlegroups.com
Hey James.
Grrr. 😬
I don't think this justifies an exception to the backport policy. It's not significantly different from any of any number of other fixes that folks ask for a backport for and we say no. Fielding "but you backported that one" isn't an extra task that's going to be helpful.
When I looked at the trace you posted in IRC yesterday, my first thought was "3.2?". I think supporting Django 3.2 at this point isn't worth the effort.
Yes, sure it's not EOL for another year or so but if folks want to use async they should be on the latest version.
There's significant changes still in each version, and sticking on the old LTS is asking for trouble. (I certainly don't see making exceptions to the backport policy to help out such to be a good idea. The old LTS is for frozen apps, not new development.)
You're wanting to add async support to your third-party library: super; just say it's 4.2+ and move on.
Folks who aren't on the LTS will be updating shortly enough anyway. A little-bit of carrot never hurt.
Kind Regards,
Carlton
Carlton
--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/CAL13Cg-Pexnfd7ynnOWEoLkmfM5gYhcMfHgkYzOG7nH8sWA9sQ%40mail.gmail.com.
James Bennett
Dec 30, 2022, 4:04:52 AM12/30/22
to django-d...@googlegroups.com
On Fri, Dec 30, 2022 at 12:01 AM Carlton Gibson
<carlton...@gmail.com> wrote:
> When I looked at the trace you posted in IRC yesterday, my first thought was "3.2?". I think supporting Django 3.2 at this point isn't worth the effort.
It's also broken in 4.0 and 4.1. I just posted the first trace I got
<carlton...@gmail.com> wrote:
> When I looked at the trace you posted in IRC yesterday, my first thought was "3.2?". I think supporting Django 3.2 at this point isn't worth the effort.
back from my test matrix, which happened to be a Django 3.2 run.
> Yes, sure it's not EOL for another year or so but if folks want to use async they should be on the latest version.
async views/middlewares, and for testing them with AsyncClient, since
3.1. Telling people that actually it was badly broken to the point of
almost unusability until 4.2 and thus not to use any of those versions
that claimed to have the support is... suboptimal. Especially given,
as I noted, the difficulty for Django novices of even figuring out
what the heck is going on when they try to test an async view and get
a weird exception.
Or to put it another way: if I submitted patches against the 3.2, 4.0,
and 4.1 branches for docs/topics/testing/tools.txt that put a big
"WARNING! BROKEN! DO NOT USE!" on all mentions of AsyncClient, I doubt
those patches would be well-received. But that's basically what those
releases of Django *need* at this point. It's intolerably misleading
to end users of the framework to have docs for those versions claiming
that you can write async code and test it with the AsyncClient when...
well, you can't, really, unless you know the precise way that it's
broken and put explicit workarounds for the brokenness in all your
views and middlewares.
> You're wanting to add async support to your third-party library: super; just say it's 4.2+ and move on.
support until Django 4.2 comes out (approximately two and a half years
after Django itself claimed to officially support this stuff), and
that they have to cut off all older versions of Django when they do so
*despite those versions officially claiming to support async* just
seems like a recipe for those authors deciding not to trust or waste
their time on Django.
So I *really* think this one needs an exception to the backports
policy. Async is too important, and Django's reputation is too
important, to leave 3.2, 4.0, and 4.1 as badly broken as they
currently are.
Carlton Gibson
Dec 30, 2022, 4:09:31 AM12/30/22
to django-d...@googlegroups.com
James,
All you're talking about is adding this to your test cases right?
# Work around Django #34063 until 4.2.request.body# ... continue
C.
--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/CAL13Cg8ZjzR%3Dzd4PwY%2Bmg1pUMXzPDUx_0ZPetCam5xEM7xqS1w%40mail.gmail.com.
James Bennett
Dec 30, 2022, 4:13:34 AM12/30/22
to django-d...@googlegroups.com
On Fri, Dec 30, 2022 at 1:09 AM Carlton Gibson <carlton...@gmail.com> wrote:
> All you're talking about is adding this to your test cases right?
>
> # Work around Django #34063 until 4.2.
> request.body
As far as I can tell it needs to go in whatever code will *read*
> All you're talking about is adding this to your test cases right?
>
> # Work around Django #34063 until 4.2.
> request.body
request.POST, not the code that generates the request. So that means a
no-op "request.body" in every view and every middleware that might
read request.POST. And that includes Django's own built-in views
(including generic views) and any built-in async-supporting
middlewares, if they have the misfortune to get called on the async
path from an AsyncClient-initiated request.
Carlton Gibson
Dec 30, 2022, 4:27:57 AM12/30/22
to django-d...@googlegroups.com
OK, yes, that's probably right.
Had #34063 been reported during the 3.1 cycle it would have been a release blocker.
As it was, it wasn't reported until 4.0 (when main was already 4.1) so two cycles later.
As such it no longer qualifies for a backport.
It's frustrating when this happens, but the backport policy has proven its worth time and again.
I **really** don't see the case for making an exception here.
(The policy has more value than the inconvenience in any of these cases, or even all of them summed.)
> As it was, it wasn't reported until 4.0 ...
I think the bottomline is people are only starting to use this stuff now. It's still experimental, for whatever that means, and there simply are and will be rough edges.
I'd be very happy to think about how we should message that better — a big sign saying "Still in development: if you want to use async, be on the latest versions" would be fine in my opinion.
I think we've been pretty upfront that that's the case.
Adding the support here is difficult and slow enough. Backporting fixes to discovered bugs, outside the established policy, is making an unrealistic burden for ourselves.
--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/CAL13Cg_1MMt2DbcC%2B4Vfzi83g_U-bBNj2-J92Y1NWTvAS9c1Ww%40mail.gmail.com.
James Bennett
Dec 30, 2022, 4:38:42 AM12/30/22
to django-d...@googlegroups.com
On Fri, Dec 30, 2022 at 1:27 AM Carlton Gibson <carlton...@gmail.com> wrote:
> It's frustrating when this happens, but the backport policy has proven its worth time and again.
> I **really** don't see the case for making an exception here.
> (The policy has more value than the inconvenience in any of these cases, or even all of them summed.)
I don't see the value here.
> It's frustrating when this happens, but the backport policy has proven its worth time and again.
> I **really** don't see the case for making an exception here.
> (The policy has more value than the inconvenience in any of these cases, or even all of them summed.)
If it's left as-is, the message is, effectively, that Django lied
about its async support for years, but suddenly *now* is asking people
to trust the claims of async support. Unless another major bug gets
found, in which case "well, *now* you can trust Django on async" will
get kicked down the road again.
This is a showstopper of a bug, and just leaving it deliberately
unfixed in *every released version of Django* because "policy said no"
has the potential to do a lot of damage to Django's reputation.
> Adding the support here is difficult and slow enough. Backporting fixes to discovered bugs, outside the established policy, is making an unrealistic burden for ourselves.
that literally prevents the built-in testing tool from working with a
huge swathe of views and middlewares (including potentially some built
in to Django itself), *when that tool is documented and Django claims
to officially support it*, is just unacceptable. I'll work up the
patches myself if that's what it takes.
Carlton Gibson
Dec 30, 2022, 5:04:27 AM12/30/22
to django-d...@googlegroups.com
James,
I think the backport policy has proven itself over the years, no?
"Django lied" — that's a bit melodramatic don't you think?.
Django introduced a new feature, as it is wont to do.
There was a bug in that new feature, as there are wont to be.
Unfortunately that bug was not discovered during the timeframe in which it would qualify for a backport.
However it's fixed in the upcoming version, which will be in pre-release in just a couple of weeks.
Sorry for the inconvenience, but backport and supported versions policy is the foundation of the stability and ease of update you've come to appreciate in Django over all these years.
> This is a showstopper of a bug...
No it's not. It's a bug in AsyncClient and AsyncRequestFactory, that means if you're using those on older versions of Django, you'll need to work around.
This is no different than any of a thousand other cases where there's been a bug in an older version that folks have needed to account for.
> Unless another major bug gets found...
I'm quite certain there are plenty more of these to be found.
The journey is still really only just beginning.
I'd guess I'd say we're entering a new phase, but the async support is neither complete nor mature.
There will be bugs.
I'm -1 on making an exception to the backport policy in this case. (Sorry I don't agree with you.)
Kind Regards,
Carlton
Carlton
--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/CAL13Cg-tMbTXN3WwqjqjgiLj1rrMO6K8YhRHnci6Qzw2d52Y-w%40mail.gmail.com.
James Bennett
Dec 30, 2022, 5:34:32 AM12/30/22
to django-d...@googlegroups.com
On Fri, Dec 30, 2022 at 2:04 AM Carlton Gibson <carlton...@gmail.com> wrote:
No it's not. It's a bug in AsyncClient and AsyncRequestFactory, that means if you're using those on older versions of Django, you'll need to work around.This is no different than any of a thousand other cases where there's been a bug in an older version that folks have needed to account for.
I believe it is very different and have explained why repeatedly.
But if it is to be left unfixed, what is the plan for how to communicate the workaround to people who will be running into this bug until at least April of 2024? And what is the plan for communicating to them that Django is aware async testing is broken and the official policy is to knowingly leave it broken in every currently-released version of the framework? Even if it will not be fixed for older versions, Django 4.1 ought to be eligible for a backport. And I strongly urge allowing a backport to 4.0 and 3.2, though I do not know what could convince you at this point.
This bug is bad. But this response to it is eroding my trust in Django, and I hope you know that is not something I say lightly. I think it also is going to erode the trust of other people when they find out about it, and is going to do so at a critical time when Django is facing stiff competition in the async space and cannot afford to lose more ground or be seen as unwilling to support its own claimed async features.
Carlton Gibson
Dec 30, 2022, 7:57:00 AM12/30/22
to django-d...@googlegroups.com
> Even if it will not be fixed for older versions, Django 4.1 ought to be eligible for a backport.
What you're suggesting is a change to the backport policy. That may be the right thing to do, but it would be quite a significant change.
These issues — where a bug report/fix comes outside the backport period — are quite common. It's always a bit frustrating that we can't backport, but the stability of released versions has been taken (for years, by the whole community — by the folks hitting the particular non-backported issue this time :) as correct on balance.
> But this response to it is eroding my trust in Django, and I hope you know that is not something I say lightly.
Since I'm the only person who's responded thus far, I can't help but read that as saying it erodes your trust in me. That's sad. Especially since all I've done is not agree with you. :)
Nonetheless you can invoke the Steering Council if no-one else responds, and you feel it's as bad as all that.
> ...a critical time when Django is facing stiff competition...
Django's USP is stability and ease of update, not async. The backport, stability, and supported versions policies are the cornerstone of that, I believe. It's not something we should endanger.
Other frameworks move faster, but they don't have extended LTS lifetimes to consider.
I see the success of FastAPI in particular. Super.
But I don't see it as any different to Flask, say, in the previous period.
May a thousand flowers bloom.
I don't see it as an existential threat to Django. Quite the contrary: What a rich and wonderful time it is for the Python web framework ecosystem as whole. (This ecosystem is itself under threat from JavaScript, I'm constantly told, which will not survive Rust, I hear, ... and so on.)
My view is that Django's async support is maturing nicely, slowly, but nicely and it's grows every release. That will continue for a few more cycles yet. Bug fixes in new features should be backported within the policy yes, but not beyond that. Better guidance in the asynchronous support docs saying, "hey, do update, this stuff is still growing" may well be in order. I have this in the Channels 4.0 release notes: "The async support in both Python and Django continues to evolve rapidly. We advise you to always upgrade to the latest versions in order to avoid issues in older versions if you’re building an async application." — I don't think similar in Django would hurt. /my view.
> ...as unwilling to support ...
Following the backport policy is not being unwilling to support.
Sorry (again) that I don't share your view. Sorry that undermines your faith (in me) — I assume we still think there's room for rational disagreement on how to weigh competing concerns in these kinds of cases.
I'm going to mute this thread until after the new year now.
Enjoy the festivities.
Kind Regards,
Carlton
Carlton
--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/CAL13Cg_5wFeiQjEbOfRYJGgZ2Hn3cQoJHoW-CSUSGD9xuNehoA%40mail.gmail.com.
James Bennett
Dec 30, 2022, 12:48:47 PM12/30/22
to django-d...@googlegroups.com
I have put it to the Steering Council:
https://forum.djangoproject.com/t/request-for-technical-board-steering-council-vote-requested-backport-ticket-34063/17920/1
https://forum.djangoproject.com/t/request-for-technical-board-steering-council-vote-requested-backport-ticket-34063/17920/1
Tim Graham
Dec 30, 2022, 5:24:51 PM12/30/22
to Django developers (Contributions to Django itself)
As perfectionists, it's always hard to say (and hear) "no" when this sort of request comes up. I'm unconvinced it's a serious issue that requires a break from normal policy. (Unreported for 2 years in 4 major releases; simple workaround present.)
Incidentally, escalating an issue to the steering council after less than 24 hours (during a holiday week, no less) probably isn't something we want to model as a best practice in forming consensus.
Kevin Grinberg
Dec 30, 2022, 5:59:22 PM12/30/22
to Django developers (Contributions to Django itself)
I'll say upfront that I haven't hit this particular issue, but it's mostly because I've avoided the Django async stack after some challenging experiences on the old(er) channels/daphne/etc. stack and its evolution. I've personally been in the "let's see how this develops" camp, which admittedly isn't the most community-focused. I'll resolve to do better in the new year.
This does sort of get at one aspect of this issue though - there's less usage of the async stuff as it's being rolled out slowly across multiple releases (which it itself an experiment of sorts), in part because [at least] some people don't trust it to actually be stable and are waiting to see the whole story before committing projects more fully.
So from the perspective of "how many users are impacted", I agree the impact is probably low. But unless I'm misunderstanding the nature of the bug, this seems like it basically makes async views un-testable without a pretty annoying workaround (assuming it's more than a handful of views)? Plus the fact that it's not easily discoverable.
FWIW (I'm an n of 1 obviously), but I go LTS-to-LTS for most projects, just given uncertainty in when I'll be upgrading any particular project, and wanting to be certain I never fall out of support for at least security releases. So for my own personal needs, fair enough - by the time I migrate my 3.2 projects to 4.2, this will be fixed, and it's unlikely that I'll personally be experimenting with async views in the next 3 months or so. But that doesn't really seem like the sort of behavior we want to encourage, if we're trying to actually get people to use async, get some real-world usage, and signal that Django has a good async story.
I'm very cognizant of maintenance burden and feel sheepish advocating for someone else to do work, but if this thread is asking for opinions, I do think it's important to backport this issue. I can't literally equate it to a data loss bug (the specific example in the policy), but I'd argue that the gradual nature of the async rollout and its importance in moving Django forward mitigates for an exception to the backport policy.
Cheers,
Kevin
This does sort of get at one aspect of this issue though - there's less usage of the async stuff as it's being rolled out slowly across multiple releases (which it itself an experiment of sorts), in part because [at least] some people don't trust it to actually be stable and are waiting to see the whole story before committing projects more fully.
So from the perspective of "how many users are impacted", I agree the impact is probably low. But unless I'm misunderstanding the nature of the bug, this seems like it basically makes async views un-testable without a pretty annoying workaround (assuming it's more than a handful of views)? Plus the fact that it's not easily discoverable.
FWIW (I'm an n of 1 obviously), but I go LTS-to-LTS for most projects, just given uncertainty in when I'll be upgrading any particular project, and wanting to be certain I never fall out of support for at least security releases. So for my own personal needs, fair enough - by the time I migrate my 3.2 projects to 4.2, this will be fixed, and it's unlikely that I'll personally be experimenting with async views in the next 3 months or so. But that doesn't really seem like the sort of behavior we want to encourage, if we're trying to actually get people to use async, get some real-world usage, and signal that Django has a good async story.
I'm very cognizant of maintenance burden and feel sheepish advocating for someone else to do work, but if this thread is asking for opinions, I do think it's important to backport this issue. I can't literally equate it to a data loss bug (the specific example in the policy), but I'd argue that the gradual nature of the async rollout and its importance in moving Django forward mitigates for an exception to the backport policy.
Cheers,
Kevin
James Bennett
Dec 30, 2022, 7:37:05 PM12/30/22
to django-d...@googlegroups.com
On Fri, Dec 30, 2022 at 2:24 PM Tim Graham <timog...@gmail.com> wrote:
> As perfectionists, it's always hard to say (and hear) "no" when this sort of request comes up.
I wrote a 2,000-word argument explaining why I believe this warrants
> As perfectionists, it's always hard to say (and hear) "no" when this sort of request comes up.
backporting. I think that deserves more engagement than just a "no".
> I'm unconvinced it's a serious issue that requires a break from normal policy. (Unreported for 2 years in 4 major releases; simple workaround present.)
explained at length why I think it is serious enough to warrant an
exception (and why I believe the policy should also be
revisited/revised to accommodate cases like this explicitly).
> Incidentally, escalating an issue to the steering council after less than 24 hours (during a holiday week, no less) probably isn't something we want to model as a best practice in forming consensus.
falls into that category, too. And I will point out here, as I did in
the forum thread, that the discussion here seemed to be blocked on a
Merger who was not convinced and *who suggested the Steering Council
as an option*. And procedurally, one point of the SC is to serve as a
"court of appeal" if someone cannot convince the Mergers of the merits
of a minor change.
Meanwhile, I think Kevin is pointing in the direction of something
important, which is that it is very easy to wind up in a vicious cycle
where people get the impression that Django does not take its async
story seriously and so they stay away from async Django, which then is
used as an argument for not taking the async story as seriously
(because obviously async Django is not that widely used), which then
gives people the impression...
Async is hard enough without Django making it harder still. So I think
it is important for Django to make clear that the async story is
supposed to be good and that the Django project is committed to
supporting async. Right now, a developer who decides to try out async
Django, with any currently-supported release, and follows the obvious
"how to test my async code" clues in the documentation, will very
likely end up with something broken. Worse: the breakage will be hard
to diagnose, harder to find out the workaround for, and if they do
manage to find it they'll likely also be treated to one or more
threads of the Django project saying that due to the technicalities of
when the breakage was first formally reported, none of the currently
released and allegedly bugfix-supported versions of Django are
actually eligible to receive a fix.
I put it to you that this is not a good experience for the developer,
that it is not the experience the Django project should be inflicting
on the developer, that it will tend to erode the developer's trust and
confidence in Django if this is the path Django decides to take, and
that the solution to all this is comparatively easy: backport the
frankly pretty minimal fix for the async test client into the full set
of currently-supported feature releases of Django as a show of good
faith and support, and to make async support easier for the community
and the overall ecosystem.
Carlton Gibson
Dec 31, 2022, 2:22:59 AM12/31/22
to django-d...@googlegroups.com
Just to clarify this point, which I think has been glossed over:
This isn't correct.
Under normal circumstances you just use the sync Client, as you've always done. `response = client.get(`/my-async-view/`)`.
Django handles that the **view** is async for you.
It's only if you need to write an actual **async test**, i.e. an `async def` test case that you need the async client and factory.
The factory can use the suggested `request.body` workaround if needed.
Where you need async def tests, dropping down to the request factory is available. An example might be testing a middleware for async
support in isolation, which fits the request factory model directly.
The `AsyncClient` is affected, yes. The question is whether that justifies a backport at this very late hour.
--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/f9fc0dad-fb09-4c5b-8c68-a83375f42d2en%40googlegroups.com.
James Bennett
Dec 31, 2022, 3:40:11 AM12/31/22
to django-d...@googlegroups.com
On Fri, Dec 30, 2022 at 11:22 PM Carlton Gibson
<carlton...@gmail.com> wrote:
> Under normal circumstances you just use the sync Client, as you've always done. `response = client.get(`/my-async-view/`)`.
> Django handles that the **view** is async for you.
>
> It's only if you need to write an actual **async test**, i.e. an `async def` test case that you need the async client and factory.
Well, no. And I have been over this at least once in my explanations.
<carlton...@gmail.com> wrote:
> Under normal circumstances you just use the sync Client, as you've always done. `response = client.get(`/my-async-view/`)`.
> Django handles that the **view** is async for you.
>
> It's only if you need to write an actual **async test**, i.e. an `async def` test case that you need the async client and factory.
But I will explain it again as succinctly as I can.
First: the Django testing documentation steers readers toward
AsyncClient for asynchronous testing, especially for a consistently
async codebase. Which is something that has become table stakes, or
perhaps even more basic than that, for any Python framework claiming
to support async. Given that Python itself imposes limitations on use
of async code in synchronous contexts (with "await" literally being a
syntax error in any but an async context), telling people to just
write sync tests with the sync client is neither an acceptable
substitute for just fixing the AsyncClient, nor is it compatible with
the fact that the documentation currently prominently advertises
AsyncClient and its ability to let you write a full async codebase,
tests and all.
Second: as I *know* I have explained, it can be difficult to predict
in advance which code path Django will choose for mixed sync/async
code, because of all the work Django does to try to make them coexist
nicely. Any developer who wants to ensure async code paths are
actually tested has no useful alternative to AsyncClient, because only
AsyncClient is natively an ASGI client and thus will reliably trigger
async code paths through Django.
> The factory can use the suggested `request.body` workaround if needed.
both the workaround and the need for it to casual readers of the
documentation. If no exception can be made to the backport policy for
fixing the async test support, then no exception should be made to
change the documentation to tell people to use the workaround. And if
an exception is to be made, the exception which causes the least
damage to Django and the least overall ongoing burden is just to *fix
the async test support*. As I have been advocating.
> The `AsyncClient` is affected, yes. The question is whether that justifies a backport at this very late hour.
longer explanations, and then over on the forum I wrote over two
thousand words explaining in detail why I believe the backport is in
fact justified, and which anticipated and addressed several arguments
now being raised here. So I ask that people actually read and engage
with that in future replies. This is the bare minimum of respect and
good faith that can be expected of participants in discussions on this
and other forums concerning Django and its development.
Adam Johnson
Dec 31, 2022, 5:12:51 AM12/31/22
to django-d...@googlegroups.com
This thread got too long too fast: it's a hell of a lot to catch up on. In the future, please can we all let things sit for a bit so other voices can chime in, especially when the issue has concretely affected very few users.
In the past I have also been frustrated at particular bug fixes not being backported. But I've gradually come to appreciate just how valuable the backport policy is. It keeps Django forward-facing, and prevents the fellows spending much of their precious time backporting.
Also, I’ve learned several techniques to backport within projects when required (e.g. https://adamj.eu/tech/2020/07/29/backporting-a-django-orm-feature-with-database-instrumentation/ ). I encourage others to do the same. We could perhaps document some of these techniques (one source: Paul Ganssle’s talk ”What to Do When the Bug is in Someone Else’s Code ”: https://ganssle.io/talks/#upstream-bugs ).
I also believe that there is no overriding reason to backport in this case. Self-backporting the fix within a project is very feasible—there’s no need for "the request.body workaround" (which is an understandable source of frustration).
I have created a repo demonstrating three different ways to self-backport the fix: https://github.com/adamchainz/django-ticket-34063-backport (client only, not request factory). The code for those three techniques follows. I hope you can use one of these, James.
1. Monkey-patching
import django
from django import test
if django.VERSION < (4, 2):
# Backport of https://code.djangoproject.com/ticket/34063
from django.core.handlers.wsgi import LimitedStream
from django.test.client import AsyncClientHandler
orig_get_response_async = AsyncClientHandler.get_response_async
async def get_response_async(self, request):
request._stream = LimitedStream(
request._stream,
len(request._stream),
)
return await orig_get_response_async(self, request)
AsyncClientHandler.get_response_async = get_response_async
class IndexTests(test.TestCase):
async def test_post(self):
await self.async_client.post("/")
from django import test
if django.VERSION < (4, 2):
# Backport of https://code.djangoproject.com/ticket/34063
from django.core.handlers.wsgi import LimitedStream
from django.test.client import AsyncClientHandler
orig_get_response_async = AsyncClientHandler.get_response_async
async def get_response_async(self, request):
request._stream = LimitedStream(
request._stream,
len(request._stream),
)
return await orig_get_response_async(self, request)
AsyncClientHandler.get_response_async = get_response_async
class IndexTests(test.TestCase):
async def test_post(self):
await self.async_client.post("/")
2. Subclassing
import django
from django import test
if django.VERSION < (4, 2):
# Backport of https://code.djangoproject.com/ticket/34063
from django.core.handlers.wsgi import LimitedStream
from django.test.client import AsyncClient, AsyncClientHandler
class FixedAsyncClientHandler(AsyncClientHandler):
async def get_response_async(self, request):
request._stream = LimitedStream(
request._stream,
len(request._stream),
)
return await super().get_response_async(request)
class FixedAsyncClient(AsyncClient):
def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)
self.handler.__class__ = FixedAsyncClientHandler
class TestCase(test.TestCase):
if django.VERSION < (4, 2):
async_client_class = FixedAsyncClient
class IndexTests(TestCase):
async def test_post(self):
await self.async_client.post("/")
from django import test
if django.VERSION < (4, 2):
# Backport of https://code.djangoproject.com/ticket/34063
from django.core.handlers.wsgi import LimitedStream
from django.test.client import AsyncClient, AsyncClientHandler
class FixedAsyncClientHandler(AsyncClientHandler):
async def get_response_async(self, request):
request._stream = LimitedStream(
request._stream,
len(request._stream),
)
return await super().get_response_async(request)
class FixedAsyncClient(AsyncClient):
def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)
self.handler.__class__ = FixedAsyncClientHandler
class TestCase(test.TestCase):
if django.VERSION < (4, 2):
async_client_class = FixedAsyncClient
class IndexTests(TestCase):
async def test_post(self):
await self.async_client.post("/")
3. patchy ( My package: https://pypi.org/project/patchy/ )
import django
import patchy
from django import test
if django.VERSION < (4, 2):
# Backport of https://code.djangoproject.com/ticket/34063
from django.test.client import AsyncClientHandler
patchy.patch(
AsyncClientHandler.__call__,
"""\
@@ -14,7 +14,8 @@
sender=self.__class__, scope=scope
)
request_started.connect(close_old_connections)
- request = ASGIRequest(scope, body_file)
+ from django.core.handlers.wsgi import LimitedStream
+ request = ASGIRequest(scope, LimitedStream(body_file, len(body_file)))
# Sneaky little hack so that we can easily get round
# CsrfViewMiddleware. This makes life easier, and is probably required
# for backwards compatibility with external tests against admin views.
"""
)
class IndexTests(test.TestCase):
async def test_post(self):
await self.async_client.post("/")
import patchy
from django import test
if django.VERSION < (4, 2):
# Backport of https://code.djangoproject.com/ticket/34063
from django.test.client import AsyncClientHandler
patchy.patch(
AsyncClientHandler.__call__,
"""\
@@ -14,7 +14,8 @@
sender=self.__class__, scope=scope
)
request_started.connect(close_old_connections)
- request = ASGIRequest(scope, body_file)
+ from django.core.handlers.wsgi import LimitedStream
+ request = ASGIRequest(scope, LimitedStream(body_file, len(body_file)))
# Sneaky little hack so that we can easily get round
# CsrfViewMiddleware. This makes life easier, and is probably required
# for backwards compatibility with external tests against admin views.
"""
)
class IndexTests(test.TestCase):
async def test_post(self):
await self.async_client.post("/")
--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/CAL13Cg9LYH2CW-TNi_DuJvCWZk4CZ9aS_aKVAPhqgNzndW%2B5pw%40mail.gmail.com.
James Bennett
Dec 31, 2022, 6:18:06 PM12/31/22
to django-d...@googlegroups.com
On Sat, Dec 31, 2022 at 2:12 AM 'Adam Johnson' via Django developers
(Contributions to Django itself) <django-d...@googlegroups.com>
wrote:
summary of the argument, I pointed out the benefits of the backport
policy, and also pointed out that making exceptions to it from time to
time has happened and without ill effect. I also explained why I
believe *not* backporting this runs counter to the goals the backport
policy is theoretically supposed to achieve, and why an exception to
backport this fix is correct.
As I said above:
> So I ask that people actually read and engage with that in future replies. This is the
> bare minimum of respect and good faith that can be expected of participants in
> discussions on this and other forums concerning Django and its development.
I should not have to be asking people to show that basic level of
respect and good faith. Yet here I am having to ask more than once.
> Also, I’ve learned several techniques to backport within projects when required (e.g. https://adamj.eu/tech/2020/07/29/backporting-a-django-orm-feature-with-database-instrumentation/ ). I encourage others to do the same. We could perhaps document some of these techniques (one source: Paul Ganssle’s talk ”What to Do When the Bug is in Someone Else’s Code ”: https://ganssle.io/talks/#upstream-bugs ).
>
> I also believe that there is no overriding reason to backport in this case. Self-backporting the fix within a project is very feasible—there’s no need for "the request.body workaround" (which is an understandable source of frustration).
This has multiple issues.
First, merely from a technical perspective, a "self backport" for this
bug can never be as self-contained as you seem to think it would be.
Take, for example, the code I was writing when I tripped over the bug:
a middleware that accesses request.POST, and is part of an application
I distribute to others. Even if I "self backport" a working
AsyncClient into my own test suite, anyone who has my middleware
configured in their settings now has the bug, too, and now *their*
test suite must account for it. And then anyone who uses that person's
code gets "infected" by it and now has to account for it.
This leads to either:
1. Everyone must maintain their own copy of the "self backport" of
things Django shipped, or
2. Everyone relies on a centralized repository for the code.
The first case clearly is bad: it takes not many individual developers
at all needing to maintain their own "self backports" before the total
maintenance cost imposed on the community outweighs the maintenance
cost Django would have incurred by just applying the fix. And given
that this is a high-severity bug in a key part of a feature Django
wants to hype up, I think the current status quo of "not many
affected" will not last long. And the cost analysis here, it should be
noted, does *not* include the decreased trust and confidence in Django
on the part of developers who see such a high-severity bug that Django
knows about and actively refuses to apply a fix for, even when *all*
currently-released versions of Django are affected by it.
The second case is worse: the correct name for a centralized
repository of Django with bugfixes applied is... Django. If we reach
the point where people in the community are expected to maintain forks
just to get documented basic functionality like the unit-testing tools
working, we are in a truly bad place indeed.
Finally, this ignores the question I have asked several times now: if
the official recommendation of the Django project is for everyone in
the Django ecosystem to apply workarounds or "self backports" for this
bug, how exactly is that to be communicated? It cannot go into the
documentation of the affected Django versions, because the same strict
"computer says no" reading of Django's policies that is being wielded
against the idea of a backported bugfix also applies to backporting
such notes into the documentation of older releases. Which leaves this
in a state of undiscoverability -- as I noted, I am someone who is not
exactly a novice at tracking down problems in Django, and it took me
over a day of working on this to end up at ticket 34063. And if an
exception *is* to be made to backport a "how to fix it yourself" into
the documentation of older versions, roughly the same amount of effort
would suffice simply to backport the bugfix itself. Even better:
backporting the fix does not create a stream of people reading the
Django documentation, discovering "wait, there was a bug of this
severity and their solution to it is to tell me to fix it myself", and
having that affect their perception of Django's commitment to
reliability and stability.
Which brings me back to the point I have been making over and over:
this bug is worth an exception to the backport policy, should be
backported to the full suite of currently-supported releases (4.1,
4.0, 3.2), and the backport policy itself should be revisited so that
severe bugs like this can't slip through a "well technically the
policy does not allow...".
(after which there should also be a post-mortem on how Django ended up
shipping AsyncClient in such a broken state in the first place, but
one thing at a time)
(Contributions to Django itself) <django-d...@googlegroups.com>
wrote:
> In the past I have also been frustrated at particular bug fixes not being backported. But I've gradually come to appreciate just how valuable the backport policy is. It keeps Django forward-facing, and prevents the fellows spending much of their precious time backporting.
In the forum post, which attempted to be a single-place-to-read
summary of the argument, I pointed out the benefits of the backport
policy, and also pointed out that making exceptions to it from time to
time has happened and without ill effect. I also explained why I
believe *not* backporting this runs counter to the goals the backport
policy is theoretically supposed to achieve, and why an exception to
backport this fix is correct.
As I said above:
> So I ask that people actually read and engage with that in future replies. This is the
> bare minimum of respect and good faith that can be expected of participants in
> discussions on this and other forums concerning Django and its development.
respect and good faith. Yet here I am having to ask more than once.
> Also, I’ve learned several techniques to backport within projects when required (e.g. https://adamj.eu/tech/2020/07/29/backporting-a-django-orm-feature-with-database-instrumentation/ ). I encourage others to do the same. We could perhaps document some of these techniques (one source: Paul Ganssle’s talk ”What to Do When the Bug is in Someone Else’s Code ”: https://ganssle.io/talks/#upstream-bugs ).
>
> I also believe that there is no overriding reason to backport in this case. Self-backporting the fix within a project is very feasible—there’s no need for "the request.body workaround" (which is an understandable source of frustration).
First, merely from a technical perspective, a "self backport" for this
bug can never be as self-contained as you seem to think it would be.
Take, for example, the code I was writing when I tripped over the bug:
a middleware that accesses request.POST, and is part of an application
I distribute to others. Even if I "self backport" a working
AsyncClient into my own test suite, anyone who has my middleware
configured in their settings now has the bug, too, and now *their*
test suite must account for it. And then anyone who uses that person's
code gets "infected" by it and now has to account for it.
This leads to either:
1. Everyone must maintain their own copy of the "self backport" of
things Django shipped, or
2. Everyone relies on a centralized repository for the code.
The first case clearly is bad: it takes not many individual developers
at all needing to maintain their own "self backports" before the total
maintenance cost imposed on the community outweighs the maintenance
cost Django would have incurred by just applying the fix. And given
that this is a high-severity bug in a key part of a feature Django
wants to hype up, I think the current status quo of "not many
affected" will not last long. And the cost analysis here, it should be
noted, does *not* include the decreased trust and confidence in Django
on the part of developers who see such a high-severity bug that Django
knows about and actively refuses to apply a fix for, even when *all*
currently-released versions of Django are affected by it.
The second case is worse: the correct name for a centralized
repository of Django with bugfixes applied is... Django. If we reach
the point where people in the community are expected to maintain forks
just to get documented basic functionality like the unit-testing tools
working, we are in a truly bad place indeed.
Finally, this ignores the question I have asked several times now: if
the official recommendation of the Django project is for everyone in
the Django ecosystem to apply workarounds or "self backports" for this
bug, how exactly is that to be communicated? It cannot go into the
documentation of the affected Django versions, because the same strict
"computer says no" reading of Django's policies that is being wielded
against the idea of a backported bugfix also applies to backporting
such notes into the documentation of older releases. Which leaves this
in a state of undiscoverability -- as I noted, I am someone who is not
exactly a novice at tracking down problems in Django, and it took me
over a day of working on this to end up at ticket 34063. And if an
exception *is* to be made to backport a "how to fix it yourself" into
the documentation of older versions, roughly the same amount of effort
would suffice simply to backport the bugfix itself. Even better:
backporting the fix does not create a stream of people reading the
Django documentation, discovering "wait, there was a bug of this
severity and their solution to it is to tell me to fix it myself", and
having that affect their perception of Django's commitment to
reliability and stability.
Which brings me back to the point I have been making over and over:
this bug is worth an exception to the backport policy, should be
backported to the full suite of currently-supported releases (4.1,
4.0, 3.2), and the backport policy itself should be revisited so that
severe bugs like this can't slip through a "well technically the
policy does not allow...".
(after which there should also be a post-mortem on how Django ended up
shipping AsyncClient in such a broken state in the first place, but
one thing at a time)
Matthew Pava
Jan 1, 2023, 1:15:11 AM1/1/23
to django-d...@googlegroups.com
Happy New Year!
I've been reading this thread with keen interest since James first brought it up. I think James's rationale is on point and that the back port to fix the bug needs to be done.
To the future of async,
Matthew
-----Original Message-----
From: django-d...@googlegroups.com <django-d...@googlegroups.com> On Behalf Of James Bennett
Sent: Saturday, December 31, 2022 5:18 PM
To: django-d...@googlegroups.com
Subject: Re: Backport for ticket 34063?
On Sat, Dec 31, 2022 at 2:12 AM 'Adam Johnson' via Django developers (Contributions to Django itself) <django-d...@googlegroups.com>
wrote:
> In the past I have also been frustrated at particular bug fixes not being backported. But I've gradually come to appreciate just how valuable the backport policy is. It keeps Django forward-facing, and prevents the fellows spending much of their precious time backporting.
In the forum post, which attempted to be a single-place-to-read summary of the argument, I pointed out the benefits of the backport policy, and also pointed out that making exceptions to it from time to time has happened and without ill effect. I also explained why I believe *not* backporting this runs counter to the goals the backport policy is theoretically supposed to achieve, and why an exception to backport this fix is correct.
As I said above:
> So I ask that people actually read and engage with that in future
> replies. This is the bare minimum of respect and good faith that can
> be expected of participants in discussions on this and other forums concerning Django and its development.
I should not have to be asking people to show that basic level of respect and good faith. Yet here I am having to ask more than once.
> Also, I’ve learned several techniques to backport within projects when required (e.g. https://us-east-2.protection.sophos.com?d=adamj.eu&u=aHR0cHM6Ly9hZGFtai5ldS90ZWNoLzIwMjAvMDcvMjkvYmFja3BvcnRpbmctYS1kamFuZ28tb3JtLWZlYXR1cmUtd2l0aC1kYXRhYmFzZS1pbnN0cnVtZW50YXRpb24v&i=NWVjN2YxNzUxNGEyNzMxNmMyMGRkZGU1&t=Y1hFNWxaWTQ0T09pWXE5b3RPL1dlQUw0SGZ5ZVZrUWtnUGppOE1IZExmUT0=&h=f422e964a4e74b35a8571b953844e0bd&s=AVNPUEhUT0NFTkNSWVBUSVbDWDgkUnpYUq-QXI1H6ilbanDf7stBFoYC6v6v8FXX_gGKbmjVHK647tPXPvUXfXKzFncDtPsW9aPoXXJ46maODTjGRXhVHIPlJFvNLQcISg ). I encourage others to do the same. We could perhaps document some of these techniques (one source: Paul Ganssle’s talk ”What to Do When the Bug is in Someone Else’s Code ”: https://us-east-2.protection.sophos.com?d=ganssle.io&u=aHR0cHM6Ly9nYW5zc2xlLmlvL3RhbGtzLyN1cHN0cmVhbS1idWdz&i=NWVjN2YxNzUxNGEyNzMxNmMyMGRkZGU1&t=ODRyektDQkRpbGtMYk4xemw5K0FxMi9xclVUdmVTR1ZFUWd2NHNKYllwST0=&h=f422e964a4e74b35a8571b953844e0bd&s=AVNPUEhUT0NFTkNSWVBUSVbDWDgkUnpYUq-QXI1H6ilbanDf7stBFoYC6v6v8FXX_gGKbmjVHK647tPXPvUXfXKzFncDtPsW9aPoXXJ46maODTjGRXhVHIPlJFvNLQcISg ).
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
I've been reading this thread with keen interest since James first brought it up. I think James's rationale is on point and that the back port to fix the bug needs to be done.
To the future of async,
Matthew
-----Original Message-----
From: django-d...@googlegroups.com <django-d...@googlegroups.com> On Behalf Of James Bennett
Sent: Saturday, December 31, 2022 5:18 PM
To: django-d...@googlegroups.com
Subject: Re: Backport for ticket 34063?
On Sat, Dec 31, 2022 at 2:12 AM 'Adam Johnson' via Django developers (Contributions to Django itself) <django-d...@googlegroups.com>
wrote:
> In the past I have also been frustrated at particular bug fixes not being backported. But I've gradually come to appreciate just how valuable the backport policy is. It keeps Django forward-facing, and prevents the fellows spending much of their precious time backporting.
In the forum post, which attempted to be a single-place-to-read summary of the argument, I pointed out the benefits of the backport policy, and also pointed out that making exceptions to it from time to time has happened and without ill effect. I also explained why I believe *not* backporting this runs counter to the goals the backport policy is theoretically supposed to achieve, and why an exception to backport this fix is correct.
As I said above:
> So I ask that people actually read and engage with that in future
> replies. This is the bare minimum of respect and good faith that can
> be expected of participants in discussions on this and other forums concerning Django and its development.
I should not have to be asking people to show that basic level of respect and good faith. Yet here I am having to ask more than once.
>
> I also believe that there is no overriding reason to backport in this case. Self-backporting the fix within a project is very feasible—there’s no need for "the request.body workaround" (which is an understandable source of frustration).
This has multiple issues.
First, merely from a technical perspective, a "self backport" for this bug can never be as self-contained as you seem to think it would be.
Take, for example, the code I was writing when I tripped over the bug:
a middleware that accesses request.POST, and is part of an application I distribute to others. Even if I "self backport" a working AsyncClient into my own test suite, anyone who has my middleware configured in their settings now has the bug, too, and now *their* test suite must account for it. And then anyone who uses that person's code gets "infected" by it and now has to account for it.
This leads to either:
1. Everyone must maintain their own copy of the "self backport" of things Django shipped, or 2. Everyone relies on a centralized repository for the code.
The first case clearly is bad: it takes not many individual developers at all needing to maintain their own "self backports" before the total maintenance cost imposed on the community outweighs the maintenance cost Django would have incurred by just applying the fix. And given that this is a high-severity bug in a key part of a feature Django wants to hype up, I think the current status quo of "not many affected" will not last long. And the cost analysis here, it should be noted, does *not* include the decreased trust and confidence in Django on the part of developers who see such a high-severity bug that Django knows about and actively refuses to apply a fix for, even when *all* currently-released versions of Django are affected by it.
The second case is worse: the correct name for a centralized repository of Django with bugfixes applied is... Django. If we reach the point where people in the community are expected to maintain forks just to get documented basic functionality like the unit-testing tools working, we are in a truly bad place indeed.
Finally, this ignores the question I have asked several times now: if the official recommendation of the Django project is for everyone in the Django ecosystem to apply workarounds or "self backports" for this bug, how exactly is that to be communicated? It cannot go into the documentation of the affected Django versions, because the same strict "computer says no" reading of Django's policies that is being wielded against the idea of a backported bugfix also applies to backporting such notes into the documentation of older releases. Which leaves this in a state of undiscoverability -- as I noted, I am someone who is not exactly a novice at tracking down problems in Django, and it took me over a day of working on this to end up at ticket 34063. And if an exception *is* to be made to backport a "how to fix it yourself" into the documentation of older versions, roughly the same amount of effort would suffice simply to backport the bugfix itself. Even better:
backporting the fix does not create a stream of people reading the Django documentation, discovering "wait, there was a bug of this severity and their solution to it is to tell me to fix it myself", and having that affect their perception of Django's commitment to reliability and stability.
Which brings me back to the point I have been making over and over:
this bug is worth an exception to the backport policy, should be backported to the full suite of currently-supported releases (4.1, 4.0, 3.2), and the backport policy itself should be revisited so that severe bugs like this can't slip through a "well technically the policy does not allow...".
(after which there should also be a post-mortem on how Django ended up shipping AsyncClient in such a broken state in the first place, but one thing at a time)
--
> I also believe that there is no overriding reason to backport in this case. Self-backporting the fix within a project is very feasible—there’s no need for "the request.body workaround" (which is an understandable source of frustration).
This has multiple issues.
First, merely from a technical perspective, a "self backport" for this bug can never be as self-contained as you seem to think it would be.
Take, for example, the code I was writing when I tripped over the bug:
a middleware that accesses request.POST, and is part of an application I distribute to others. Even if I "self backport" a working AsyncClient into my own test suite, anyone who has my middleware configured in their settings now has the bug, too, and now *their* test suite must account for it. And then anyone who uses that person's code gets "infected" by it and now has to account for it.
This leads to either:
1. Everyone must maintain their own copy of the "self backport" of things Django shipped, or 2. Everyone relies on a centralized repository for the code.
The first case clearly is bad: it takes not many individual developers at all needing to maintain their own "self backports" before the total maintenance cost imposed on the community outweighs the maintenance cost Django would have incurred by just applying the fix. And given that this is a high-severity bug in a key part of a feature Django wants to hype up, I think the current status quo of "not many affected" will not last long. And the cost analysis here, it should be noted, does *not* include the decreased trust and confidence in Django on the part of developers who see such a high-severity bug that Django knows about and actively refuses to apply a fix for, even when *all* currently-released versions of Django are affected by it.
The second case is worse: the correct name for a centralized repository of Django with bugfixes applied is... Django. If we reach the point where people in the community are expected to maintain forks just to get documented basic functionality like the unit-testing tools working, we are in a truly bad place indeed.
Finally, this ignores the question I have asked several times now: if the official recommendation of the Django project is for everyone in the Django ecosystem to apply workarounds or "self backports" for this bug, how exactly is that to be communicated? It cannot go into the documentation of the affected Django versions, because the same strict "computer says no" reading of Django's policies that is being wielded against the idea of a backported bugfix also applies to backporting such notes into the documentation of older releases. Which leaves this in a state of undiscoverability -- as I noted, I am someone who is not exactly a novice at tracking down problems in Django, and it took me over a day of working on this to end up at ticket 34063. And if an exception *is* to be made to backport a "how to fix it yourself" into the documentation of older versions, roughly the same amount of effort would suffice simply to backport the bugfix itself. Even better:
backporting the fix does not create a stream of people reading the Django documentation, discovering "wait, there was a bug of this severity and their solution to it is to tell me to fix it myself", and having that affect their perception of Django's commitment to reliability and stability.
Which brings me back to the point I have been making over and over:
this bug is worth an exception to the backport policy, should be backported to the full suite of currently-supported releases (4.1, 4.0, 3.2), and the backport policy itself should be revisited so that severe bugs like this can't slip through a "well technically the policy does not allow...".
(after which there should also be a post-mortem on how Django ended up shipping AsyncClient in such a broken state in the first place, but one thing at a time)
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To view this discussion on the web visit https://us-east-2.protection.sophos.com?d=google.com&u=aHR0cHM6Ly9ncm91cHMuZ29vZ2xlLmNvbS9kL21zZ2lkL2RqYW5nby1kZXZlbG9wZXJzL0NBTDEzQ2ctQmtIUjBMNlF0SndkV2ZwYjgwdkQwZS1iSE5LVm9fb0JzJTJCOXlMbXBzZUV3JTQwbWFpbC5nbWFpbC5jb20=&i=NWVjN2YxNzUxNGEyNzMxNmMyMGRkZGU1&t=clRaemMrRkphSFhhVWpvcEhvT0VQUEMrQXZ0SXVMZk1HY2dCRWZJTjNYUT0=&h=f422e964a4e74b35a8571b953844e0bd&s=AVNPUEhUT0NFTkNSWVBUSVbDWDgkUnpYUq-QXI1H6ilbanDf7stBFoYC6v6v8FXX_gGKbmjVHK647tPXPvUXfXKzFncDtPsW9aPoXXJ46maODTjGRXhVHIPlJFvNLQcISg.
Shai Berger
Jan 1, 2023, 10:21:48 AM1/1/23
to Matthew Pava, django-d...@googlegroups.com
Hi,
I think at this point it would help to move the discussion forward, if
we tried to step beyond the specific issue and phrase the revision in
the backporting policy. This will let us, I hope, have a more
principle-based discussion.
If I get it right -- please correct me, James -- it would be something
like this addition:
"In a new domain of functionality, which is considered major and
central, bugs which would have been release blockers if found in time
will be considered as candidates for backporting if found within the
next two LTS versions" -- or even "... if found before use of the new
domain of functionality becomes mainstream" -- or something similar.
I think looking at it from that angle will be more fruitful. I will say
that looking at this principle, thinking about the vicious cycle
mentioned by James, I tend towards accepting his arguments.
We may want to phrase it a different way: Think of such major domains
as "experimental". We did that in the Python3 transition -- we had
"experimental support" from 1.5, and IIRC that "experimental" label
wasn't dropped until 1.8. I doubt we can retroactively declare async
views as still experimental, but we can modify the backporting policy
to say "release-blocker-level bugs in experimental features will be
candidates for backporting as long as the feature is experimental";
and we can set an exception that says "async is still experimental for
backporting considerations", in view of the little use we've seen so
far.
(I can see the argument against the last proposition, that says
"experimental means potentially broken, so it should be less worthy of
backports rather than more"; I disagree, because (a) we do want to
encourage such experimentation, and (b) no, it doesn't really mean
potentially broken, it means the API is not yet covered by the
stability guarantees; we're at more liberty to change things when we
fix)
HTH,
Shai.
I think at this point it would help to move the discussion forward, if
we tried to step beyond the specific issue and phrase the revision in
the backporting policy. This will let us, I hope, have a more
principle-based discussion.
If I get it right -- please correct me, James -- it would be something
like this addition:
"In a new domain of functionality, which is considered major and
central, bugs which would have been release blockers if found in time
will be considered as candidates for backporting if found within the
next two LTS versions" -- or even "... if found before use of the new
domain of functionality becomes mainstream" -- or something similar.
I think looking at it from that angle will be more fruitful. I will say
that looking at this principle, thinking about the vicious cycle
mentioned by James, I tend towards accepting his arguments.
We may want to phrase it a different way: Think of such major domains
as "experimental". We did that in the Python3 transition -- we had
"experimental support" from 1.5, and IIRC that "experimental" label
wasn't dropped until 1.8. I doubt we can retroactively declare async
views as still experimental, but we can modify the backporting policy
to say "release-blocker-level bugs in experimental features will be
candidates for backporting as long as the feature is experimental";
and we can set an exception that says "async is still experimental for
backporting considerations", in view of the little use we've seen so
far.
(I can see the argument against the last proposition, that says
"experimental means potentially broken, so it should be less worthy of
backports rather than more"; I disagree, because (a) we do want to
encourage such experimentation, and (b) no, it doesn't really mean
potentially broken, it means the API is not yet covered by the
stability guarantees; we're at more liberty to change things when we
fix)
HTH,
Shai.
Tim Graham
Jan 1, 2023, 3:54:36 PM1/1/23
to Django developers (Contributions to Django itself)
Older Django releases are currently maintained with minimal support that allows existing projects to continue running securely. I don't think we should invest resources in promoting them as a place to use experimental features. A benefit of running an old LTS release like 3.2 is that any release at this point is probably a very important upgrade (security or data loss issue). If we start making new LTS bug fix releases to fix experimental features, releases may be more frequent and less important. Of course, with more changes, the risk of unrelated regressions increases, including the possible bad situation in which users put off a bug fix upgrade because it doesn't have any security fixes only to face issues updating to a subsequent security release because of an issue in an interim bug fix release. (Maybe mergers can estimate how many other async fixes would be eligible for a backport based on the proposed criteria.)
James argues that this bug that went unreported for 2+ years is now very important and is going to cause massive pain for many users. Only time will answer this, but I'm skeptical. So far I believe we've seen two people affected, James and the ticket reporter.
This bug will be a non-issue for third-party apps with the release of Django 5.0 in December if they follow the suggested Django version support policy:
"Following the release of Django 5.0, we suggest that third-party app authors drop support for all versions of Django prior to 4.2. At that time, you should be able to run your package’s tests using python -Wd so that deprecation warnings appear. After making the deprecation warning fixes, your app should be compatible with Django 5.0."
Until then, apps could run the affected tests only on Django 4.2 (which will have an alpha release in about two weeks) if they don't want to work around the bug.
James Bennett
Jan 1, 2023, 5:41:03 PM1/1/23
to django-d...@googlegroups.com
On Sun, Jan 1, 2023 at 12:54 PM Tim Graham <timog...@gmail.com> wrote:
> Older Django releases are currently maintained with minimal support that allows existing projects to continue running securely. I don't think we should invest resources in promoting them as a place to use experimental features. A benefit of running an old LTS release like 3.2 is that any release at this point is probably a very important upgrade (security or data loss issue). If we start making new LTS bug fix releases to fix experimental features, releases may be more frequent and less important. Of course, with more changes, the risk of unrelated regressions increases, including the possible bad situation in which users put off a bug fix upgrade because it doesn't have any security fixes only to face issues updating to a subsequent security release because of an issue in an interim bug fix release. (Maybe mergers can estimate how many other async fixes would be eligible for a backport based on the proposed criteria.)
The backport policy already says that Django will backport certain
> Older Django releases are currently maintained with minimal support that allows existing projects to continue running securely. I don't think we should invest resources in promoting them as a place to use experimental features. A benefit of running an old LTS release like 3.2 is that any release at this point is probably a very important upgrade (security or data loss issue). If we start making new LTS bug fix releases to fix experimental features, releases may be more frequent and less important. Of course, with more changes, the risk of unrelated regressions increases, including the possible bad situation in which users put off a bug fix upgrade because it doesn't have any security fixes only to face issues updating to a subsequent security release because of an issue in an interim bug fix release. (Maybe mergers can estimate how many other async fixes would be eligible for a backport based on the proposed criteria.)
things into older releases. I do not agree with your assessment of the
risk of backporting, but even if we grant it for sake of argument, the
Django project overall has already clearly endorsed -- through the
current backport policy -- the idea that some fixes are important
enough to override such concerns and to backport even into releases
that are on "minimal" support.
I happen to think that this is one such fix, for reasons that I have
laid out at length. I also think the backport policy should be amended
to prevent an issue like this from "slipping through the cracks" again
in the future. I am still considering the wording Shai proposed,
though on first read I thought it was getting at the right idea.
> James argues that this bug that went unreported for 2+ years is now very important and is going to cause massive pain for many users. Only time will answer this, but I'm skeptical. So far I believe we've seen two people affected, James and the ticket reporter.
I had been trying to avoid bringing this up because I felt that the
backport policy and the severity of the bug were the things to focus
on, and because I was trying to avoid singling out individual mistakes
that contributed to the situation we have now. But there is at least
one other report of this bug in the Django Trac instance. It is the
first one I found when trying to diagnose the problem in my own code
(and the one I mentioned in my initial question on IRC, as anyone who
saw that can confirm), and it is one that was filed only a couple of
months after the release of Django 3.1, so well within the window of
time when the backport policy would have said to apply the fix to the
3.1 branch as well as to main. It also correctly diagnosed the source
of the bug and provided an attempt at a patch.
That ticket was #32189:
https://code.djangoproject.com/ticket/32189
But it was closed invalid as a user error. We can debate whether the
user should have been trying to access request.POST for their
particular use case, but we cannot debate whether this is the
underlying bug that user was encountering. It also was clearly not the
normal behavior one ought to see from accessing request.POST, even in
a non-POST request or supposedly non-appropriate context, and likely
either should not have been closed, or should have produced a
follow-up ticket to investigate why that specific behavior was
occurring.
As for whether anyone else is affected: my searching turned up several
references on other sites to the exact error message the bug produces,
which all seemed to receive replies along the lines of #32189: that it
was the fault of the reporter for accessing request.POST rather than
request.body.
So. I have said several times that using a strict reading of the
backport policy to avoid fixing a severe bug sends a bad message. I
will amend that now, and say that using a strict reading of the
backport policy to avoid fixing a bug, when the bug was reported to
Django within a window where the backport policy should not have
mattered, sends an even worse message.
I also do think that more people will trip over this bug as they start
using async Django, and that leaving it unfixed creates a problem of
how to communicate the need for a workaround without violating a
strict reading of the backport policy.
> This bug will be a non-issue for third-party apps with the release of Django 5.0 in December if they follow the suggested Django version support policy:
the choice of long-delayed async adoption, or fracturing of the
community, onto maintainers of third-party packages and libraries. My
objection was to waiting until Django 3.2 drops out of support, which
is in April 2024, but the objection would apply equally to saying that
a feature released in 2020 must be left in a state where it is not
reliably usable until December 2023.
> Until then, apps could run the affected tests only on Django 4.2 (which will have an alpha release in about two weeks) if they don't want to work around the bug.
sends an extremely bad message about Django and its commitment to
reliability and stability. At the moment, there are zero
officially-supported releases of Django one can install that have a
working AsyncClient, despite AsyncClient being a documented and
allegedly supported API since August of 2020. Under my proposal,
developers would not have to wait for a future feature release just to
have the first actually-working AsyncClient, and developers of
third-party packages and libraries would not have to resort to
workaround, or delay async adoption, or fracture their usual support
matrices to deal with the bug.
So I return again to my contention that an exception ought to be made
to the backport policy, and that the fix should be backported to all
currently-supported Django release series: 4.1, 4.0, and 3.2 LTS.
Tim Graham
Jan 1, 2023, 6:30:57 PM1/1/23
to Django developers (Contributions to Django itself)
I guess it's nice to have a strict backport policy, partly to avoid hours of discussion like this, which ultimately boils down to a judgment/prediction about severity and whether or not the current backport policy should be amended. If I encountered a similar issue, I would have just worked around it and moved on. Surely there are other issues like this one that don't meet the current backport criteria but that could be similarly argued by someone persistent enough. It would be nice if the outcome here avoided that in the future.
I guess there's not much to add on either side of the argument at this point. Perhaps the best solution is what Shai outlined: to propose amending the backport criteria, either by expanding the LTS backport categories beyond security and data loss issues, or by allowing the steering council to vote on backport proposals on a case-by-case basis, at least if there's no consensus on the mailing list.
Tim Graham
Jan 1, 2023, 10:01:04 PM1/1/23
to Django developers (Contributions to Django itself)
Incidentally, I don't think it's important for the ultimate decision here, but I looked at the below analysis of ticket #32189. Carlton's analysis on the ticket that request.POST is empty when using 'content-type': 'application/json' remains true. The results of the tests provided in the description of #32189 remain unchanged by the fix for #34063 (test_request_factory_data_multipart passes and test_request_factory_data_json fails in the same way as in the ticket description). I would triage #32189 exactly as Carlton did. I don't see anything in that ticket that leads to the "AssertionError: Cannot read more than the available bytes from the HTTP incoming data." as in #34063. It's possible I made a mistake or that my knowledge is lacking, but I see nothing that would make me mark #32189 as a duplicate of #34063 (which I was planning to correct for posterity if it's indeed the case). It's possible that reporter did encounter the "Cannot read..." AssertionError at some point, but I see nothing concrete in the ticket that reproduces it. I hope that's clarifying!
James Bennett
Jan 2, 2023, 2:18:58 AM1/2/23
to django-d...@googlegroups.com
On Sun, Jan 1, 2023 at 7:01 PM Tim Graham <timog...@gmail.com> wrote:
> Incidentally, I don't think it's important for the ultimate decision here, but I looked at the below analysis of ticket #32189. Carlton's analysis on the ticket that request.POST is empty when using 'content-type': 'application/json' remains true.
The bug report specifically says:
> Incidentally, I don't think it's important for the ultimate decision here, but I looked at the below analysis of ticket #32189. Carlton's analysis on the ticket that request.POST is empty when using 'content-type': 'application/json' remains true.
> First due to the limitation in reading from FakePayload, in the real world you can ask to read more just you would not read more i think
> I found http request was setting a high chunk size for an unrelated test and failing to read from fake payload
>
> Then i notice that when trying to use a content type json not multipart (default) that the post data was empty again!
> This time it seemed to be due to handling of application / json content
I read that as clearly saying that the reporter's earlier attempt(s)
*did* encounter the same bug as #34063, since otherwise why mention
the read/chunk size and the mechanics of reading from FakePayload? It
seems like a later attempt ran into the separate issue of the
application/json content type not populating request.POST. Ideally
this bug report would have been followed up with a "wait, why did it
hit that code path with the read size error" during triage, since that
is *not* what ought to be happening, even when request.POST is
inappropriately accessed on a non-form request (request.POST should
just be an empty QueryDict in that case).
So I do believe #32189 is a duplicate of #34063. It just happened that
the reporter had hit multiple issues: one was user error, but the
other was a genuine bug in Django.
> If I encountered a similar issue, I would have just worked around it and moved on. Surely there are other issues like this one that don't meet the current backport criteria but that could be similarly argued by someone persistent enough. It would be nice if the outcome here avoided that in the future.
this severity in the async testing support, I do not think Django can
afford such a lackadaisical "work around it yourself" policy without
incurring significant reputational harm. Nor can the response be that
people should just upgrade -- mostly because Django should have fixed
this bug two years ago, but even today Django should take
responsibility for the bug and provide a backport of the fix into all
the supported releases. And especially should do so since it *still*
is not possible, today, to "upgrade" to a released version of Django
containing the fix.
> I guess it's nice to have a strict backport policy, partly to avoid hours of discussion like this
and the backport policy amended with language to explicitly allow
fixes like this one but not open the floodgates to anything and
everything, in the amount of time that has been devoted to trying to
prevent this from being backported.
At any rate, your remaining objections basically seem to boil down to
not liking the time and effort being spent on this. I have already
offered to do whatever work is needed to see the backports through to
release. It is not a particularly difficult patch, so far as I can
tell, and I have significant direct experience backporting and
maintaining much gnarlier bugfixes in branches of Django. So the
time/effort objection is moot.
Mariusz Felisiak
Jan 11, 2023, 3:15:07 AM1/11/23
to Django developers (Contributions to Django itself)
Hi,
I'm against the backport for the same reasons already mentioned by Carlton. So -1 from me.
Best,
Mariusz
Mariusz
Reply all
Reply to author
Forward
0 new messages