Re: [Django] #33700: APPEND_SLASH adds significant latency to all requests not ending in / (even if successful)

[Django]

#33700: APPEND_SLASH adds significant latency to all requests not ending in / 
(even
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
-+-
Changes (by Simon Charette):

 * cc: Simon Charette (added)

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/0107018fef0e37cb-9408ba2e-7dde-4573-8e59-f0f868f9d254-00%40eu-central-1.amazonses.com.

Re: [Django] #33700: APPEND_SLASH adds significant latency to all requests not ending in / (even if successful)

[Django]

#33700: APPEND_SLASH adds significant latency to all requests not ending in / 
(even
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 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.
-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/0107018feebae401-2200ffd4-3489-4303-b672-f5cbbd9ee398-00%40eu-central-1.amazonses.com.

Re: [Django] #33700: APPEND_SLASH adds significant latency to all requests not ending in / (even if successful)

[Django]

#33700: APPEND_SLASH adds significant latency to all requests not ending in / 
(even
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: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/0107018bf7d47d90-b1891496-cf39-4f37-ae70-d87c6f728fec-00%40eu-central-1.amazonses.com.

Re: [Django] #33700: APPEND_SLASH adds significant latency to all requests not ending in / (even if successful)

[Django]

#33700: APPEND_SLASH adds significant latency to all requests not ending in / 
(even
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
-+-
Changes (by Carlton Gibson ):

 * 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 
 }}}

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/01070181248e59a7-7a9de199-ac28-4a77-90fd-afc0179a5b6d-00%40eu-central-1.amazonses.com.

Re: [Django] #33700: APPEND_SLASH adds significant latency to all requests not ending in / (even if successful)

[Django]

#33700: APPEND_SLASH adds significant latency to all requests not ending in / 
(even
if successful)
-+-
 Reporter:  Anders Kaseorg   |Owner:  Anders
 Type:   |  Kaseorg
  Cleanup/optimization   |   Status:  assigned
Component:  HTTP handling|  Version:  4.0
 Severity:  Normal   |   Resolution:
 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
-+-
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: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/010701811968cd29-40c77d7c-bf97-421d-9c0b-cf0059ed392c-00%40eu-central-1.amazonses.com.

Re: [Django] #33700: APPEND_SLASH adds significant latency to all requests not ending in / (even if successful)

[Django]

#33700: APPEND_SLASH adds significant latency to all requests not ending in / 
(even
if successful)
-+-
 Reporter:  Anders Kaseorg   |Owner:  Anders
 Type:   |  Kaseorg
  Cleanup/optimization   |   Status:  assigned
Component:  HTTP handling|  Version:  4.0
 Severity:  Normal   |   Resolution:
 Keywords:  CommonMiddleware | Triage Stage:  Accepted
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-
Changes (by Carlton Gibson):

 * 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: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/01070180bc12d197-3e3b6b2b-b820-42e2-a837-c1b03c0fced4-00%40eu-central-1.amazonses.com.

Re: [Django] #33700: APPEND_SLASH adds significant latency to all requests not ending in / (even if successful)

[Django]

#33700: APPEND_SLASH adds significant latency to all requests not ending in / 
(even
if successful)
-+-
 Reporter:  Anders Kaseorg   |Owner:  Anders
 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: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/01070180ba6fc536-59b3a740-7dd0-4b8d-8b57-a904b0e8d209-00%40eu-central-1.amazonses.com.

Re: [Django] #33700: APPEND_SLASH adds significant latency to all requests not ending in / (even if successful)

[Django]

#33700: APPEND_SLASH adds significant latency to all requests not ending in / 
(even
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:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-
Changes (by Anders Kaseorg):

 * has_patch:  0 => 1


Comment:

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

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/01070180ba6bce5e-75e8752d-e67e-4480-a732-8acbdfbe59be-00%40eu-central-1.amazonses.com.

Re: [Django] #33700: APPEND_SLASH adds significant latency to all requests not ending in / (even if successful)

[Django]

#33700: APPEND_SLASH adds significant latency to all requests not ending in / 
(even
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: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/01070180b9f3bf5e-02f977d8-b908-4c72-89de-a93399e0b67d-00%40eu-central-1.amazonses.com.

Re: [Django] #33700: APPEND_SLASH adds significant latency to all requests not ending in / (even if successful)

[Django]

#33700: APPEND_SLASH adds significant latency to all requests not ending in / 
(even
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
-+-
Changes (by JK Laiho):

 * cc: JK Laiho (removed)


-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/01070180b7bb9f16-1b2d8053-ea7f-4841-b8dd-c6b264320585-00%40eu-central-1.amazonses.com.

Re: [Django] #33700: APPEND_SLASH adds significant latency to all requests not ending in / (even if successful)

[Django]

#33700: APPEND_SLASH adds significant latency to all requests not ending in / 
(even
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: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/01070180b7af4809-2b1069cf-ef23-415a-9908-ea365f678d8d-00%40eu-central-1.amazonses.com.

Re: [Django] #33700: APPEND_SLASH adds significant latency to all requests not ending in / (even if successful)

[Django]

#33700: APPEND_SLASH adds significant latency to all requests not ending in / 
(even
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
-+-
Changes (by Keryn Knight):

 * cc: Keryn Knight (added)


-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/01070180b76b0bce-0ce08af6-ccd0-4730-9c0d-a1179f004829-00%40eu-central-1.amazonses.com.