Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#33157 closed New feature (wontfix)

Consider back-porting the fix of #32889 to version 3.2.x

Reported by: Ryan Henning Owned by: nobody
Component: HTTP handling Version: 3.2
Severity: Normal Keywords:
Cc: Carlton Gibson Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Ryan Henning)

Prior to the fix for #32889 (and when deployed under an ASGI server, and for *synchronous* views), a single Django process would only serve one request at a time. Any blocking (e.g. for the ORM) during serving a single request would block _all other requests_ from being served. This caused us issues as we scaled up.

I've manually ported the fix for #32889 into our codebase and *WOOT* it works great!

We're running Django 3.2.7 and Python 3.7 (btw) and this back-port works well.

It seems that the fix is scheduled only for Django v4.x ... but I think this should be back-ported to v3.2.x so more people can benefit. It seems to work as-is, so the back-port *should* be easy.

Change History (6)

comment:1 by Ryan Henning, 3 years ago

Description: modified (diff)

comment:2 by Tim Graham, 3 years ago

Does the issue meet a backport criteria described in the supported versions policy? (I'm guessing not, since the committer probably would have recognized the severity of the issue and already done the backport.)

The description of #32889 says "With Django 4.0 being 3.8+ the required contextvars module is available." Django 3.2 supports Python 3.6 and 3.7 -- does the patch doesn't work in those cases?

comment:3 by Ryan Henning, 3 years ago

The comment from #32889 is almost correct ... contextvars was new in Python 3.7

The fix to #32889 is rather simple, it just makes use of asgiref's ThreadSensitiveContext which is in asgiref since version 3.3.2 ... and asgiref handles the lack of contextvars in Python 3.6 smartly (see here and here from the asgiref codebase). Btw, Django 3.2.7 already requires asgiref >= 3.3.2, therefore there are no dependency issues (see here).

Therefore the backport indeed will handle both Python 3.6 (no behavior change will be seen) and Python 3.7 (fix works!). Also it does not require any dependency changes.

As far as if it is part of the packport policy... I cannot say. The document you linked gives the following: "The rule of thumb is that fixes will be backported to the last feature release for bugs that would have prevented a release in the first place (release blockers)." I would attempt to argue that this should have been fixed at the first release of the asgi handler in Django... because it is a step back into the stone ages when your linux process can only handle one request at a time, thus it should be backported now under that reasoning.

PS: I've contributed to django/channels_redis (and was added as a maintainer of that repo) + I'd love the opportunity to contribute to core Django as well 🤓, so if you'd like I can prepare the PR for this -- just let me know your thoughts. 😊

comment:4 by Mariusz Felisiak, 3 years ago

Cc: Carlton Gibson added
Resolution: wontfix
Status: newclosed

Thanks for this report, however it doesn't qualify for a backport based on our supported versions policy. #32889 is not even consider a bug, but a cleanup. Moreover, ASGIHandler was added in Django 3.0 so it will not qualify for a backport, even if we consider it a bug, because the issue has been present since the feature was introduced.

comment:5 by Ryan Henning, 3 years ago

Okay, thank you for considering. I understand your opinion on not backporting. We have to draw the line somewhere -- we cannot backport everything!

I do still feel concerned for the many Django 3.x + ASGI users and who are likely unsure why their Django processes are under-performing (e.g. if looking at requests-per-second). This was our position until I (finally) found #32889 and backported it into our project. I'll go ahead and file another ticket about the poor performance of Django 3.x + ASGI so that it can be fixed independently of #32889.

in reply to:  5 comment:6 by Mariusz Felisiak, 3 years ago

Replying to Ryan Henning:

I'll go ahead and file another ticket about the poor performance of Django 3.x + ASGI so that it can be fixed independently of #32889.

As far as I'm aware this doesn't qualify as a "release blocker" (it's not a regression), so if that's not an issue in Django 4.0+ these tickets will be marked as invalid/wontfix.

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