Page MenuHomePhabricator

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

Authored by epriestley on Jul 30 2018, 4:45 PM.

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
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

epriestley created this revision.Jul 30 2018, 4:45 PM
epriestley requested review of this revision.Jul 30 2018, 4:46 PM
epriestley edited the summary of this revision. (Show Details)Jul 30 2018, 4:47 PM
epriestley updated this revision to Diff 46736.Jul 30 2018, 4:48 PM
  • For clarity, "connection rate limits" -> "concurrent connection limits".
amckinley accepted this revision.Jul 30 2018, 5:53 PM
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
epriestley updated this revision to Diff 46737.Jul 30 2018, 5:54 PM
  • Remove extraneous "get".
This revision was automatically updated to reflect the committed changes.