middleware: Avoid running APPEND_SLASH logic in Tornado.

Profiling suggests this saves about 600us in the runtime of every GET
/events request attempting to resolve URLs to determine whether we
need to do the APPEND_SLASH behavior.

It's possible that we end up doing the same URL resolution work later
and we're just moving around some runtime, but I think even if we do,
Django probably doesn't do any fancy caching that would mean doing
this query twice doesn't just do twice the work.

In any case, we probably want to extend this behavior to our whole API
because the APPEND_SLASH redirect behavior is essentially a bug there.
That is a more involved refactor, however.
This commit is contained in:
Tim Abbott 2020-02-14 11:29:05 -08:00
parent 10e7e15088
commit 229090a3a5
2 changed files with 22 additions and 1 deletions

View File

@ -11,6 +11,7 @@ from django.contrib.sessions.middleware import SessionMiddleware
from django.core.exceptions import DisallowedHost, SuspiciousOperation
from django.db import connection
from django.http import HttpRequest, HttpResponse, StreamingHttpResponse
from django.middleware.common import CommonMiddleware
from django.shortcuts import render
from django.utils.cache import patch_vary_headers
from django.utils.deprecation import MiddlewareMixin
@ -511,3 +512,23 @@ class FinalizeOpenGraphDescription(MiddlewareMixin):
assert not response.streaming
response.content = alter_content(request, response.content)
return response
class ZulipCommonMiddleware(CommonMiddleware):
"""
Patched version of CommonMiddleware to disable the APPEND_SLASH
redirect behavior inside Tornado.
While this has some correctness benefit in encouraging clients
to implement the API correctly, this also saves about 600us in
the runtime of every GET /events query, as the APPEND_SLASH
route resolution logic is surprisingly expensive.
TODO: We should probably extend this behavior to apply to all of
our API routes. The APPEND_SLASH behavior is really only useful
for non-API endpoints things like /login. But doing that
transition will require more careful testing.
"""
def should_redirect_with_slash(self, request: HttpRequest) -> bool:
if settings.RUNNING_INSIDE_TORNADO:
return False
return super().should_redirect_with_slash(request)

View File

@ -161,7 +161,7 @@ MIDDLEWARE = (
'zerver.middleware.JsonErrorHandler',
'zerver.middleware.RateLimitMiddleware',
'zerver.middleware.FlushDisplayRecipientCache',
'django.middleware.common.CommonMiddleware',
'zerver.middleware.ZulipCommonMiddleware',
'zerver.middleware.SessionHostDomainMiddleware',
'django.middleware.locale.LocaleMiddleware',
'django.middleware.csrf.CsrfViewMiddleware',