Varnish helper to use cUrl instead of fopen for requests #17

Merged
merged 6 commits into from Oct 23, 2012

Projects

None yet

5 participants

@jnonon

Varnish helper does not offer any type of insight into the status for the request.
Using curl adds a dependency over the helper, but gives the user more control over the request

@lsmith77
Liip member

alternatively we could instead make use of buzz or some other http client lib ..

@jnonon

Guzzle would work as well. I just didn't want to make too many dependencies for a simple purge request. Your point is valid and acceptable.

@lsmith77
Liip member

Isnt Guzzle a bit oversized? Then again its well maintained, while Buzz isnt that well maintained.

@dbu
Liip member
dbu commented Oct 8, 2012

is there some interface for web clients we could use to be compatible with more than 1? or is there a preference in symfony core for one of the clients so people do not need to have 2 different clients in their codebase?

@lsmith77
Liip member

there is no interface (there was some discussion on the FIG list a while back to create one). traditionally Buzz has been preferred because Kris is part of the inner Symfony circles, but like I said imho its not very well maintained. Then again due to the very small target feature set it doesnt need that much maintenance either.

@dbu
Liip member
dbu commented Oct 8, 2012

hi @jnonon,

just discussed with lukas again and we think curl should be fine for now. if ever some interfaces emerge, we can always refactor.

can you please add ext-curl to the suggest: and require-dev: dependencies in composer.json? (in suggest, explain what the extension is used for)

and in the dependency injection, please check if the varnish helper is enabled if the curl extension is available and throw a meaningful exception otherwise:
https://github.com/liip/LiipCacheControlBundle/blob/master/DependencyInjection/LiipCacheControlExtension.php#L48

@jnonon

I will work in your suggested changes in the next couple of hours.
Thanks for all your recommendations and responses.

@dbu
Liip member
dbu commented Oct 8, 2012

cool. i have to thank you for the contributions :-)

Jonathan Nonon Changes:
* Updated composer dependencies
* Added an Exception to be thrown when extension curl is not present.
* Updated README.md
da79dd1
@jnonon

Let me know if I overlooked any other detail. Thanks.

@lsmith77
Liip member

could you go over the points mentioned here?
http://jmsyst.com/reviews/9f6449f7-c1b7-4a70-8be5-6df3de4688d3

@cryptocompress cryptocompress commented on an outdated diff Oct 8, 2012
Helper/Varnish.php
- fclose($fp);
+ list($header, $body) = explode("\r\n\r\n", $response, 2);
@cryptocompress
cryptocompress Oct 8, 2012

you can get length of header from curl:

    $length = curl_getinfo($connection, CURLINFO_HEADER_SIZE);

    $header = substr($response, 0, $length);
    $body   = substr($response, $length);
@dbu dbu commented on an outdated diff Oct 9, 2012
Helper/Varnish.php
+ $requestResponseByIp[$ip] = array('headers' => $header, 'body' => $body);
+
+ }
+
+ return $requestResponseByIp;
+
+ }
+ /**
+ * Override or modify default cUrl Options
+ * @param array $options
+ */
+ public function setRequestOptions($options)
+ {
+
+ foreach($options as $option => $value) {
+
@dbu
dbu Oct 9, 2012

extra white line here

@dbu dbu commented on an outdated diff Oct 9, 2012
Helper/Varnish.php
}
/**
* Send a request to all configured varnishes
*
- * @param string $request request string
+ * @param array $request request string
* @throws \RuntimeException if connection to one of the varnish servers fails. TODO: should we be more tolerant?
@dbu
dbu Oct 9, 2012

can you please doc comment the return value?

@dbu dbu commented on an outdated diff Oct 9, 2012
Helper/Varnish.php
}
/**
- * Force this absolute path to be refreshed
+ * Force this absolute path to be refreshed
*
* @param string $path Must be an absolute path
* @throws \RuntimeException if connection to one of the varnish servers fails.
@dbu
dbu Oct 9, 2012

especially for the public methods: can you please doc comment the return value?

@dbu dbu commented on the diff Oct 9, 2012
Helper/Varnish.php
@@ -66,51 +74,81 @@ public function __construct($domain, array $ips, $port)
*/
@dbu
dbu Oct 9, 2012

especially for the public methods: can you please doc comment the return value?

@dbu
dbu Oct 21, 2012

this is done now.

@dbu dbu and 1 other commented on an outdated diff Oct 9, 2012
DependencyNotMetException.php
@@ -0,0 +1,15 @@
+<?php
+namespace Liip\CacheControlBundle;
+
+/**
+ * Thrown when a bundle dependency is not met.
+ *
+ * @author jnonon <jnonon@github.com>
+ *
+ */
+use Exception;
+
+class DependencyNotMetException extends Exception
@dbu
dbu Oct 9, 2012

not sure if we need a specific exception for this. i think i never created one for DI and this will not be something the user is going to catch anyways. if anything, it should be an exception in symfony2 core which afaik does not exist.

@stof is there any best practice for this?

@stof
stof Oct 9, 2012

I would simply use RuntimeException here, especially as it is used in the DI extension, so not in a place where you are likely to catch it.

And btw, if the composer file is done properly, this exception should never be thrown as composer would fail to install the bundle when curl is not loaded :)

