WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#24757 closed defect (bug) (fixed)

Diff timeout of 1 second is not sufficient.

Reported by: markjaquith Owned by: markjaquith
Milestone: 3.6 Priority: high
Severity: major Version: 3.6
Component: Revisions Keywords: has-patch commit
Focuses: Cc:

Description

[15-Jul-2013 05:22:54 UTC] PHP Fatal error:  Maximum execution time of 1 second exceeded in /srv/www/example.com/wordpress/wp-includes/wp-diff.php on line 496

Attachments (3)

24757.diff (584 bytes) - added by markjaquith 2 years ago.
24757.2.diff (567 bytes) - added by markjaquith 2 years ago.
24757.3.diff (542 bytes) - added by markjaquith 2 years ago.

Download all attachments as: .zip

Change History (11)

@markjaquith2 years ago

comment:1 @markjaquith2 years ago

Patch: The greater of 5 seconds or 2x the number of diffs.

comment:2 @nacin2 years ago

The default max_execution_time is 30, so 5 seems low.

We're really just trying to give the script as much time as it needs, as 30 may not be enough. One option is to give each revision a few seconds by calling set_time_limit() within the loop.

Maybe we should just do set_time_limit( 0 ) and also make sure (if we don't already) that we don't request an unreasonably high number of items at once via JS. If 30 isn't enough, then perhaps we're also requesting too many at once. Perhaps if the request times out, errors out, or takes more than X seconds, we need to cut in half the number of revisions we just tried to request.

comment:3 @markjaquith2 years ago

I implemented your "cut in half" suggestion on #24758.

@markjaquith2 years ago

comment:4 @nacin2 years ago

  • Keywords has-patch commit added

@set_time_limit( 0 ); is unlimited so it can go in the same place as it is now, rather than inside the loop. Otherwise, +1 commit.

comment:5 follow-up: @markjaquith2 years ago

Yeah, I saw some weird comment on php.net that suggested otherwise, but it didn't seem to make much sense, so I'm moving it back.

@markjaquith2 years ago

comment:6 @markjaquith2 years ago

  • Owner set to markjaquith
  • Resolution set to fixed
  • Status changed from new to closed

In 24707:

No PHP time limit when generating diffs.

Fixes #24757. Props nacin.

comment:7 in reply to: ↑ 5 @nacin2 years ago

Replying to markjaquith:

Yeah, I saw some weird comment on php.net that suggested otherwise, but it didn't seem to make much sense, so I'm moving it back.

The main concern that it takes so long that the HTTP request actually times out. In that case, #24758 is our fallback.

comment:8 @SergeyBiryukov2 years ago

  • Component changed from General to Revisions
Note: See TracTickets for help on using tickets.