From a6bb34646a2064487d648ada410ebb8db720642c Mon Sep 17 00:00:00 2001 From: Michael Dowling Date: Sun, 3 Nov 2013 14:42:47 -0800 Subject: [PATCH] Adding an abstraction over curl context and providing the context in after_send and error events from the curl adapter. --- src/Guzzle/Http/Adapter/Curl/BatchContext.php | 93 +++++++++++++++ src/Guzzle/Http/Adapter/Curl/CurlAdapter.php | 108 ++++++++---------- .../Http/Adapter/Curl/BatchContextTest.php | 43 +++++++ .../Http/Adapter/Curl/CurlAdapterTest.php | 12 +- 4 files changed, 188 insertions(+), 68 deletions(-) create mode 100644 src/Guzzle/Http/Adapter/Curl/BatchContext.php create mode 100644 tests/Guzzle/Tests/Http/Adapter/Curl/BatchContextTest.php diff --git a/src/Guzzle/Http/Adapter/Curl/BatchContext.php b/src/Guzzle/Http/Adapter/Curl/BatchContext.php new file mode 100644 index 00000000..d3301e99 --- /dev/null +++ b/src/Guzzle/Http/Adapter/Curl/BatchContext.php @@ -0,0 +1,93 @@ +multi = $mutliHandle; + $this->handles = new \SplObjectStorage(); + } + + /** + * Get all of the transactions in the context + * + * @return array + */ + public function getTransactions() + { + return iterator_to_array($this->handles); + } + + /** + * Get the curl_multi handle + * + * @return resource + */ + public function getMultiHandle() + { + return $this->multi; + } + + /** + * Get a curl easy handle for a specific transaction + * + * @param TransactionInterface $transaction Transaction associated with the handle + * + * @return resource|null Returns the handle if found or null if not found + */ + public function getHandle(TransactionInterface $transaction) + { + return isset($this->handles[$transaction]) ? $this->handles[$transaction] : null; + } + + /** + * Add a transaction to the multi handle + * + * @param TransactionInterface $transaction Transaction to add + * @param resource $handle Resource to associated with the handle + * + * @throws \RuntimeException If the handle is already registered + */ + public function addTransaction(TransactionInterface $transaction, $handle) + { + if (isset($this->handles[$transaction])) { + throw new \RuntimeException('Transaction already registered'); + } + + CurlAdapter::checkCurlMultiResult(curl_multi_add_handle($this->multi, $handle)); + $this->handles[$transaction] = $handle; + } + + /** + * Remove a transaction and associated handle from the context + * + * @param TransactionInterface $transaction Transaction to remove + */ + public function removeTransaction(TransactionInterface $transaction) + { + if (isset($this->handles[$transaction])) { + CurlAdapter::checkCurlMultiResult(curl_multi_remove_handle($this->multi, $this->handles[$transaction])); + curl_close($this->handles[$transaction]); + unset($this->handles[$transaction]); + } + } +} diff --git a/src/Guzzle/Http/Adapter/Curl/CurlAdapter.php b/src/Guzzle/Http/Adapter/Curl/CurlAdapter.php index 0c222d8d..c7028ad5 100644 --- a/src/Guzzle/Http/Adapter/Curl/CurlAdapter.php +++ b/src/Guzzle/Http/Adapter/Curl/CurlAdapter.php @@ -52,6 +52,20 @@ class CurlAdapter implements AdapterInterface, BatchAdapterInterface } } + /** + * Throw an exception for a cURL multi response if needed + * + * @param int $code Curl response code + * @throws AdapterException + */ + public static function checkCurlMultiResult($code) + { + if ($code != CURLM_OK && $code != CURLM_CALL_MULTI_PERFORM) { + $buffer = function_exists('curl_multi_strerror') ? curl_multi_strerror($code) : self::ERROR_STR; + throw new AdapterException(sprintf('cURL error %s: %s', $code, $buffer)); + } + } + public function send(TransactionInterface $transaction) { $this->batch([$transaction]); @@ -61,49 +75,40 @@ class CurlAdapter implements AdapterInterface, BatchAdapterInterface public function batch(array $transactions) { - $context = [ - 'transactions' => $transactions, - 'handles' => new \SplObjectStorage(), - 'multi' => $this->checkoutMultiHandle() - ]; + $context = new BatchContext($this->checkoutMultiHandle()); foreach ($transactions as $transaction) { try { - $this->prepare($transaction, $context); + $context->addTransaction( + $transaction, + $this->factory->createHandle($transaction, $this->messageFactory) + ); } catch (RequestException $e) { - $stats = isset($context['handles'][$transaction]) && is_resource($context['handles'][$transaction]) - ? curl_getinfo($context['handles'][$transaction]) - : []; - $this->onError($transaction, $e, $context, $stats); + $this->onError($transaction, $e, $context, ['curl_context' => $context]); } } $this->perform($context); - $this->releaseMultiHandle($context['multi']); - } - - private function prepare(TransactionInterface $transaction, array $context) - { - $handle = $this->factory->createHandle($transaction, $this->messageFactory); - $this->checkCurlResult(curl_multi_add_handle($context['multi'], $handle)); - $context['handles'][$transaction] = $handle; + $this->releaseMultiHandle($context->getMultiHandle()); } /** * Execute and select curl handles * - * @param array $context TransactionInterface context + * @param BatchContext $context */ - private function perform(array $context) + private function perform(BatchContext $context) { // The first curl_multi_select often times out no matter what, but is usually required for fast transfers $selectTimeout = 0.001; $active = false; + $multi = $context->getMultiHandle(); + do { - while (($mrc = curl_multi_exec($context['multi'], $active)) == CURLM_CALL_MULTI_PERFORM); - $this->checkCurlResult($mrc); + while (($mrc = curl_multi_exec($multi, $active)) == CURLM_CALL_MULTI_PERFORM); + self::checkCurlMultiResult($mrc); $this->processMessages($context); - if ($active && curl_multi_select($context['multi'], $selectTimeout) === -1) { + if ($active && curl_multi_select($multi, $selectTimeout) === -1) { // Perform a usleep if a select returns -1: https://bugs.php.net/bug.php?id=61141 usleep(150); } @@ -113,19 +118,14 @@ class CurlAdapter implements AdapterInterface, BatchAdapterInterface /** * Check for errors and fix headers of a request based on a curl response - * - * @param TransactionInterface $transaction Transaction to process - * @param array $curl Curl data - * @param array $context Array of context information of the transfer - * * @throws RequestException on error */ - private function processResponse(TransactionInterface $transaction, array $curl, array $context) + private function processResponse(TransactionInterface $transaction, array $curl, BatchContext $context) { - $stats = curl_getinfo($context['handles'][$transaction]); - curl_multi_remove_handle($context['multi'], $context['handles'][$transaction]); - curl_close($context['handles'][$transaction]); - unset($context['handles'][$transaction]); + $handle = $context->getHandle($transaction); + $stats = curl_getinfo($handle); + $stats['curl_context'] = $context; + $context->removeTransaction($transaction); $request = $transaction->getRequest(); try { @@ -140,21 +140,6 @@ class CurlAdapter implements AdapterInterface, BatchAdapterInterface } } - /** - * Process any received curl multi messages - */ - private function processMessages(array $context) - { - while ($done = curl_multi_info_read($context['multi'])) { - foreach ($context['handles'] as $transaction) { - if ($context['handles'][$transaction] === $done['handle']) { - $this->processResponse($transaction, $done, $context); - continue 2; - } - } - } - } - /** * Check if a cURL transfer resulted in what should be an exception * @@ -181,16 +166,18 @@ class CurlAdapter implements AdapterInterface, BatchAdapterInterface } /** - * Throw an exception for a cURL multi response if needed - * - * @param int $code Curl response code - * @throws AdapterException + * Process any received curl multi messages */ - private function checkCurlResult($code) + private function processMessages(BatchContext $context) { - if ($code != CURLM_OK && $code != CURLM_CALL_MULTI_PERFORM) { - $buffer = function_exists('curl_multi_strerror') ? curl_multi_strerror($code) : self::ERROR_STR; - throw new AdapterException(sprintf('cURL error %s: %s', $code, $buffer)); + $multi = $context->getMultiHandle(); + while ($done = curl_multi_info_read($multi)) { + foreach ($context->getTransactions() as $transaction) { + if ($context->getHandle($transaction) === $done['handle']) { + $this->processResponse($transaction, $done, $context); + continue 2; + } + } } } @@ -234,18 +221,15 @@ class CurlAdapter implements AdapterInterface, BatchAdapterInterface /** * Handle an error */ - private function onError(TransactionInterface $transaction, \Exception $e, array $context, array $stats) + private function onError(TransactionInterface $transaction, \Exception $e, BatchContext $context, array $stats) { if (!$transaction->getRequest()->getEventDispatcher()->dispatch( RequestEvents::ERROR, new RequestErrorEvent($transaction, $e, $stats) )->isPropagationStopped()) { // Clean up multi handles and context - foreach ($context['handles'] as $transaction) { - curl_multi_remove_handle($context['multi'], $context['handles'][$transaction]); - curl_close($context['handles'][$transaction]); - } - $this->releaseMultiHandle($context['multi']); + $context->removeTransaction($transaction); + $this->releaseMultiHandle($context->getMultiHandle()); throw $e; } } diff --git a/tests/Guzzle/Tests/Http/Adapter/Curl/BatchContextTest.php b/tests/Guzzle/Tests/Http/Adapter/Curl/BatchContextTest.php new file mode 100644 index 00000000..6eef306b --- /dev/null +++ b/tests/Guzzle/Tests/Http/Adapter/Curl/BatchContextTest.php @@ -0,0 +1,43 @@ +addTransaction($t, $h); + try { + $b->addTransaction($t, $h); + $this->fail('Did not throw'); + } catch (\RuntimeException $e) { + curl_close($h); + curl_multi_close($m); + } + } + + public function testManagesHandles() + { + $m = curl_multi_init(); + $b = new BatchContext($m); + $h = curl_init(); + $t = new Transaction(new Client(), new Request('GET', '/')); + $b->addTransaction($t, $h); + $this->assertEquals([$t], $b->getTransactions()); + $b->removeTransaction($t); + $this->assertEquals([], $b->getTransactions()); + curl_multi_close($m); + } +} diff --git a/tests/Guzzle/Tests/Http/Adapter/Curl/CurlAdapterTest.php b/tests/Guzzle/Tests/Http/Adapter/Curl/CurlAdapterTest.php index ac341440..709f8d43 100644 --- a/tests/Guzzle/Tests/Http/Adapter/Curl/CurlAdapterTest.php +++ b/tests/Guzzle/Tests/Http/Adapter/Curl/CurlAdapterTest.php @@ -92,7 +92,7 @@ class CurlAdapterTest extends \PHPUnit_Framework_TestCase $this->assertInstanceOf('Guzzle\Http\Event\RequestErrorEvent', $ev); $this->assertSame($r, $ev->getRequest()); $this->assertInstanceOf('Guzzle\Http\Exception\RequestException', $ev->getException()); - $this->assertEquals([], $ev->getTransferInfo()); + $this->assertEquals(['curl_context'], array_keys($ev->getTransferInfo())); } public function testDispatchesAfterSendEvent() @@ -102,12 +102,15 @@ class CurlAdapterTest extends \PHPUnit_Framework_TestCase $r = new Request('GET', self::$server->getUrl()); $t = new Transaction(new Client(), $r); $a = new CurlAdapter(new MessageFactory()); - $r->getEventDispatcher()->addListener(RequestEvents::AFTER_SEND, function (RequestAfterSendEvent $e) { + $ev = null; + $r->getEventDispatcher()->addListener(RequestEvents::AFTER_SEND, function (RequestAfterSendEvent $e) use (&$ev) { + $ev = $e; $e->intercept(new Response(200, ['Foo' => 'bar'])); }); $response = $a->send($t); $this->assertEquals(200, $response->getStatusCode()); $this->assertEquals('bar', $response->getHeader('Foo')); + $this->assertArrayHasKey('curl_context', $ev->getTransferInfo()); } public function testDispatchesErrorEventAndRecovers() @@ -136,10 +139,7 @@ class CurlAdapterTest extends \PHPUnit_Framework_TestCase */ public function testChecksCurlMultiResult() { - $a = new CurlAdapter(new MessageFactory()); - $r = new \ReflectionMethod($a, 'checkCurlResult'); - $r->setAccessible(true); - $r->invoke($a, -2); + CurlAdapter::checkCurlMultiResult(-2); } public function testChecksForCurlException()