@dbu
dbu Oct 9, 2012

@stof thanks for the feedback, lets use RuntimeException then. it is correct to not require curl in the composer.json as this service is optional and users can use the bundle without.

@stof
stof Oct 9, 2012

yeah, I saw it is an optional dependency after commenting

@stof stof and 1 other commented on an outdated diff Oct 9, 2012
Helper/Varnish.php
@@ -56,6 +58,12 @@ public function __construct($domain, array $ips, $port)
}
$this->ips = $ips;
$this->port = $port;
+
+ $this->curlHandler = curl_init($this->domain);
+ //Default Option
+ curl_setopt($this->curlHandler, CURLOPT_RETURNTRANSFER, true);
+ curl_setopt($this->curlHandler, CURLOPT_HEADER, true); // Display headers
@stof
stof Oct 9, 2012

could you avoid doing a curl init in the constructor ? the constructor should be as lightweight as possible to avoid making DI expensive. The logic should be done only when it is needed

@dbu
dbu Oct 9, 2012

@jnonon you could do a protected getCurl that checks if $this->curlHandler is initialized yet and otherwise initializes it.

@stof
stof Oct 9, 2012

btw, Buzz had some weird issues a while ago because they were reusing the same curl resource for several request. I don't remember if it was general or only for some PHP setup. But Buzz switched to create a curl handler per request instead to fix it.
So reusing the curl handle should be done carefully.

@jnonon

I will be adding my changes soon. I have one more question, What will be the right approach:
a) Create a new curl handler per request
b) Create a single resource

Thanks for your recommendations. @stof @dbu

@cryptocompress

a) Create a new curl handler per request
b) is micro-optimization. on mass requests reuse should be done with multi_exec

@jnonon jnonon Changes:
* Removed DependencyNotMetException in favor of RuntimeException
* Varnish helper now uses a single curl handler per request.
068c73b
@jnonon

Updates?

@dbu dbu commented on an outdated diff Oct 15, 2012
Helper/Varnish.php
@@ -40,6 +40,8 @@ class Varnish
private $domain;
private $port;
+ private $curlHandler;
@dbu
dbu Oct 15, 2012

i think you can now remove this as you create a new handler for each request, right?

@dbu
Liip member

sorry, github does not notify when a new commit is added... please comment so that i notice its ready.

@stof stof commented on an outdated diff Oct 15, 2012
Helper/Varnish.php
* @throws \RuntimeException if connection to one of the varnish servers fails.
*/
- public function invalidatePath($path)
+ public function invalidatePath($path, $options = array())
@stof
stof Oct 15, 2012

please typehint the array

