#33700 closed Cleanup/optimization (fixed)
APPEND_SLASH adds significant latency to all requests not ending in / (even if successful)
Reported by: | Anders Kaseorg | Owned by: | Anders Kaseorg |
---|---|---|---|
Component: | HTTP handling | Version: | 4.0 |
Severity: | Normal | Keywords: | CommonMiddleware |
Cc: | Sam, Emett Speer, Keryn Knight, Florian Apolloner, Simon Charette | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
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 subclassing CommonMiddleware
to skip it in certain conditions.
Here’s a 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
Change History (12)
comment:1 by , 2 years ago
Cc: | added |
---|
comment:3 by , 2 years ago
Cc: | removed |
---|
comment:4 by , 2 years ago
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!
comment:5 by , 2 years ago
Has patch: | set |
---|
Submitted a patch at https://github.com/django/django/pull/15689.
comment:6 by , 2 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:7 by , 2 years ago
Cc: | added |
---|---|
Triage Stage: | Unreviewed → Accepted |
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.)
comment:8 by , 2 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
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.
comment:10 by , 12 months ago
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.
comment:11 by , 5 months ago
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.
comment:12 by , 5 months ago
Cc: | added |
---|
The Zulip middleware there...
... 🤔
Looks like that would be simpler just doing
APPEND_SLASH = not RUNNING_INSIDE_TORNADO
in the settings file, sinceshould_redirect_with_slash()
would always immediately returnFalse
in that case.If you're not normalising URLs, I wonder if you don't want
APPEND_SLASH = False
anyway?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 — orneedsinfo
at least pending a suggested patch (that doesn't break anything).