Support maxlag and Retry-After header #28
Conversation
c960657
changed the title
| new Response( 503 ), | ||
| new Response( 200 ), | ||
| ) | ||
| ); |
There was a problem hiding this comment.
These test methods are quite long and I see this part repeated several times, perhaps some things can be factored out?
| return function( $numberOfRetries ) { | ||
| return function( $numberOfRetries, Response $response = null ) { | ||
| // The $response argument is only passed as of Guzzle 6.2.2. | ||
| if( $response ) { |
There was a problem hiding this comment.
Small detail: I think explicit checks are typically better: $response !== null
| $retryAfter = $response->getHeaderLine( 'Retry-After' ); | ||
| if( is_numeric( $retryAfter ) ) { | ||
| return 1000 * $retryAfter; | ||
| } elseif( $retryAfter ) { |
There was a problem hiding this comment.
You can almost always avoid if-else stuff. You can have two guard clauses here:
if (a) {
return x;
}
if (b) {
return y;
}| strstr( $data['error']['info'], 'anti-abuse measure' ) | ||
| ) | ||
| ) { | ||
| $shouldRetry = true; |
There was a problem hiding this comment.
This method is really long... though that was the case before already and you did not make it worse
| $this->assertEquals( [ 1000 ] , $delays ); | ||
| } | ||
|
|
||
| protected function getDelayMocker( &$delays ) |
There was a problem hiding this comment.
why not private? also: bracket is misplaced
| return function( $numberOfRetries, Response $response = null ) { | ||
| // The $response argument is only passed as of Guzzle 6.2.2. | ||
| if( $response !== null ) { | ||
| // Retry-After may be a number of seonds or an absolute date. |
There was a problem hiding this comment.
seonds -> seconds
It also might be nice to include a link to https://www.mediawiki.org/wiki/Manual:Maxlag_parameter here in a comment.
There was a problem hiding this comment.
I have included a link in newRetryDecider().
| } | ||
|
|
||
| if( $retryAfter ) { | ||
| $seconds = strtotime( $retryAfter ) - time(); |
There was a problem hiding this comment.
Is there some documentation that states that the header may contain a date / timestamp rather than a value in seconds?
I can't see it on the wiki page and I imagine including code like this will lead to some odd wait times if a date is returned due to timezones.
There was a problem hiding this comment.
The API doc doesn't mention in, but it is allowed according to RFC 7231, section 7.1.3, so I thought it was worth spending a few lines on.
The RFC requires the absolute date returned to be a full date with timezone in a format supported by strtotime(), so timezones are not a problem. But it may cause issues if the local clock is very far behind – but I don't expect that to be a common problem.
There was a problem hiding this comment.
I have added a reference to RFC 7231 in a comment.
|
Thanks for the PRs! :) |
* Start toward phpunit 9 WIP though... * There are no integration tests in datamodel
When using the
maxlagparameter, the API may an error codemaxlagand aRetry-AfterHTTP header (documented here: https://www.mediawiki.org/wiki/Manual:Maxlag_parameter).This patch adds
maxlagto the list of error codes that should cause a retry, and it makes the API use the delay reported by the API.It also mocks out the actual
sleep()call so tests now run faster.