@stof stof commented on an outdated diff Oct 15, 2012
Helper/Varnish.php
{
+
+ $requestResponseByIp = array();
+
+ $curlHandler = curl_init($this->domain);
+ //Default Options
+ curl_setopt($curlHandler, CURLOPT_RETURNTRANSFER, true);
+ curl_setopt($curlHandler, CURLOPT_HEADER, true); // Display headers
+
+ foreach($options as $option => $value) {
@stof
stof Oct 15, 2012

missing space after foreach

@stof stof commented on an outdated diff Oct 15, 2012
Helper/Varnish.php
{
+
+ $requestResponseByIp = array();
+
+ $curlHandler = curl_init($this->domain);
+ //Default Options
+ curl_setopt($curlHandler, CURLOPT_RETURNTRANSFER, true);
+ curl_setopt($curlHandler, CURLOPT_HEADER, true); // Display headers
+
+ foreach($options as $option => $value) {
+
+ curl_setopt($curlHandler, (int)$option, $value);
@stof
stof Oct 15, 2012

extra empty line, and missing space after the typecast

@stof stof commented on an outdated diff Oct 15, 2012
Helper/Varnish.php
- fwrite($fp, $request);
+ curl_setopt($curlHandler, CURLOPT_URL, $ip.':'.$this->port.$request['path']);
+
+ $response = curl_exec($curlHandler);
+
+ list($header, $body) = explode("\r\n\r\n", $response, 2);
@stof
stof Oct 15, 2012

you should handle curl errors

@stof stof commented on an outdated diff Oct 15, 2012
composer.json
@@ -22,5 +25,8 @@
"autoload": {
"psr-0": { "Liip\\CacheControlBundle": "" }
},
+ "suggest": {
+ "ext-curl": "Used by Varnish Helper to construct requests. See http://www.php.net/manual/en/curl.installation.php"
@stof
stof Oct 15, 2012

I would remove the link to the PHP documentation from here to make the message shorter

@jnonon jnonon Changes:
* Added missing typehint
* Added return values when cUrl errors occurs
* Modified composer definition
43a9bb9
@jnonon

Please, confirm my changes.

@cryptocompress cryptocompress commented on the diff Oct 16, 2012
Helper/Varnish.php
- // read answer to the end, to be sure varnish is finished before continuing
- while (!feof($fp)) {
- fgets($fp, 128);
+ $response = curl_exec($curlHandler);
+
+ //Failed
+ if ($response === false) {
+ $header = '';
+ $body = '';
+ $error = curl_error($curlHandler);
+ $errorNumber = curl_errno($curlHandler);
@cryptocompress
cryptocompress Oct 16, 2012

throw "Exception(curl_error($curlHandler), curl_errno($curlHandler));" here

@jnonon
jnonon Oct 16, 2012

I didn't want to throw an exception because it can work on one IP; but fail in another one. Let's see an example:
Given ip1, ip2, ..., ipN as Varnish Servers and ip4 is the one and only one failing, then everything from ip4 to ipN will be unprocessed.

@cryptocompress
cryptocompress Oct 16, 2012

i see your point but "continue on error" looks wrong. you should at least throw the exception after foreach. do not return errors.

maybe you want to use multi_exec for multiple hosts/ips...

@dbu
dbu Oct 21, 2012

agree with that. and make sure not to overwrite an error return value with a non-error one. i suggest you get the return code into a temp value and append a string to $errorMessage for each backend that fails. then throw after the loop if the error message is not empty.

@cryptocompress cryptocompress and 1 other commented on an outdated diff Oct 16, 2012
Helper/Varnish.php
{
+
+ $requestResponseByIp = array();
+
+ $curlHandler = curl_init($this->domain);
+ //Default Options
+ curl_setopt($curlHandler, CURLOPT_RETURNTRANSFER, true);
+ curl_setopt($curlHandler, CURLOPT_HEADER, true); // Display headers
@cryptocompress
cryptocompress Oct 16, 2012

these options should be set after foreach or you can overwrite them accidentally

@jnonon

Fixed bug reported by @cryptocompress

@lsmith77
Liip member

@dbu: can you handle the final review?

@dbu dbu commented on the diff Oct 21, 2012
DependencyInjection/LiipCacheControlExtension.php
@@ -46,6 +47,12 @@ public function load(array $configs, ContainerBuilder $container)
}
if (!empty($config['varnish'])) {
+
+ if (!extension_loaded('curl')) {
+ throw new RuntimeException('Varnish Helper requires cUrl php extension. Please install it to continue');
+
@dbu
dbu Oct 21, 2012

unnecessary empty line

@dbu
Liip member

i agree with @cryptocompress that it would be better to have an exception thrown than an array returned that the client has to cicle manually to find out if the cache invalidation operation was successful. apart from that detail, this would be ready to merge.

@jnonon are you ok to change this behaviour?

@jnonon

@dbu, I see a problem if we throw an exception. As explained to @cryptocompress, Assume we have servers in locations/zones/countries A, B and C, with a set of Varnish servers {a1, a2, ..., aN}, {b1, b2, ..., bN} and {c1, c2, ..., cN} respectively; Given a failure in Zone B, specifically server b2 fails for a reason, and for the sake of the example, a timeout. Throwing an exception will stop processing other servers in Zone B (b3, b4, ..., bN) and Zone C completely. If we don't want to stop the executions, we will have to add more logic to 'continue-on-error' which will add more complexity to the class.

In this particular case (purging/refreshing cache) we don't want to stop the execution, we just need the status response, which will allow us to take another action for the specific server with a failure.

What do you think?

@dbu dbu merged commit 7771185 into liip:master Oct 23, 2012

1 check passed

Details default Review: No Comments — Travis: Passed
@lsmith77
Liip member

@jnonon thank you for working through all the feedback!

@dbu
Liip member

indeed, thanks for bearing with us :-)

i was thinking of collecting errors and only throwing an exception at the end, not to stop invalidating as soon as there is an error. but i guess you are right that either the user cares and has to do something anyway, and if he does not care an exception would be bad.
if you could do a PR to update the README a bit and mention that point, would be awesome.

@jnonon

@dbu I opened a new pull request on README.
Thanks to all of you for your suggestions and patience :-D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment