[Django] #33700: APPEND_SLASH adds significant latency to all requests not ending in / (even if successful)
Django
if successful)
-------------------------------------+-------------------------------------
Reporter: Anders | Owner: nobody
Kaseorg |
Type: | Status: new
Cleanup/optimization |
Component: HTTP | Version: 4.0
handling |
Severity: Normal | Keywords: CommonMiddleware
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
Originally, `APPEND_SLASH` worked by looking for 404 responses and
replacing them with redirects, so as not to unnecessarily impact the
performance of successful responses. However, commit
9390da7fb6e251eaa9a785692f987296cb14523f in 1.9.5/1.10 changed this to
check `should_redirect_with_slash()` on every request, resulting in a
moderately expensive extra `urlconf` lookup for every request not ending
with `/`, whether or not it succeeds as written.
This performance impact was not considered in the commit message or the
corresponding ticket #26293, so I assume it was an oversight. That ticket
asserted “This doesn't really make sense, since the two settings are not
interdependent”, which is incorrect—performance was the reason for the
interdependence.
The overhead was found to be significant enough in Zulip to merit
[https://github.com/zulip/zulip/commit/229090a3a58de15eb2a76a8e513fa5d9a190170c
subclassing] `CommonMiddleware` to skip it in certain conditions.
Here’s a [https://gist.github.com/andersk/0140318d32f1657ff40cad9e7e353bd2
minimal test project] with an exaggerated number of routes so the overhead
can be easily observed.
{{{
$ ./manage.py runserver
}}}
{{{
$ wrk http://127.0.0.1:8000/url9999
Running 10s test @ http://127.0.0.1:8000/url9999
2 threads and 10 connections
Thread Stats Avg Stdev Max +/- Stdev
Latency 232.40ms 73.85ms 570.86ms 69.16%
Req/Sec 21.70 9.47 40.00 63.35%
426 requests in 10.01s, 64.90KB read
Requests/sec: 42.56
Transfer/sec: 6.48KB
$ sed -i 's/# APPEND_SLASH = False/APPEND_SLASH = False/'
slash_test_settings.py
$ wrk http://127.0.0.1:8000/url9999
Running 10s test @ http://127.0.0.1:8000/url9999
2 threads and 10 connections
Thread Stats Avg Stdev Max +/- Stdev
Latency 139.80ms 52.07ms 352.19ms 69.09%
Req/Sec 36.46 12.23 60.00 58.12%
714 requests in 10.01s, 108.79KB read
Requests/sec: 71.32
Transfer/sec: 10.87KB
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33700>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
if successful)
-------------------------------------+-------------------------------------
Type: | Status: new
Cleanup/optimization |
Component: HTTP handling | Version: 4.0
Severity: Normal | Resolution:
Keywords: CommonMiddleware | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Keryn Knight (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/33700#comment:1>
Django
if successful)
-------------------------------------+-------------------------------------
Reporter: Anders Kaseorg | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: HTTP handling | Version: 4.0
Severity: Normal | Resolution:
Keywords: CommonMiddleware | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Carlton Gibson):
The Zulip middleware there...
{{{
def should_redirect_with_slash(self, request: HttpRequest) -> bool:
if settings.RUNNING_INSIDE_TORNADO:
return False
return super().should_redirect_with_slash(request)
}}}
... 🤔
Looks like that would be simpler just doing `APPEND_SLASH =
RUNNING_INSIDE_TORNADO` in the settings file, since
`should_redirect_with_slash()` would always immediately return `False` in
that case.
> ... extra urlconf lookup for every request not ending with /, whether or
not it succeeds as written.
If you're not normalising URLs, I wonder if you don't `APPEND_SLASH =
False` anyway?
> ...performance impact was not considered...
The assumption is that URLs without `/` are likely incorrect, so
''succeeding as written'' should be rare, i.e. not performance sensitive.
I'm inclined to say `wontfix` — you're welcome to disable APPEND_SLASH, or
implement a response only version of the middleware — or `needsinfo` at
least pending a suggested patch (that doesn't break anything).
--
Ticket URL: <https://code.djangoproject.com/ticket/33700#comment:2>
Django
if successful)
-------------------------------------+-------------------------------------
Reporter: Anders Kaseorg | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: HTTP handling | Version: 4.0
Severity: Normal | Resolution:
Keywords: CommonMiddleware | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: JK Laiho (removed)
--
Ticket URL: <https://code.djangoproject.com/ticket/33700#comment:3>
Django
if successful)
-------------------------------------+-------------------------------------
Reporter: Anders Kaseorg | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: HTTP handling | Version: 4.0
Severity: Normal | Resolution:
Keywords: CommonMiddleware | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Anders Kaseorg):
We have a mix of URLs with `/` (most user-facing HTML) and URLs without
`/` (REST API with compatibility considerations, access-controlled
uploaded files with given names, SAML `metadata.xml`, SCIM endpoints at
specified paths, static content in development). `RUNNING_INSIDE_TORNADO`
is not the right test; it was just an easy workaround for one part of the
problem. Of course we can fork `CommonMiddleware` in an arbitrary Zulip-
specific way, but we’d prefer to improve Django for everyone, and this
seems like a clear opportunity for that.
I don’t think it’s reasonable to assume that URLs without `/` are likely
incorrect. Many URLs are required to be without `/` for compatibility or
convention or other technical reasons. I’m typing this very comment at a
URL without `/`.
I’ve done some more investigation with the help of `git bisect`, and I
found that the logging problem #26293 that was targeted by
9390da7fb6e251eaa9a785692f987296cb14523f was subsequently addressed more
completely by 40b69607c751c4afa453edfd41d2ed155e58187e (#26504).
Therefore, we can simply revert 9390da7fb6e251eaa9a785692f987296cb14523f
to improve performance without regressing #26293. Everybody wins!
--
Ticket URL: <https://code.djangoproject.com/ticket/33700#comment:4>
Django
if successful)
-------------------------------------+-------------------------------------
Reporter: Anders Kaseorg | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: HTTP handling | Version: 4.0
Severity: Normal | Resolution:
Keywords: CommonMiddleware | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
Comment:
Submitted a patch at https://github.com/django/django/pull/15689.
--
Ticket URL: <https://code.djangoproject.com/ticket/33700#comment:5>
Django
if successful)
-------------------------------------+-------------------------------------
Type: | Kaseorg
Cleanup/optimization | Status: assigned
Component: HTTP handling | Version: 4.0
Severity: Normal | Resolution:
Keywords: CommonMiddleware | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Anders Kaseorg):
* owner: nobody => Anders Kaseorg
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/33700#comment:6>
Django
if successful)
-------------------------------------+-------------------------------------
Reporter: Anders Kaseorg | Owner: Anders
Type: | Kaseorg
Cleanup/optimization | Status: assigned
Component: HTTP handling | Version: 4.0
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Florian Apolloner (added)
* stage: Unreviewed => Accepted
Comment:
OK, thanks for the follow-up, and PR Anders. I'll Accept for review so we
can get some eyes on it.
(I didn't think it through 100% yet, or look at the reasons for the
previous changes, but I have mentally queried at times the reason for the
APPEND_SLASH check in process request, so I'm happy to have a further look
in any case.)
--
Ticket URL: <https://code.djangoproject.com/ticket/33700#comment:7>
Django
if successful)
-------------------------------------+-------------------------------------
Reporter: Anders Kaseorg | Owner: Anders
Type: | Kaseorg
Cleanup/optimization | Status: assigned
Component: HTTP handling | Version: 4.0
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Carlton Gibson):
* stage: Accepted => Ready for checkin
Comment:
This looks correct to me. #24720 (1.9) originally addressed the
performance issue of the check in `process_request`. This looks like it
regressed in #26293 (1.10) which handled the orthogonal logging issue. The
test changes in the PR here change to verify the whole middleware
behaviour, rather than just process_response. That looks OK.
I'll mark RFC, and leave for a period to allow others to check if they
wish, since CommonMiddleware is always sensitive.
--
Ticket URL: <https://code.djangoproject.com/ticket/33700#comment:8>
Django
if successful)
-------------------------------------+-------------------------------------
Reporter: Anders Kaseorg | Owner: Anders
Type: | Kaseorg
Component: HTTP handling | Version: 4.0
Keywords: CommonMiddleware | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"fbac2a4dd846b52c4f379eacb5bab654fe9540cc" fbac2a4d]:
{{{
#!CommitTicketReference repository=""
revision="fbac2a4dd846b52c4f379eacb5bab654fe9540cc"
Fixed #33700 -- Skipped extra resolution for successful requests not
ending with /.
By moving a should_redirect_with_slash call out of an if block, commit
9390da7fb6e251eaa9a785692f987296cb14523f negated the performance fix
of commit 434d309ef6dbecbfd2b322d3a1da78aa5cb05fa8 (#24720).
Meanwhile, the logging issue #26293 that it targeted was subsequently
fixed more fully by commit 40b69607c751c4afa453edfd41d2ed155e58187e
(#26504), so it is no longer needed. This effectively reverts it.
This speeds up successful requests not ending with / when APPEND_SLASH
is enabled (the default, and still useful in projects with a mix of
URLs with and without trailing /). The amount of speedup varies from
about 5% in a typical project to nearly 50% on a benchmark with many
routes.
Signed-off-by: Anders Kaseorg <and...@mit.edu>
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33700#comment:9>
Django
if successful)
-------------------------------------+-------------------------------------
Reporter: Anders Kaseorg | Owner: Anders
Type: | Kaseorg
Cleanup/optimization | Status: closed
Component: HTTP handling | Version: 4.0
Severity: Normal | Resolution: fixed
Keywords: CommonMiddleware | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Jeppe Fihl-Pearson):
A minor heads up that I have identified the changes made for this ticket
as the cause of a performance issue in how Django operates together with
uWSGI, related to the `APPEND_SLASH` redirects: #34989.
--
Ticket URL: <https://code.djangoproject.com/ticket/33700#comment:10>
Django
if successful)
-------------------------------------+-------------------------------------
Reporter: Anders Kaseorg | Owner: Anders
Type: | Kaseorg
Cleanup/optimization | Status: closed
Component: HTTP handling | Version: 4.0
Severity: Normal | Resolution: fixed
Keywords: CommonMiddleware | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Just wanted to flag a performance regression in the other direction now.
Because the `should_redirect_with_slash` redirect doesn't happen during
`process_request` anymore it means that if a `APPEND_SLASH` did need to
happen a 404 page would have to be rendered before the redirect occurs.
This can be problematic if your 404 view returns a `TemplateResponse`
which normally lazily renders after all `process_request` middleware
happen. A early redirect in the `process_request` avoided this.
--
Ticket URL: <https://code.djangoproject.com/ticket/33700#comment:11>
Django
if successful)
-------------------------------------+-------------------------------------
Reporter: Anders Kaseorg | Owner: Anders
Type: | Kaseorg
Cleanup/optimization | Status: closed
Component: HTTP handling | Version: 4.0
Severity: Normal | Resolution: fixed
Keywords: CommonMiddleware | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Simon Charette (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/33700#comment:12>