APPEND_SLASH adds significant latency to all requests not ending in / (even if successful)

[Anders Kaseorg]

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

[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 = not 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 want 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).

[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!

[Anders Kaseorg]

Submitted a patch at ​https://github.com/django/django/pull/15689.

[Carlton Gibson]

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.)

[Carlton Gibson]

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.

[Carlton Gibson <carlton@…>]

In fbac2a4d:

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 <andersk@…>

[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.

[Ryan Siemens]

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.