Skip to content

Commit cf1993b

Browse files
richardm-striperichardm-stripe
committed
Address code review
1 parent 03db6cb commit cf1993b

File tree

3 files changed

+99
-41
lines changed

3 files changed

+99
-41
lines changed

‎lib/HttpClient/CurlClient.php

+35-27
Original file line numberDiff line numberDiff line change
@@ -319,14 +319,7 @@ private function useHeadersToDetermineWriteCallback($opts, $determineWriteCallba
319319
{
320320
$rheaders = new Util\CaseInsensitiveArray();
321321
$headerCallback = function ($curl, $header_line) use (&$rheaders) {
322-
// Ignore the HTTP request line (HTTP/1.1 200 OK)
323-
if (false === \strpos($header_line, ':')) {
324-
return \strlen($header_line);
325-
}
326-
list($key, $value) = \explode(':', \trim($header_line), 2);
327-
$rheaders[\trim($key)] = \trim($value);
328-
329-
return \strlen($header_line);
322+
return self::parseLineIntoHeaderArray($header_line, $rheaders);
330323
};
331324

332325
$writeCallback = null;
@@ -341,6 +334,17 @@ private function useHeadersToDetermineWriteCallback($opts, $determineWriteCallba
341334
return [$headerCallback, $writeCallbackWrapper];
342335
}
343336

337+
private static function parseLineIntoHeaderArray($line, &$headers)
338+
{
339+
if (false === \strpos($line, ':')) {
340+
return \strlen($line);
341+
}
342+
list($key, $value) = \explode(':', \trim($line), 2);
343+
$headers[\trim($key)] = \trim($value);
344+
345+
return \strlen($line);
346+
}
347+
344348
/**
345349
* Like `executeRequestWithRetries` except:
346350
* 1. Does not buffer the body of a successful (status code < 300)
@@ -363,10 +367,6 @@ public function executeStreamingRequestWithRetries($opts, $absUrl, $readBodyChun
363367
/** @var int */
364368
$numRetries = 0;
365369

366-
// Did the last request return statusCode < 300?
367-
/** @var null|bool */
368-
$succeeded = null;
369-
370370
// Will contain the bytes of the body of the last request
371371
// if it was not successful and should not be retries
372372
/** @var null|string */
@@ -380,23 +380,27 @@ public function executeStreamingRequestWithRetries($opts, $absUrl, $readBodyChun
380380
/** @var null|array */
381381
$lastRHeaders = null;
382382

383+
$errno = null;
384+
$message = null;
385+
383386
$determineWriteCallback = function ($rheaders) use (
384387
&$readBodyChunk,
385388
&$shouldRetry,
386-
&$succeeded,
387389
&$rbody,
388390
&$numRetries,
389391
&$rcode,
390-
&$lastRHeaders
392+
&$lastRHeaders,
393+
&$errno,
394+
&$message
391395
) {
392396
$lastRHeaders = $rheaders;
393397
$errno = \curl_errno($this->curlHandle);
398+
394399
$rcode = \curl_getinfo($this->curlHandle, \CURLINFO_HTTP_CODE);
395400

396401
// Send the bytes from the body of a successful request to the caller-provided $readBodyChunk.
397402
if ($rcode < 300) {
398403
$rbody = null;
399-
$succeeded = true;
400404

401405
return function ($curl, $data) use (&$readBodyChunk) {
402406
// Don't expose the $curl handle to the user, and don't require them to
@@ -406,7 +410,6 @@ public function executeStreamingRequestWithRetries($opts, $absUrl, $readBodyChun
406410
return \strlen($data);
407411
};
408412
}
409-
$succeeded = false;
410413

411414
$shouldRetry = $this->shouldRetry($errno, $rcode, $rheaders, $numRetries);
412415

@@ -435,10 +438,24 @@ public function executeStreamingRequestWithRetries($opts, $absUrl, $readBodyChun
435438

436439
$shouldRetry = false;
437440
$rbody = null;
438-
$succeeded = null;
439441
$this->resetCurlHandle();
440442
\curl_setopt_array($this->curlHandle, $opts);
441443
$result = \curl_exec($this->curlHandle);
444+
$errno = \curl_errno($this->curlHandle);
445+
if (0 !== $errno) {
446+
$message = \curl_error($this->curlHandle);
447+
}
448+
if (!$this->getEnablePersistentConnections()) {
449+
$this->closeCurlHandle();
450+
}
451+
452+
if (\is_callable($this->getRequestStatusCallback())) {
453+
\call_user_func_array(
454+
$this->getRequestStatusCallback(),
455+
[$rbody, $rcode, $lastRHeaders, $errno, $message, $shouldRetry, $numRetries]
456+
);
457+
}
458+
442459
if ($shouldRetry) {
443460
++$numRetries;
444461
$sleepSeconds = $this->sleepTime($numRetries, $lastRHeaders);
@@ -448,9 +465,7 @@ public function executeStreamingRequestWithRetries($opts, $absUrl, $readBodyChun
448465
}
449466
}
450467

451-
$errno = \curl_errno($this->curlHandle);
452468
if (0 !== $errno) {
453-
$message = \curl_error($this->curlHandle);
454469
$this->handleCurlError($absUrl, $errno, $message, $numRetries);
455470
}
456471

@@ -473,14 +488,7 @@ public function executeRequestWithRetries($opts, $absUrl)
473488
// Create a callback to capture HTTP headers for the response
474489
$rheaders = new Util\CaseInsensitiveArray();
475490
$headerCallback = function ($curl, $header_line) use (&$rheaders) {
476-
// Ignore the HTTP request line (HTTP/1.1 200 OK)
477-
if (false === \strpos($header_line, ':')) {
478-
return \strlen($header_line);
479-
}
480-
list($key, $value) = \explode(':', \trim($header_line), 2);
481-
$rheaders[\trim($key)] = \trim($value);
482-
483-
return \strlen($header_line);
491+
return CurlClient::parseLineIntoHeaderArray($header_line, $rheaders);
484492
};
485493
$opts[\CURLOPT_HEADERFUNCTION] = $headerCallback;
486494

‎tests/Stripe/HttpClient/CurlClientTest.php

+47-6
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,9 @@ final class CurlClientTest extends \PHPUnit\Framework\TestCase
1717
/** @var \ReflectionProperty */
1818
private $maxNetworkRetryDelayProperty;
1919

20+
/** @var \ReflectionProperty */
21+
private $curlHandle;
22+
2023
/** @var float */
2124
private $origInitialNetworkRetryDelay;
2225

@@ -62,6 +65,9 @@ public function setUpReflectors()
6265

6366
$this->sleepTimeMethod = $curlClientReflector->getMethod('sleepTime');
6467
$this->sleepTimeMethod->setAccessible(true);
68+
69+
$this->curlHandle = $curlClientReflector->getProperty('curlHandle');
70+
$this->curlHandle->setAccessible(true);
6571
}
6672

6773
/**
@@ -84,6 +90,12 @@ private function setInitialNetworkRetryDelay($initialNetworkRetryDelay)
8490
$this->initialNetworkRetryDelayProperty->setValue(null, $initialNetworkRetryDelay);
8591
}
8692

93+
private function fastRetries()
94+
{
95+
$this->setInitialNetworkRetryDelay(0.001);
96+
$this->setMaxNetworkRetryDelay(0.002);
97+
}
98+
8799
private function createFakeRandomGenerator($returnValue = 1.0)
88100
{
89101
$fakeRandomGenerator = $this->createMock('\Stripe\Util\RandomGenerator');
@@ -410,24 +422,31 @@ public function testExecuteStreamingRequestWithRetriesRetries()
410422
{}
411423
EOF;
412424

413-
$originalNRetries = \Stripe\Stripe::getMaxNetworkRetries();
414425
\Stripe\Stripe::setMaxNetworkRetries(3);
426+
$this->fastRetries();
415427
$absUrl = $this->startTestServer($serverCode);
416428
$opts = [];
417429
$opts[\CURLOPT_HTTPGET] = 1;
418430
$opts[\CURLOPT_URL] = $absUrl;
419431
$opts[\CURLOPT_HTTPHEADER] = ['Authorization: Basic c2tfdGVzdF94eXo6'];
420432
$curl = new CurlClient();
433+
$calls = [];
421434
$receivedChunks = [];
435+
436+
$curl->setRequestStatusCallback(function ($rbody, $rcode, $rheaders, $errno, $message, $willBeRetried, $numRetries) use (&$calls) {
437+
$calls[] = [$rcode, $numRetries];
438+
});
439+
422440
$result = $curl->executeStreamingRequestWithRetries($opts, $absUrl, function ($chunk) use (&$receivedChunks) {
423441
$receivedChunks[] = $chunk;
424442
});
425443
$nRequests = $this->stopTestServer();
426444

427445
static::assertSame([], $receivedChunks);
428-
\Stripe\Stripe::setMaxNetworkRetries($originalNRetries);
429446

430447
static::assertSame(4, $nRequests);
448+
449+
static::assertSame([[500, 0], [500, 1], [500, 2], [500, 3]], $calls);
431450
}
432451

433452
public function testExecuteStreamingRequestWithRetriesHandlesDisconnect()
@@ -442,8 +461,7 @@ public function testExecuteStreamingRequestWithRetriesHandlesDisconnect()
442461
exit();
443462
EOF;
444463

445-
$originalNRetries = \Stripe\Stripe::getMaxNetworkRetries();
446-
\Stripe\Stripe::setMaxNetworkRetries(3);
464+
$this->fastRetries();
447465
$absUrl = $this->startTestServer($serverCode);
448466
$opts = [];
449467
$opts[\CURLOPT_HTTPGET] = 1;
@@ -462,11 +480,34 @@ public function testExecuteStreamingRequestWithRetriesHandlesDisconnect()
462480
}
463481

464482
$nRequests = $this->stopTestServer();
483+
static::assertNotNull($exception);
465484
static::assertSame('Stripe\Exception\ApiConnectionException', \get_class($exception));
466485

467486
static::assertSame(['12345'], $receivedChunks);
468-
\Stripe\Stripe::setMaxNetworkRetries($originalNRetries);
469-
470487
static::assertSame(1, $nRequests);
471488
}
489+
490+
public function testExecuteStreamingRequestWithRetriesPersistentConnection()
491+
{
492+
$curl = new CurlClient();
493+
$coupon = \Stripe\Coupon::retrieve('coupon_xyz');
494+
495+
$absUrl = \Stripe\Stripe::$apiBase . "/v1/coupons/xyz";
496+
$opts[\CURLOPT_HTTPGET] = 1;
497+
$opts[\CURLOPT_URL] = $absUrl;
498+
$opts[\CURLOPT_HTTPHEADER] = ['Authorization: Basic c2tfdGVzdF94eXo6'];
499+
$discardCallback = function ($chunk) {};
500+
$curl->executeStreamingRequestWithRetries($opts, $absUrl, $discardCallback);
501+
$firstHandle = $this->curlHandle->getValue($curl);
502+
503+
$curl->executeStreamingRequestWithRetries($opts, $absUrl, $discardCallback);
504+
$secondHandle = $this->curlHandle->getValue($curl);
505+
506+
$curl->setEnablePersistentConnections(false);
507+
$curl->executeStreamingRequestWithRetries($opts, $absUrl, $discardCallback);
508+
$thirdHandle = $this->curlHandle->getValue($curl);
509+
510+
static::assertSame($firstHandle, $secondHandle);
511+
static::assertNull($thirdHandle);
512+
}
472513
}

‎tests/TestServer.php

+17-8
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,17 @@ trait TestServer
2424
// value to `stripe-mock`'s standard 12111.
2525
protected $serverPort = 12113;
2626

27+
28+
private function lint($path)
29+
{
30+
$output = '';
31+
$exitCode = null;
32+
\exec("php -l {$path}", $output, $exitCode);
33+
if (0 !== $exitCode) {
34+
$text = \implode("\n", $output);
35+
throw new \Exception("Error in test server code: {$text}");
36+
}
37+
}
2738
/**
2839
* Makes a directory in a temporary path containing only an `index.php` file with
2940
* the specified content ($code).
@@ -33,20 +44,21 @@ trait TestServer
3344
private function makeTemporaryServerDirectory($code)
3445
{
3546
$dir = \sys_get_temp_dir() . \DIRECTORY_SEPARATOR . 'stripe-php-test-server';
36-
$indexPhp = $dir . \DIRECTORY_SEPARATOR . 'index.php';
37-
if (\is_file($indexPhp)) {
38-
\unlink($indexPhp);
47+
$indexPHP = $dir . \DIRECTORY_SEPARATOR . 'index.php';
48+
if (\is_file($indexPHP)) {
49+
\unlink($indexPHP);
3950
}
4051

4152
if (\is_dir($dir)) {
4253
\rmdir($dir);
4354
}
4455

4556
\mkdir($dir);
46-
$handle = \fopen($indexPhp, 'wb');
57+
$handle = \fopen($indexPHP, 'wb');
4758
\fwrite($handle, $code);
4859
\fclose($handle);
4960

61+
$this->lint($indexPHP);
5062
return $dir;
5163
}
5264

@@ -68,8 +80,6 @@ public function startTestServer($code)
6880

6981
$pid = \proc_get_status($this->serverProc)['pid'];
7082

71-
// echo "Started test server on pid $pid\n";
72-
7383
$this->serverStderr = $pipes[2];
7484

7585
if (!\is_resource($this->serverProc)) {
@@ -138,14 +148,13 @@ public function stopTestServer()
138148

139149
// Kill the parent process.
140150
\exec("kill {$pid}");
141-
\usleep(100000);
151+
\usleep(10000);
142152
}
143153

144154
if ($status['running']) {
145155
throw new \Exception('Could not kill test server');
146156
}
147157

148-
// echo "Terminated test server on pid $pid\n";
149158
\fclose($this->serverStderr);
150159
\proc_close($this->serverProc);
151160
$this->serverProc = null;

0 commit comments

Comments
 (0)