From 7ada2ee9fdcce20f902591792da16aaadf8c15cd Mon Sep 17 00:00:00 2001 From: Richard Marmorstein Date: Fri, 18 Jun 2021 13:36:04 -0400 Subject: [PATCH 01/11] WIP --- lib/ApiOperations/Request.php | 34 ++++ lib/ApiRequestor.php | 92 +++++++++-- lib/HttpClient/CurlClient.php | 181 ++++++++++++++++++++- tests/Stripe/ApiResourceTest.php | 43 +++++ tests/Stripe/HttpClient/CurlClientTest.php | 117 +++++++++++++ tests/TestServer.php | 126 ++++++++++++++ tests/bootstrap.php | 1 + 7 files changed, 580 insertions(+), 14 deletions(-) create mode 100644 tests/Stripe/ApiResourceTest.php create mode 100644 tests/TestServer.php diff --git a/lib/ApiOperations/Request.php b/lib/ApiOperations/Request.php index e300dc13f..b1fd65b3f 100644 --- a/lib/ApiOperations/Request.php +++ b/lib/ApiOperations/Request.php @@ -45,6 +45,23 @@ protected function _request($method, $url, $params = [], $options = null) return [$resp->json, $options]; } + /** + * @param string $method HTTP method ('get', 'post', etc.) + * @param string $url URL for the request + * @param callable $readBodyChunk function that will receive chunks of data from a successful request body + * @param array $params list of parameters for the request + * @param null|array|string $options + * + * @throws \Stripe\Exception\ApiErrorException if the request fails + * + * @return array tuple containing (the JSON response, $options) + */ + protected function _requestStreaming($method, $url, $readBodyChunk, $params = [], $options = null) + { + $opts = $this->_opts->merge($options); + static::_staticStreamingRequest($method, $url, $readBodyChunk, $params, $opts); + } + /** * @param string $method HTTP method ('get', 'post', etc.) * @param string $url URL for the request @@ -65,4 +82,21 @@ protected static function _staticRequest($method, $url, $params, $options) return [$response, $opts]; } + + /** + * @param string $method HTTP method ('get', 'post', etc.) + * @param string $url URL for the request + * @param callable $readBodyChunk function that will receive chunks of data from a successful request body + * @param array $params list of parameters for the request + * @param null|array|string $options + * + * @throws \Stripe\Exception\ApiErrorException if the request fails + */ + protected static function _staticStreamingRequest($method, $url, $readBodyChunk, $params, $options) + { + $opts = \Stripe\Util\RequestOptions::parse($options); + $baseUrl = isset($opts->apiBase) ? $opts->apiBase : static::baseUrl(); + $requestor = new \Stripe\ApiRequestor($opts->apiKey, $baseUrl); + $requestor->requestStreaming($method, $url, $readBodyChunk, $params, $opts->headers); + } } diff --git a/lib/ApiRequestor.php b/lib/ApiRequestor.php index 048c462c0..55a6a45b3 100644 --- a/lib/ApiRequestor.php +++ b/lib/ApiRequestor.php @@ -123,6 +123,28 @@ public function request($method, $url, $params = null, $headers = null) return [$resp, $myApiKey]; } + /** + * @param string $method + * @param string $url + * @param callable $readBodyChunk + * @param null|array $params + * @param null|array $headers + * + * @throws Exception\ApiErrorException + * + * @return array tuple containing (ApiReponse, API key) + */ + public function requestStreaming($method, $url, $readBodyChunk, $params = null, $headers = null) + { + $params = $params ?: []; + $headers = $headers ?: []; + list($rbody, $rcode, $rheaders, $myApiKey) = + $this->_requestRawStreaming($method, $url, $params, $headers, $readBodyChunk); + if ($rcode >= 300) { + $this->_interpretResponse($rbody, $rcode, $rheaders); + } + } + /** * @param string $rbody a JSON string * @param int $rcode @@ -328,18 +350,7 @@ private static function _defaultHeaders($apiKey, $clientInfo = null) ]; } - /** - * @param string $method - * @param string $url - * @param array $params - * @param array $headers - * - * @throws Exception\AuthenticationException - * @throws Exception\ApiConnectionException - * - * @return array - */ - private function _requestRaw($method, $url, $params, $headers) + private function _prepareRequest($method, $url, $params, $headers) { $myApiKey = $this->_apiKey; if (!$myApiKey) { @@ -416,6 +427,24 @@ function ($key) use ($params) { $rawHeaders[] = $header . ': ' . $value; } + return [$absUrl, $rawHeaders, $params, $hasFile, $myApiKey]; + } + + /** + * @param string $method + * @param string $url + * @param array $params + * @param array $headers + * + * @throws Exception\AuthenticationException + * @throws Exception\ApiConnectionException + * + * @return array + */ + private function _requestRaw($method, $url, $params, $headers) + { + list($absUrl, $rawHeaders, $params, $hasFile, $myApiKey) = $this->_prepareRequest($method, $url, $params, $headers); + $requestStartMs = Util\Util::currentTimeMillis(); list($rbody, $rcode, $rheaders) = $this->httpClient()->request( @@ -438,6 +467,45 @@ function ($key) use ($params) { return [$rbody, $rcode, $rheaders, $myApiKey]; } + /** + * @param string $method + * @param string $url + * @param array $params + * @param array $headers + * @param callable $readBodyChunk + * + * @throws Exception\AuthenticationException + * @throws Exception\ApiConnectionException + * + * @return array + */ + private function _requestRawStreaming($method, $url, $params, $headers, $readBodyChunk) + { + list($absUrl, $rawHeaders, $params, $hasFile, $myApiKey) = $this->_prepareRequest($method, $url, $params, $headers); + + $requestStartMs = Util\Util::currentTimeMillis(); + + list($rbody, $rcode, $rheaders) = $this->httpClient()->requestStreaming( + $method, + $absUrl, + $rawHeaders, + $params, + $hasFile, + $readBodyChunk + ); + + if (isset($rheaders['request-id']) + && \is_string($rheaders['request-id']) + && \strlen($rheaders['request-id']) > 0) { + self::$requestTelemetry = new RequestTelemetry( + $rheaders['request-id'], + Util\Util::currentTimeMillis() - $requestStartMs + ); + } + + return [$rbody, $rcode, $rheaders, $myApiKey]; + } + /** * @param resource $resource * diff --git a/lib/HttpClient/CurlClient.php b/lib/HttpClient/CurlClient.php index bae042897..e239a192b 100644 --- a/lib/HttpClient/CurlClient.php +++ b/lib/HttpClient/CurlClient.php @@ -193,7 +193,7 @@ public function getConnectTimeout() // END OF USER DEFINED TIMEOUTS - public function request($method, $absUrl, $headers, $params, $hasFile) + private function constructRequest($method, $absUrl, $headers, $params, $hasFile) { $method = \strtolower($method); @@ -275,16 +275,193 @@ public function request($method, $absUrl, $headers, $params, $hasFile) // potential issues (cf. https://github.com/stripe/stripe-php/issues/1045). $opts[\CURLOPT_IPRESOLVE] = \CURL_IPRESOLVE_V4; + return [$opts, $absUrl]; + } + + public function request($method, $absUrl, $headers, $params, $hasFile) + { + list($opts, $absUrl) = $this->constructRequest($method, $absUrl, $headers, $params, $hasFile); + list($rbody, $rcode, $rheaders) = $this->executeRequestWithRetries($opts, $absUrl); return [$rbody, $rcode, $rheaders]; } + public function requestStreaming($method, $absUrl, $headers, $params, $hasFile, $readBodyChunk) + { + list($opts, $absUrl) = $this->constructRequest($method, $absUrl, $headers, $params, $hasFile); + + $opts[\CURLOPT_RETURNTRANSFER] = false; + list($rbody, $rcode, $rheaders) = $this->executeStreamingRequestWithRetries($opts, $absUrl, $readBodyChunk); + + return [$rbody, $rcode, $rheaders]; + } + + /** + * Curl permits sending \CURLOPT_HEADERFUNCTION, which is called with lines + * from the header and \CURLOPT_WRITEFUNCTION, which is called with bytes + * from the body. You usually want to handle the body differently depending + * on what was in the header. + * + * This function makes it easier to specify different callbacks depending + * on the contents of the heeder. After the header has been completely read + * and the body begins to stream, it will call $determineWriteCallback with + * the array of headers. $determineWriteCallback should, based on the + * headers it receives, return a "writeCallback" that describes what to do + * with the incoming HTTP response body. + * + * @param array $opts + * @param callable $determineWriteCallback + * + * @return array + */ + private function useHeadersToDetermineWriteCallback($opts, $determineWriteCallback) + { + $rheaders = new Util\CaseInsensitiveArray(); + $headerCallback = function ($curl, $header_line) use (&$rheaders) { + // Ignore the HTTP request line (HTTP/1.1 200 OK) + if (false === \strpos($header_line, ':')) { + return \strlen($header_line); + } + list($key, $value) = \explode(':', \trim($header_line), 2); + $rheaders[\trim($key)] = \trim($value); + + return \strlen($header_line); + }; + + $writeCallback = null; + $writeCallbackWrapper = function ($curl, $data) use (&$writeCallback, &$rheaders, &$determineWriteCallback) { + if (null === $writeCallback) { + $writeCallback = \call_user_func_array($determineWriteCallback, [$rheaders]); + } + + return \call_user_func_array($writeCallback, [$curl, $data]); + }; + + return [$headerCallback, $writeCallbackWrapper]; + } + + /** + * Like `executeRequestWithRetries` except: + * 1. Does not buffer the body of a successful (status code < 300) + * response into memory -- instead, calls the caller-provided + * $readBodyChunk with each chunk of incoming data. + * 2. Does not retry if a network error occurs while streaming the + * body of a successful response. + * + * @param array $opts cURL options + * @param string $absUrl + * @param callable @readBodyChunk + * @param mixed $readBodyChunk + * + * @return array + */ + public function executeStreamingRequestWithRetries($opts, $absUrl, $readBodyChunk) + { + /** @var bool */ + $shouldRetry = false; + /** @var int */ + $numRetries = 0; + + // Did the last request return statusCode < 300? + /** @var null|bool */ + $succeeded = null; + + // Will contain the bytes of the body of the last request + // if it was not successful and should not be retries + /** @var null|string */ + $rbody = null; + + // Status code of the last request + /** @var null|bool */ + $rcode = null; + + // Array of headers from the last request + /** @var null|array */ + $lastRHeaders = null; + + $determineWriteCallback = function ($rheaders) use ( + &$readBodyChunk, + &$shouldRetry, + &$succeeded, + &$rbody, + &$numRetries, + &$rcode, + &$lastRHeaders + ) { + $lastRHeaders = $rheaders; + $errno = \curl_errno($this->curlHandle); + $rcode = \curl_getinfo($this->curlHandle, \CURLINFO_HTTP_CODE); + + // Send the bytes from the body of a successful request to the caller-provided $readBodyChunk. + if ($rcode < 300) { + $rbody = null; + $succeeded = true; + + return function ($curl, $data) use (&$readBodyChunk) { + // Don't expose the $curl handle to the user, and don't require them to + // return the length of $data. + \call_user_func_array($readBodyChunk, [$data]); + + return \strlen($data); + }; + } + $succeeded = false; + + $shouldRetry = $this->shouldRetry($errno, $rcode, $rheaders, $numRetries); + + // Discard the body from an unsuccessful request that should be retried. + if ($shouldRetry) { + return function ($curl, $data) { + return \strlen($data); + }; + } else { + // Otherwise, buffer the body into $rbody. It will need to be parsed to determine + // which exception to throw to the user. + $rbody = ''; + + return function ($curl, $data) use (&$rbody) { + $rbody .= $data; + + return \strlen($data); + }; + } + }; + + while (true) { + list($headerCallback, $writeCallback) = $this->useHeadersToDetermineWriteCallback($opts, $determineWriteCallback); + $opts[\CURLOPT_HEADERFUNCTION] = $headerCallback; + $opts[\CURLOPT_WRITEFUNCTION] = $writeCallback; + + $shouldRetry = false; + $rbody = null; + $succeeded = null; + $this->resetCurlHandle(); + \curl_setopt_array($this->curlHandle, $opts); + $result = \curl_exec($this->curlHandle); + if ($shouldRetry) { + ++$numRetries; + $sleepSeconds = $this->sleepTime($numRetries, $lastRHeaders); + \usleep((int) ($sleepSeconds * 1000000)); + } else { + break; + } + } + + $errno = \curl_errno($this->curlHandle); + if (0 !== $errno) { + $message = \curl_error($this->curlHandle); + $this->handleCurlError($absUrl, $errno, $message, $numRetries); + } + + return [$rbody, $rcode, $lastRHeaders]; + } + /** * @param array $opts cURL options * @param string $absUrl */ - private function executeRequestWithRetries($opts, $absUrl) + public function executeRequestWithRetries($opts, $absUrl) { $numRetries = 0; diff --git a/tests/Stripe/ApiResourceTest.php b/tests/Stripe/ApiResourceTest.php new file mode 100644 index 000000000..598ce5eb0 --- /dev/null +++ b/tests/Stripe/ApiResourceTest.php @@ -0,0 +1,43 @@ +instanceUrl(); + list($opts) = $this->_requestStreaming('get', $url, $readBodyChunk, $params, $opts); + + return $this; + } +} + +/** + * @internal + * @coversNothing + */ +final class ApiResourceTest extends \PHPUnit\Framework\TestCase +{ + use TestHelper; + + public function testRequestStreaming() + { + $foo = FooResource::retrieve('foo'); + $body = ''; + $readBodyChunk = function ($chunk) use (&$body) { + $body .= $chunk; + }; + + $foo->pdf($readBodyChunk); + + $parsed = \json_decode($body, true); + static::assertSame($parsed['object'], 'coupon'); + } +} diff --git a/tests/Stripe/HttpClient/CurlClientTest.php b/tests/Stripe/HttpClient/CurlClientTest.php index 4c74efd99..e78452231 100644 --- a/tests/Stripe/HttpClient/CurlClientTest.php +++ b/tests/Stripe/HttpClient/CurlClientTest.php @@ -9,6 +9,7 @@ final class CurlClientTest extends \PHPUnit\Framework\TestCase { use \Stripe\TestHelper; + use \Stripe\TestServer; /** @var \ReflectionProperty */ private $initialNetworkRetryDelayProperty; @@ -352,4 +353,120 @@ public function testSetRequestStatusCallback() \Stripe\ApiRequestor::setHttpClient(null); } } + + /** + * @after + */ + public function tearDownTestServer() + { + $this->stopTestServer(); + } + + // This is a pretty flaky/uncertain way to try and get an + // http server to deliver the body in separate "chunks". + // + // It seems to work but feel free to just skip or delete + // if this flakes. + public function testExecuteRequestWithRetriesCallsWriteFunctionWithChunks() + { + $chunk1 = 'First, bytes'; + $chunk2 = 'more bytes'; + $chunk3 = 'final bytes'; + $serverCode = <<startTestServer($serverCode); + $opts = []; + $opts[\CURLOPT_HTTPGET] = 1; + $opts[\CURLOPT_URL] = $absUrl; + $opts[\CURLOPT_HTTPHEADER] = ['Authorization: Basic c2tfdGVzdF94eXo6']; + $curl = new CurlClient(); + $receivedChunks = []; + $curl->executeStreamingRequestWithRetries($opts, $absUrl, function ($chunk) use (&$receivedChunks) { + $receivedChunks[] = $chunk; + }); + static::assertSame([$chunk1, $chunk2, $chunk3], $receivedChunks); + $this->stopTestServer(); + } + + public function testExecuteStreamingRequestWithRetriesRetries() + { + $serverCode = <<<'EOF' + +{} +EOF; + + $originalNRetries = \Stripe\Stripe::getMaxNetworkRetries(); + \Stripe\Stripe::setMaxNetworkRetries(3); + $absUrl = $this->startTestServer($serverCode); + $opts = []; + $opts[\CURLOPT_HTTPGET] = 1; + $opts[\CURLOPT_URL] = $absUrl; + $opts[\CURLOPT_HTTPHEADER] = ['Authorization: Basic c2tfdGVzdF94eXo6']; + $curl = new CurlClient(); + $receivedChunks = []; + $result = $curl->executeStreamingRequestWithRetries($opts, $absUrl, function ($chunk) use (&$receivedChunks) { + $receivedChunks[] = $chunk; + }); + $nRequests = $this->stopTestServer(); + + static::assertSame([], $receivedChunks); + \Stripe\Stripe::setMaxNetworkRetries($originalNRetries); + + static::assertSame(4, $nRequests); + } + + public function testExecuteStreamingRequestWithRetriesHandlesDisconnect() + { + $serverCode = <<<'EOF' +startTestServer($serverCode); + $opts = []; + $opts[\CURLOPT_HTTPGET] = 1; + $opts[\CURLOPT_URL] = $absUrl; + $opts[\CURLOPT_HTTPHEADER] = ['Authorization: Basic c2tfdGVzdF94eXo6']; + $curl = new CurlClient(); + $receivedChunks = []; + $exception = null; + + try { + $result = $curl->executeStreamingRequestWithRetries($opts, $absUrl, function ($chunk) use (&$receivedChunks) { + $receivedChunks[] = $chunk; + }); + } catch (\Exception $e) { + $exception = $e; + } + + $nRequests = $this->stopTestServer(); + static::assertSame('Stripe\Exception\ApiConnectionException', \get_class($exception)); + + static::assertSame(['12345'], $receivedChunks); + \Stripe\Stripe::setMaxNetworkRetries($originalNRetries); + + static::assertSame(1, $nRequests); + } } diff --git a/tests/TestServer.php b/tests/TestServer.php new file mode 100644 index 000000000..146a4e2ab --- /dev/null +++ b/tests/TestServer.php @@ -0,0 +1,126 @@ +serverProc) { + throw new \Exception('Error: test server was already running'); + } + + $dir = $this->makeTemporaryServerDirectory($code); + + $command = 'php -S localhost:' . $this->serverPort . ' -t ' . $dir; + $pipes = []; + $this->serverProc = \proc_open( + $command, + [2 => ['pipe', 'w']], + $pipes + ); + + $pid = \proc_get_status($this->serverProc)['pid']; + + // echo "Started test server on pid $pid\n"; + + $this->serverStderr = $pipes[2]; + + if (!\is_resource($this->serverProc)) { + throw new \Exception("Error starting test server on pid {$pid}, command failed: {$command}"); + } + + while ($r = \fgets($this->serverStderr)) { + if (\str_contains($r, 'started')) { + break; + } + if (\str_contains($r, 'Failed')) { + throw new \Exception("Error starting test server on pid {$pid}: " . $r . ' Was the port ' . $this->serverPort . ' already taken?'); + } + } + + return 'localhost:' . $this->serverPort; + } + + /** + * Stops the test server and returns the number of requests + * as indicated the logs in its stderr. + */ + public function stopTestServer() + { + $n = 0; + if (!$this->serverProc) { + return; + } + + \stream_set_blocking($this->serverStderr, false); + $lines = \explode(\PHP_EOL, \stream_get_contents($this->serverStderr)); + foreach ($lines as $line) { + if (\str_contains($line, 'Accepted')) { + ++$n; + } + } + while (true) { + $status = \proc_get_status($this->serverProc); + if (!$status['running']) { + break; + } + $pid = $status['pid']; + \exec("pkill -P {$pid}"); + \usleep(100000); + } + // echo "Terminated test server on pid $pid\n"; + \fclose($this->serverStderr); + \proc_close($this->serverProc); + $this->serverProc = null; + $this->serverStderr = null; + + return $n; + } +} diff --git a/tests/bootstrap.php b/tests/bootstrap.php index c2e449dde..5ab2c37d8 100644 --- a/tests/bootstrap.php +++ b/tests/bootstrap.php @@ -68,3 +68,4 @@ function checkStripeMockReachable() } require_once __DIR__ . '/TestHelper.php'; +require_once __DIR__ . '/TestServer.php'; From d1013e0815c99cf4e5f2949fc6cddc42f5661219 Mon Sep 17 00:00:00 2001 From: Richard Marmorstein Date: Mon, 21 Jun 2021 15:15:18 -0400 Subject: [PATCH 02/11] format --- tests/TestServer.php | 51 ++++++++++++++++++++++++++++++++++++++------ 1 file changed, 44 insertions(+), 7 deletions(-) diff --git a/tests/TestServer.php b/tests/TestServer.php index 146a4e2ab..ba200b428 100644 --- a/tests/TestServer.php +++ b/tests/TestServer.php @@ -76,18 +76,41 @@ public function startTestServer($code) throw new \Exception("Error starting test server on pid {$pid}, command failed: {$command}"); } - while ($r = \fgets($this->serverStderr)) { - if (\str_contains($r, 'started')) { + while (true) { + $conn = @\fsockopen('localhost', $this->serverPort); + if (\is_resource($conn)) { + \fclose($conn); + break; } - if (\str_contains($r, 'Failed')) { - throw new \Exception("Error starting test server on pid {$pid}: " . $r . ' Was the port ' . $this->serverPort . ' already taken?'); - } + \fclose($conn); } return 'localhost:' . $this->serverPort; } + /** + * Dirty way to parse the stderr of `php -S` to see how many + * requests were sent. Not robust -- and the format of the + * output changes from PHP version to PHP version, so beware. + * + * @param mixed $s + */ + private static function isPHPTestServerRequestLogLine($s) + { + if (\str_contains($s, 'Accepted')) { + return false; + } + if (\str_contains($s, 'Closing')) { + return false; + } + + // Lines start with a left square bracket, and contain + // a status code in brackets followed by a colon, like [200]: + // or [404]: + return \preg_match('/^\[.*\[[0-9]{3}\]:/', $s); + } + /** * Stops the test server and returns the number of requests * as indicated the logs in its stderr. @@ -102,19 +125,33 @@ public function stopTestServer() \stream_set_blocking($this->serverStderr, false); $lines = \explode(\PHP_EOL, \stream_get_contents($this->serverStderr)); foreach ($lines as $line) { - if (\str_contains($line, 'Accepted')) { + if (self::isPHPTestServerRequestLogLine($line)) { ++$n; + } else { + \flush(); + \ob_flush(); } } - while (true) { + + for ($i = 0; $i < 20; ++$i) { $status = \proc_get_status($this->serverProc); if (!$status['running']) { break; } $pid = $status['pid']; + // Kills any child processes -- the php test server + // appears to start up a child. \exec("pkill -P {$pid}"); + + // Kill the parent process. + \exec("kill {$pid}"); \usleep(100000); } + + if ($status['running']) { + throw new Exception('Could not kill test server'); + } + // echo "Terminated test server on pid $pid\n"; \fclose($this->serverStderr); \proc_close($this->serverProc); From e3ef53c4e487fecf24aa4b02ee0bd2021de79c18 Mon Sep 17 00:00:00 2001 From: Richard Marmorstein Date: Mon, 21 Jun 2021 15:18:15 -0400 Subject: [PATCH 03/11] Fix --- tests/TestServer.php | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/TestServer.php b/tests/TestServer.php index ba200b428..43e0b0be0 100644 --- a/tests/TestServer.php +++ b/tests/TestServer.php @@ -83,7 +83,6 @@ public function startTestServer($code) break; } - \fclose($conn); } return 'localhost:' . $this->serverPort; From b082f677f79c023fbbcb5e852c82562aff6c5c78 Mon Sep 17 00:00:00 2001 From: Richard Marmorstein Date: Mon, 21 Jun 2021 15:18:39 -0400 Subject: [PATCH 04/11] Not mixedgit add tests/ --- tests/TestServer.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/TestServer.php b/tests/TestServer.php index 43e0b0be0..be058b89f 100644 --- a/tests/TestServer.php +++ b/tests/TestServer.php @@ -93,7 +93,7 @@ public function startTestServer($code) * requests were sent. Not robust -- and the format of the * output changes from PHP version to PHP version, so beware. * - * @param mixed $s + * @param string $s */ private static function isPHPTestServerRequestLogLine($s) { From c38b15c4db3c6512714ed176203533d7516f9cc7 Mon Sep 17 00:00:00 2001 From: Richard Marmorstein Date: Mon, 21 Jun 2021 15:25:58 -0400 Subject: [PATCH 05/11] Fix phpstan --- tests/TestServer.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/TestServer.php b/tests/TestServer.php index be058b89f..69f07dc25 100644 --- a/tests/TestServer.php +++ b/tests/TestServer.php @@ -132,6 +132,7 @@ public function stopTestServer() } } + $status = []; for ($i = 0; $i < 20; ++$i) { $status = \proc_get_status($this->serverProc); if (!$status['running']) { @@ -148,7 +149,7 @@ public function stopTestServer() } if ($status['running']) { - throw new Exception('Could not kill test server'); + throw new \Exception('Could not kill test server'); } // echo "Terminated test server on pid $pid\n"; From 791d2a2e9b8223d2da323e46d65d554c985e0110 Mon Sep 17 00:00:00 2001 From: Richard Marmorstein Date: Mon, 21 Jun 2021 15:33:29 -0400 Subject: [PATCH 06/11] rename requestStreaming -> requestStream --- lib/ApiOperations/Request.php | 4 ++-- lib/ApiRequestor.php | 4 ++-- lib/HttpClient/CurlClient.php | 2 +- tests/Stripe/ApiResourceTest.php | 4 ++-- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/lib/ApiOperations/Request.php b/lib/ApiOperations/Request.php index b1fd65b3f..24c374156 100644 --- a/lib/ApiOperations/Request.php +++ b/lib/ApiOperations/Request.php @@ -56,7 +56,7 @@ protected function _request($method, $url, $params = [], $options = null) * * @return array tuple containing (the JSON response, $options) */ - protected function _requestStreaming($method, $url, $readBodyChunk, $params = [], $options = null) + protected function _requestStream($method, $url, $readBodyChunk, $params = [], $options = null) { $opts = $this->_opts->merge($options); static::_staticStreamingRequest($method, $url, $readBodyChunk, $params, $opts); @@ -97,6 +97,6 @@ protected static function _staticStreamingRequest($method, $url, $readBodyChunk, $opts = \Stripe\Util\RequestOptions::parse($options); $baseUrl = isset($opts->apiBase) ? $opts->apiBase : static::baseUrl(); $requestor = new \Stripe\ApiRequestor($opts->apiKey, $baseUrl); - $requestor->requestStreaming($method, $url, $readBodyChunk, $params, $opts->headers); + $requestor->requestStream($method, $url, $readBodyChunk, $params, $opts->headers); } } diff --git a/lib/ApiRequestor.php b/lib/ApiRequestor.php index 55a6a45b3..98e4dfece 100644 --- a/lib/ApiRequestor.php +++ b/lib/ApiRequestor.php @@ -134,7 +134,7 @@ public function request($method, $url, $params = null, $headers = null) * * @return array tuple containing (ApiReponse, API key) */ - public function requestStreaming($method, $url, $readBodyChunk, $params = null, $headers = null) + public function requestStream($method, $url, $readBodyChunk, $params = null, $headers = null) { $params = $params ?: []; $headers = $headers ?: []; @@ -485,7 +485,7 @@ private function _requestRawStreaming($method, $url, $params, $headers, $readBod $requestStartMs = Util\Util::currentTimeMillis(); - list($rbody, $rcode, $rheaders) = $this->httpClient()->requestStreaming( + list($rbody, $rcode, $rheaders) = $this->httpClient()->requestStream( $method, $absUrl, $rawHeaders, diff --git a/lib/HttpClient/CurlClient.php b/lib/HttpClient/CurlClient.php index e239a192b..629812bf0 100644 --- a/lib/HttpClient/CurlClient.php +++ b/lib/HttpClient/CurlClient.php @@ -287,7 +287,7 @@ public function request($method, $absUrl, $headers, $params, $hasFile) return [$rbody, $rcode, $rheaders]; } - public function requestStreaming($method, $absUrl, $headers, $params, $hasFile, $readBodyChunk) + public function requestStream($method, $absUrl, $headers, $params, $hasFile, $readBodyChunk) { list($opts, $absUrl) = $this->constructRequest($method, $absUrl, $headers, $params, $hasFile); diff --git a/tests/Stripe/ApiResourceTest.php b/tests/Stripe/ApiResourceTest.php index 598ce5eb0..d76c67d7c 100644 --- a/tests/Stripe/ApiResourceTest.php +++ b/tests/Stripe/ApiResourceTest.php @@ -3,7 +3,7 @@ namespace Stripe; // Fake resource that calls GET /v1/coupons/{coupon} but using -// _requestStreaming to test the _requestStreaming interface. +// _requestStream to test the _requestStream interface. class FooResource extends ApiResource { @@ -13,7 +13,7 @@ class FooResource extends ApiResource public function pdf($readBodyChunk, $params = null, $opts = null) { $url = $this->instanceUrl(); - list($opts) = $this->_requestStreaming('get', $url, $readBodyChunk, $params, $opts); + list($opts) = $this->_requestStream('get', $url, $readBodyChunk, $params, $opts); return $this; } From 74a13ad056b5568c5debfff7ad10619bfc8065f4 Mon Sep 17 00:00:00 2001 From: Richard Marmorstein Date: Mon, 21 Jun 2021 16:16:12 -0400 Subject: [PATCH 07/11] Old PHP lacks \str_contains --- tests/TestServer.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/TestServer.php b/tests/TestServer.php index 69f07dc25..ef79b50fa 100644 --- a/tests/TestServer.php +++ b/tests/TestServer.php @@ -97,10 +97,10 @@ public function startTestServer($code) */ private static function isPHPTestServerRequestLogLine($s) { - if (\str_contains($s, 'Accepted')) { + if (\strpos('Accepted', $s) !== false) { return false; } - if (\str_contains($s, 'Closing')) { + if (\strpos('Closing', $s) !== false) { return false; } From b5e165a573a8c6e69d88e8874dcc751d907ad392 Mon Sep 17 00:00:00 2001 From: Richard Marmorstein Date: Mon, 21 Jun 2021 16:21:34 -0400 Subject: [PATCH 08/11] Format --- tests/TestServer.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/TestServer.php b/tests/TestServer.php index ef79b50fa..87f0e3ee8 100644 --- a/tests/TestServer.php +++ b/tests/TestServer.php @@ -97,10 +97,10 @@ public function startTestServer($code) */ private static function isPHPTestServerRequestLogLine($s) { - if (\strpos('Accepted', $s) !== false) { + if (false !== \strpos('Accepted', $s)) { return false; } - if (\strpos('Closing', $s) !== false) { + if (false !== \strpos('Closing', $s)) { return false; } From 03db6cb26ef8b96d04105200bd9fc775abd262d0 Mon Sep 17 00:00:00 2001 From: Richard Marmorstein Date: Mon, 21 Jun 2021 16:23:23 -0400 Subject: [PATCH 09/11] Fix --- tests/TestServer.php | 7 ------- 1 file changed, 7 deletions(-) diff --git a/tests/TestServer.php b/tests/TestServer.php index 87f0e3ee8..7a2fa80b1 100644 --- a/tests/TestServer.php +++ b/tests/TestServer.php @@ -97,13 +97,6 @@ public function startTestServer($code) */ private static function isPHPTestServerRequestLogLine($s) { - if (false !== \strpos('Accepted', $s)) { - return false; - } - if (false !== \strpos('Closing', $s)) { - return false; - } - // Lines start with a left square bracket, and contain // a status code in brackets followed by a colon, like [200]: // or [404]: From cf1993b879e668e21f77d71b44054e208d912331 Mon Sep 17 00:00:00 2001 From: Richard Marmorstein Date: Mon, 21 Jun 2021 23:05:14 -0400 Subject: [PATCH 10/11] Address code review --- lib/HttpClient/CurlClient.php | 62 ++++++++++++---------- tests/Stripe/HttpClient/CurlClientTest.php | 53 +++++++++++++++--- tests/TestServer.php | 25 ++++++--- 3 files changed, 99 insertions(+), 41 deletions(-) diff --git a/lib/HttpClient/CurlClient.php b/lib/HttpClient/CurlClient.php index 629812bf0..2139999a9 100644 --- a/lib/HttpClient/CurlClient.php +++ b/lib/HttpClient/CurlClient.php @@ -319,14 +319,7 @@ private function useHeadersToDetermineWriteCallback($opts, $determineWriteCallba { $rheaders = new Util\CaseInsensitiveArray(); $headerCallback = function ($curl, $header_line) use (&$rheaders) { - // Ignore the HTTP request line (HTTP/1.1 200 OK) - if (false === \strpos($header_line, ':')) { - return \strlen($header_line); - } - list($key, $value) = \explode(':', \trim($header_line), 2); - $rheaders[\trim($key)] = \trim($value); - - return \strlen($header_line); + return self::parseLineIntoHeaderArray($header_line, $rheaders); }; $writeCallback = null; @@ -341,6 +334,17 @@ private function useHeadersToDetermineWriteCallback($opts, $determineWriteCallba return [$headerCallback, $writeCallbackWrapper]; } + private static function parseLineIntoHeaderArray($line, &$headers) + { + if (false === \strpos($line, ':')) { + return \strlen($line); + } + list($key, $value) = \explode(':', \trim($line), 2); + $headers[\trim($key)] = \trim($value); + + return \strlen($line); + } + /** * Like `executeRequestWithRetries` except: * 1. Does not buffer the body of a successful (status code < 300) @@ -363,10 +367,6 @@ public function executeStreamingRequestWithRetries($opts, $absUrl, $readBodyChun /** @var int */ $numRetries = 0; - // Did the last request return statusCode < 300? - /** @var null|bool */ - $succeeded = null; - // Will contain the bytes of the body of the last request // if it was not successful and should not be retries /** @var null|string */ @@ -380,23 +380,27 @@ public function executeStreamingRequestWithRetries($opts, $absUrl, $readBodyChun /** @var null|array */ $lastRHeaders = null; + $errno = null; + $message = null; + $determineWriteCallback = function ($rheaders) use ( &$readBodyChunk, &$shouldRetry, - &$succeeded, &$rbody, &$numRetries, &$rcode, - &$lastRHeaders + &$lastRHeaders, + &$errno, + &$message ) { $lastRHeaders = $rheaders; $errno = \curl_errno($this->curlHandle); + $rcode = \curl_getinfo($this->curlHandle, \CURLINFO_HTTP_CODE); // Send the bytes from the body of a successful request to the caller-provided $readBodyChunk. if ($rcode < 300) { $rbody = null; - $succeeded = true; return function ($curl, $data) use (&$readBodyChunk) { // 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 return \strlen($data); }; } - $succeeded = false; $shouldRetry = $this->shouldRetry($errno, $rcode, $rheaders, $numRetries); @@ -435,10 +438,24 @@ public function executeStreamingRequestWithRetries($opts, $absUrl, $readBodyChun $shouldRetry = false; $rbody = null; - $succeeded = null; $this->resetCurlHandle(); \curl_setopt_array($this->curlHandle, $opts); $result = \curl_exec($this->curlHandle); + $errno = \curl_errno($this->curlHandle); + if (0 !== $errno) { + $message = \curl_error($this->curlHandle); + } + if (!$this->getEnablePersistentConnections()) { + $this->closeCurlHandle(); + } + + if (\is_callable($this->getRequestStatusCallback())) { + \call_user_func_array( + $this->getRequestStatusCallback(), + [$rbody, $rcode, $lastRHeaders, $errno, $message, $shouldRetry, $numRetries] + ); + } + if ($shouldRetry) { ++$numRetries; $sleepSeconds = $this->sleepTime($numRetries, $lastRHeaders); @@ -448,9 +465,7 @@ public function executeStreamingRequestWithRetries($opts, $absUrl, $readBodyChun } } - $errno = \curl_errno($this->curlHandle); if (0 !== $errno) { - $message = \curl_error($this->curlHandle); $this->handleCurlError($absUrl, $errno, $message, $numRetries); } @@ -473,14 +488,7 @@ public function executeRequestWithRetries($opts, $absUrl) // Create a callback to capture HTTP headers for the response $rheaders = new Util\CaseInsensitiveArray(); $headerCallback = function ($curl, $header_line) use (&$rheaders) { - // Ignore the HTTP request line (HTTP/1.1 200 OK) - if (false === \strpos($header_line, ':')) { - return \strlen($header_line); - } - list($key, $value) = \explode(':', \trim($header_line), 2); - $rheaders[\trim($key)] = \trim($value); - - return \strlen($header_line); + return CurlClient::parseLineIntoHeaderArray($header_line, $rheaders); }; $opts[\CURLOPT_HEADERFUNCTION] = $headerCallback; diff --git a/tests/Stripe/HttpClient/CurlClientTest.php b/tests/Stripe/HttpClient/CurlClientTest.php index e78452231..ea0079439 100644 --- a/tests/Stripe/HttpClient/CurlClientTest.php +++ b/tests/Stripe/HttpClient/CurlClientTest.php @@ -17,6 +17,9 @@ final class CurlClientTest extends \PHPUnit\Framework\TestCase /** @var \ReflectionProperty */ private $maxNetworkRetryDelayProperty; + /** @var \ReflectionProperty */ + private $curlHandle; + /** @var float */ private $origInitialNetworkRetryDelay; @@ -62,6 +65,9 @@ public function setUpReflectors() $this->sleepTimeMethod = $curlClientReflector->getMethod('sleepTime'); $this->sleepTimeMethod->setAccessible(true); + + $this->curlHandle = $curlClientReflector->getProperty('curlHandle'); + $this->curlHandle->setAccessible(true); } /** @@ -84,6 +90,12 @@ private function setInitialNetworkRetryDelay($initialNetworkRetryDelay) $this->initialNetworkRetryDelayProperty->setValue(null, $initialNetworkRetryDelay); } + private function fastRetries() + { + $this->setInitialNetworkRetryDelay(0.001); + $this->setMaxNetworkRetryDelay(0.002); + } + private function createFakeRandomGenerator($returnValue = 1.0) { $fakeRandomGenerator = $this->createMock('\Stripe\Util\RandomGenerator'); @@ -410,24 +422,31 @@ public function testExecuteStreamingRequestWithRetriesRetries() {} EOF; - $originalNRetries = \Stripe\Stripe::getMaxNetworkRetries(); \Stripe\Stripe::setMaxNetworkRetries(3); + $this->fastRetries(); $absUrl = $this->startTestServer($serverCode); $opts = []; $opts[\CURLOPT_HTTPGET] = 1; $opts[\CURLOPT_URL] = $absUrl; $opts[\CURLOPT_HTTPHEADER] = ['Authorization: Basic c2tfdGVzdF94eXo6']; $curl = new CurlClient(); + $calls = []; $receivedChunks = []; + + $curl->setRequestStatusCallback(function ($rbody, $rcode, $rheaders, $errno, $message, $willBeRetried, $numRetries) use (&$calls) { + $calls[] = [$rcode, $numRetries]; + }); + $result = $curl->executeStreamingRequestWithRetries($opts, $absUrl, function ($chunk) use (&$receivedChunks) { $receivedChunks[] = $chunk; }); $nRequests = $this->stopTestServer(); static::assertSame([], $receivedChunks); - \Stripe\Stripe::setMaxNetworkRetries($originalNRetries); static::assertSame(4, $nRequests); + + static::assertSame([[500, 0], [500, 1], [500, 2], [500, 3]], $calls); } public function testExecuteStreamingRequestWithRetriesHandlesDisconnect() @@ -442,8 +461,7 @@ public function testExecuteStreamingRequestWithRetriesHandlesDisconnect() exit(); EOF; - $originalNRetries = \Stripe\Stripe::getMaxNetworkRetries(); - \Stripe\Stripe::setMaxNetworkRetries(3); + $this->fastRetries(); $absUrl = $this->startTestServer($serverCode); $opts = []; $opts[\CURLOPT_HTTPGET] = 1; @@ -462,11 +480,34 @@ public function testExecuteStreamingRequestWithRetriesHandlesDisconnect() } $nRequests = $this->stopTestServer(); + static::assertNotNull($exception); static::assertSame('Stripe\Exception\ApiConnectionException', \get_class($exception)); static::assertSame(['12345'], $receivedChunks); - \Stripe\Stripe::setMaxNetworkRetries($originalNRetries); - static::assertSame(1, $nRequests); } + + public function testExecuteStreamingRequestWithRetriesPersistentConnection() + { + $curl = new CurlClient(); + $coupon = \Stripe\Coupon::retrieve('coupon_xyz'); + + $absUrl = \Stripe\Stripe::$apiBase . "/v1/coupons/xyz"; + $opts[\CURLOPT_HTTPGET] = 1; + $opts[\CURLOPT_URL] = $absUrl; + $opts[\CURLOPT_HTTPHEADER] = ['Authorization: Basic c2tfdGVzdF94eXo6']; + $discardCallback = function ($chunk) {}; + $curl->executeStreamingRequestWithRetries($opts, $absUrl, $discardCallback); + $firstHandle = $this->curlHandle->getValue($curl); + + $curl->executeStreamingRequestWithRetries($opts, $absUrl, $discardCallback); + $secondHandle = $this->curlHandle->getValue($curl); + + $curl->setEnablePersistentConnections(false); + $curl->executeStreamingRequestWithRetries($opts, $absUrl, $discardCallback); + $thirdHandle = $this->curlHandle->getValue($curl); + + static::assertSame($firstHandle, $secondHandle); + static::assertNull($thirdHandle); + } } diff --git a/tests/TestServer.php b/tests/TestServer.php index 7a2fa80b1..a653b9ae1 100644 --- a/tests/TestServer.php +++ b/tests/TestServer.php @@ -24,6 +24,17 @@ trait TestServer // value to `stripe-mock`'s standard 12111. protected $serverPort = 12113; + + private function lint($path) + { + $output = ''; + $exitCode = null; + \exec("php -l {$path}", $output, $exitCode); + if (0 !== $exitCode) { + $text = \implode("\n", $output); + throw new \Exception("Error in test server code: {$text}"); + } + } /** * Makes a directory in a temporary path containing only an `index.php` file with * the specified content ($code). @@ -33,9 +44,9 @@ trait TestServer private function makeTemporaryServerDirectory($code) { $dir = \sys_get_temp_dir() . \DIRECTORY_SEPARATOR . 'stripe-php-test-server'; - $indexPhp = $dir . \DIRECTORY_SEPARATOR . 'index.php'; - if (\is_file($indexPhp)) { - \unlink($indexPhp); + $indexPHP = $dir . \DIRECTORY_SEPARATOR . 'index.php'; + if (\is_file($indexPHP)) { + \unlink($indexPHP); } if (\is_dir($dir)) { @@ -43,10 +54,11 @@ private function makeTemporaryServerDirectory($code) } \mkdir($dir); - $handle = \fopen($indexPhp, 'wb'); + $handle = \fopen($indexPHP, 'wb'); \fwrite($handle, $code); \fclose($handle); + $this->lint($indexPHP); return $dir; } @@ -68,8 +80,6 @@ public function startTestServer($code) $pid = \proc_get_status($this->serverProc)['pid']; - // echo "Started test server on pid $pid\n"; - $this->serverStderr = $pipes[2]; if (!\is_resource($this->serverProc)) { @@ -138,14 +148,13 @@ public function stopTestServer() // Kill the parent process. \exec("kill {$pid}"); - \usleep(100000); + \usleep(10000); } if ($status['running']) { throw new \Exception('Could not kill test server'); } - // echo "Terminated test server on pid $pid\n"; \fclose($this->serverStderr); \proc_close($this->serverProc); $this->serverProc = null; From 5cfd64543bf4f1b4ef4348ab2076f39cd3054d3a Mon Sep 17 00:00:00 2001 From: Richard Marmorstein Date: Mon, 21 Jun 2021 23:40:07 -0400 Subject: [PATCH 11/11] Fix test for php7.0 --- lib/HttpClient/CurlClient.php | 3 +- tests/Stripe/HttpClient/CurlClientTest.php | 40 +--------------------- tests/TestServer.php | 21 ++++++------ 3 files changed, 12 insertions(+), 52 deletions(-) diff --git a/lib/HttpClient/CurlClient.php b/lib/HttpClient/CurlClient.php index 2139999a9..213352200 100644 --- a/lib/HttpClient/CurlClient.php +++ b/lib/HttpClient/CurlClient.php @@ -390,8 +390,7 @@ public function executeStreamingRequestWithRetries($opts, $absUrl, $readBodyChun &$numRetries, &$rcode, &$lastRHeaders, - &$errno, - &$message + &$errno ) { $lastRHeaders = $rheaders; $errno = \curl_errno($this->curlHandle); diff --git a/tests/Stripe/HttpClient/CurlClientTest.php b/tests/Stripe/HttpClient/CurlClientTest.php index ea0079439..c097514af 100644 --- a/tests/Stripe/HttpClient/CurlClientTest.php +++ b/tests/Stripe/HttpClient/CurlClientTest.php @@ -374,44 +374,6 @@ public function tearDownTestServer() $this->stopTestServer(); } - // This is a pretty flaky/uncertain way to try and get an - // http server to deliver the body in separate "chunks". - // - // It seems to work but feel free to just skip or delete - // if this flakes. - public function testExecuteRequestWithRetriesCallsWriteFunctionWithChunks() - { - $chunk1 = 'First, bytes'; - $chunk2 = 'more bytes'; - $chunk3 = 'final bytes'; - $serverCode = <<startTestServer($serverCode); - $opts = []; - $opts[\CURLOPT_HTTPGET] = 1; - $opts[\CURLOPT_URL] = $absUrl; - $opts[\CURLOPT_HTTPHEADER] = ['Authorization: Basic c2tfdGVzdF94eXo6']; - $curl = new CurlClient(); - $receivedChunks = []; - $curl->executeStreamingRequestWithRetries($opts, $absUrl, function ($chunk) use (&$receivedChunks) { - $receivedChunks[] = $chunk; - }); - static::assertSame([$chunk1, $chunk2, $chunk3], $receivedChunks); - $this->stopTestServer(); - } - public function testExecuteStreamingRequestWithRetriesRetries() { $serverCode = <<<'EOF' @@ -492,7 +454,7 @@ public function testExecuteStreamingRequestWithRetriesPersistentConnection() $curl = new CurlClient(); $coupon = \Stripe\Coupon::retrieve('coupon_xyz'); - $absUrl = \Stripe\Stripe::$apiBase . "/v1/coupons/xyz"; + $absUrl = \Stripe\Stripe::$apiBase . '/v1/coupons/xyz'; $opts[\CURLOPT_HTTPGET] = 1; $opts[\CURLOPT_URL] = $absUrl; $opts[\CURLOPT_HTTPHEADER] = ['Authorization: Basic c2tfdGVzdF94eXo6']; diff --git a/tests/TestServer.php b/tests/TestServer.php index a653b9ae1..09e34eda6 100644 --- a/tests/TestServer.php +++ b/tests/TestServer.php @@ -24,17 +24,18 @@ trait TestServer // value to `stripe-mock`'s standard 12111. protected $serverPort = 12113; - private function lint($path) { - $output = ''; - $exitCode = null; - \exec("php -l {$path}", $output, $exitCode); - if (0 !== $exitCode) { - $text = \implode("\n", $output); - throw new \Exception("Error in test server code: {$text}"); - } + $output = ''; + $exitCode = null; + \exec("php -l {$path}", $output, $exitCode); + if (0 !== $exitCode) { + $text = \implode("\n", $output); + + throw new \Exception("Error in test server code: {$text}"); + } } + /** * Makes a directory in a temporary path containing only an `index.php` file with * the specified content ($code). @@ -59,6 +60,7 @@ private function makeTemporaryServerDirectory($code) \fclose($handle); $this->lint($indexPHP); + return $dir; } @@ -129,9 +131,6 @@ public function stopTestServer() foreach ($lines as $line) { if (self::isPHPTestServerRequestLogLine($line)) { ++$n; - } else { - \flush(); - \ob_flush(); } }