Skip to content
Browse files

Removing some circular references

Better cleanup of CurlMulti handles on exceptions
curl.callback.progress emits a resource rather than CurlHandle
Removing curl_handle and curl_multi debug Request params
  • Loading branch information...
1 parent 673b401 commit 15abfe65e2f54e226810da7867338d2c6b572af1 @mtdowling mtdowling committed May 9, 2013
View
29 src/Guzzle/Http/Curl/CurlHandle.php
@@ -84,12 +84,6 @@ public static function factory(RequestInterface $request)
$request->removeHeader('Accept-Encoding');
}
- // Enable the progress function if the 'progress' param was set
- if ($requestCurlOptions->get('progress')) {
- $curlOptions[CURLOPT_PROGRESSFUNCTION] = array($mediator, 'progress');
- $curlOptions[CURLOPT_NOPROGRESS] = false;
- }
-
// Enable curl debug information if the 'debug' param was set
if ($requestCurlOptions->get('debug')) {
$curlOptions[CURLOPT_STDERR] = fopen('php://temp', 'r+');
@@ -211,18 +205,29 @@ public static function factory(RequestInterface $request)
$curlOptions[CURLOPT_HTTPHEADER][] = $line;
}
+ // Add the content-length header back if it was temporarily removed
+ if ($tempContentLength) {
+ $request->setHeader('Content-Length', $tempContentLength);
+ }
+
// Apply the options to a new cURL handle.
$handle = curl_init();
- curl_setopt_array($handle, $curlOptions);
- if ($tempContentLength) {
- $request->setHeader('Content-Length', $tempContentLength);
+ // Enable the progress function if the 'progress' param was set
+ if ($requestCurlOptions->get('progress')) {
+ // Wrap the function in a function that provides the curl handle to the mediator's progress function
+ // Using this rather than injecting the handle into the mediator prevents a circular reference
+ $curlOptions[CURLOPT_PROGRESSFUNCTION] = function () use ($mediator, $handle) {
+ $args = func_get_args();
+ $args[] = $handle;
+ call_user_func_array(array($mediator, 'progress'), $args);
+ };
+ $curlOptions[CURLOPT_NOPROGRESS] = false;
}
- $handle = new static($handle, $curlOptions);
- $mediator->setCurlHandle($handle);
+ curl_setopt_array($handle, $curlOptions);
- return $handle;
+ return new static($handle, $curlOptions);
}
/**
View
15 src/Guzzle/Http/Curl/CurlMultiProxy.php
@@ -102,9 +102,18 @@ public function send()
while ($request = array_shift($this->queued)) {
$group->add($request);
}
- $group->send();
- array_pop($this->groups);
- $this->cleanupHandles();
+ try {
+ $group->send();
+ array_pop($this->groups);
+ $this->cleanupHandles();
+ } catch (\Exception $e) {
+ // Remove the group and cleanup if an exception was encountered and no more requests in group
+ if (!$group->count()) {
+ array_pop($this->groups);
+ $this->cleanupHandles();
+ }
+ throw $e;
+ }
}
}
View
33 src/Guzzle/Http/Curl/RequestMediator.php
@@ -20,11 +20,6 @@ class RequestMediator
protected $emitIo;
/**
- * @var CurlHandle
- */
- protected $curlHandle;
-
- /**
* @param RequestInterface $request Request to mediate
* @param bool $emitIo Set to true to dispatch events on input and output
*/
@@ -35,21 +30,6 @@ public function __construct(RequestInterface $request, $emitIo = false)
}
/**
- * Set the associated CurlHandle object
- *
- * @param CurlHandle $handle Curl handle
- *
- * @return RequestMediator
- */
- public function setCurlHandle(CurlHandle $handle)
- {
- $this->curlHandle = $handle;
- $this->request->getParams()->set('curl_handle', $handle);
-
- return $this;
- }
-
- /**
* Receive a response header from curl
*
* @param resource $curl Curl handle
@@ -65,16 +45,17 @@ public function receiveResponseHeader($curl, $header)
/**
* Received a progress notification
*
- * @param int $downloadSize Total download size
- * @param int $downloaded Amount of bytes downloaded
- * @param int $uploadSize Total upload size
- * @param int $uploaded Amount of bytes uploaded
+ * @param int $downloadSize Total download size
+ * @param int $downloaded Amount of bytes downloaded
+ * @param int $uploadSize Total upload size
+ * @param int $uploaded Amount of bytes uploaded
+ * @param resource $handle CurlHandle object
*/
- public function progress($downloadSize, $downloaded, $uploadSize, $uploaded)
+ public function progress($downloadSize, $downloaded, $uploadSize, $uploaded, $handle = null)
{
$this->request->dispatch('curl.callback.progress', array(
'request' => $this->request,
- 'handle' => $this->curlHandle,
+ 'handle' => $handle,
'download_size' => $downloadSize,
'downloaded' => $downloaded,
'upload_size' => $uploadSize,
View
2 src/Guzzle/Http/Message/Request.php
@@ -152,8 +152,6 @@ public function __clone()
}
$this->curlOptions = clone $this->curlOptions;
$this->params = clone $this->params;
- // Remove state based parameters from the cloned request
- $this->params->remove('curl_handle')->remove('curl_multi');
$this->url = clone $this->url;
$this->response = $this->responseBody = null;
View
5 src/Guzzle/Http/Message/RequestFactory.php
@@ -157,10 +157,7 @@ public function cloneRequestWithMethod(RequestInterface $request, $method)
} elseif ($request instanceof EntityEnclosingRequestInterface) {
$cloned->setBody($request->getBody());
}
- $cloned->getParams()
- ->replace($request->getParams()->getAll())
- ->remove('curl_handle')
- ->remove('curl_multi');
+ $cloned->getParams()->replace($request->getParams()->getAll());
return $cloned;
}
View
4 src/Guzzle/Plugin/Async/AsyncPlugin.php
@@ -49,9 +49,9 @@ public function onCurlProgress(Event $event)
($event['downloaded'] || ($event['uploaded'] && $event['upload_size'] === $event['uploaded']))
) {
// Timeout after 1ms
- curl_setopt($event['handle']->getHandle(), CURLOPT_TIMEOUT_MS, 1);
+ curl_setopt($event['handle'], CURLOPT_TIMEOUT_MS, 1);
// Even if the response is quick, tell curl not to download the body
- curl_setopt($event['handle']->getHandle(), CURLOPT_NOBODY, true);
+ curl_setopt($event['handle'], CURLOPT_NOBODY, true);
}
}
View
9 tests/Guzzle/Tests/Http/Curl/RequestMediatorTest.php
@@ -50,13 +50,4 @@ public function testEmitsEvents()
$this->assertEquals('foo', $this->events[2]['read']);
$this->assertSame($request, $this->events[2]['request']);
}
-
- public function testSetsCurlHandleParameter()
- {
- $request = new EntityEnclosingRequest('PUT', 'http://www.example.com');
- $mediator = new RequestMediator($request);
- $handle = $this->getMockBuilder('Guzzle\Http\Curl\CurlHandle')->disableOriginalConstructor()->getMock();
- $mediator->setCurlHandle($handle);
- $this->assertSame($handle, $request->getParams()->get('curl_handle'));
- }
}
View
6 tests/Guzzle/Tests/Http/Message/RequestFactoryTest.php
@@ -338,11 +338,7 @@ public function testClonesRequestsWithMethodWithoutClient()
{
$f = RequestFactory::getInstance();
$request = $f->create('GET', 'http://www.test.com', array('X-Foo' => 'Bar'));
- $request->getParams()->replace(array(
- 'curl_handle' => 'foo',
- 'curl_multi' => 'bar',
- 'test' => '123'
- ));
+ $request->getParams()->replace(array('test' => '123'));
$request->getCurlOptions()->set('foo', 'bar');
$cloned = $f->cloneRequestWithMethod($request, 'PUT');
$this->assertEquals('PUT', $cloned->getMethod());
View
2 tests/Guzzle/Tests/Plugin/Async/AsyncPluginTest.php
@@ -41,7 +41,7 @@ public function testAddsTimesOutAfterSending()
$handle = CurlHandle::factory($request);
$event = new Event(array(
'request' => $request,
- 'handle' => $handle,
+ 'handle' => $handle->getHandle(),
'uploaded' => 10,
'upload_size' => 10,
'downloaded' => 0

0 comments on commit 15abfe6

Please sign in to comment.
Something went wrong with that request. Please try again.