Page MenuHomePhabricator

Remove the execution time limit (if any) before sinking HTTP responses
ClosedPublic

Authored by epriestley on Jul 30 2018, 4:45 PM.
Tags
None
Referenced Files
Unknown Object (File)
Feb 22 2024, 6:26 PM
Unknown Object (File)
Feb 13 2024, 9:19 PM
Unknown Object (File)
Jan 6 2024, 5:12 AM
Unknown Object (File)
Dec 25 2023, 8:30 AM
Unknown Object (File)
Nov 30 2023, 4:18 AM
Unknown Object (File)
Nov 17 2023, 3:34 AM
Unknown Object (File)
Nov 6 2023, 11:23 PM
Unknown Object (File)
Oct 30 2023, 2:49 PM
Subscribers
None

Details

Summary

Ref T12907. At least part of the problem is that we can hit PHP's max_execution_time limit on the file download pathway.

We don't currently set this to anything in the application itself, but PHP often sets it to 30s by default (and we have it set to 30s in production).

When writing responses, remove this limit. This limit is mostly a protection against accidental loops/recurison/etc., not a process slot protection. It doesn't really protect process slots anyway, since it doesn't start counting until the request starts executing, so you can (by default) send the request as slowly as you want without hitting this limit.

By releasing the limit this late, hopefully all the loops and recursion issues have already been caught and we're left with mostly smooth sailing.

We already remove this limit when sending git clone responses in DiffusionServeController and nothing has blown up. This affects git clone http:// and similar.

(I may have had this turned off locally and/or just be too impatient to wait 30s, which is why I haven't caught this previously.)

Test Plan
  • Poked around and downloaded some files.
  • Will curl ... in production and see if that goes better.

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

  • For clarity, "connection rate limits" -> "concurrent connection limits".
amckinley added inline comments.
src/aphront/response/AphrontResponse.php
62

/s/get//

This revision is now accepted and ready to land.Jul 30 2018, 5:53 PM
This revision was automatically updated to reflect the committed changes.