drupal_http_request() should support timeout setting

This is a good idea.

The default in the call is basically the previously hard coded value (15 seconds), so it is backward compatible (at least for http).

However, for https, it used to be 20 seconds. I don't think this is a big deal.

I think we should add a return FALSE or something like that if a timeout happens though (and TRUE if it works well). This way callers can know what is going on.

I realised that about the HTTPS timeout being shortened, but thought it was worth it to keep things consistent. Plus of course you can now override.

I've had another shot at this, adding in some code to check for timeouts while the response is being read. Rather than return true or false, I followed the approach used for timeouts when the socket connections itself times out (i.e. !$fp returns true): I set an error in the $result object and return that immediately. Currently, this will discard any fragment of the response which has been received. I think this is the correct behaviour, but I'm open to suggestions.

I've done some basic testing for this, but it's tricky to check as you need a live website. However, the tests I've done indicate that it works as expected. I can supply the script if you're interested (it doesn't use simpletest, just attempts a request and dumps the output).

One other thing which perhaps needs discussion is whether you should have separate timeouts for the socket connection and the response read. I've used the same value for both at present, so the actual timeout for the whole request could theoretically be > $timeout (e.g. if the socket connects taking $timeout / 2, then the read times out in $timeout seconds).

The patched function now takes 6 arguments. Would this be a good time at which to consider putting some of the optional arguments into an $options array, like l() and some other functions do? Different issue, true, but it'd be a good follow-up if this one gets implemented.

Calling drupal_http_request("http://www.google.com", array(), 'GET', NULL, 3, 20); just to change the timeout to 20 is a bit clumsy...

I agree, but changing the function signature could cause problems with other modules which rely on this (e.g. aggregator.module).

Sure. That change would have to be postponed to 7.x because APIs are already frozen... ;)

moving, I support this feature request.

Mine, for some hours.

A first shot at this. Completely untested at that point.

Haven't tested this, just wanted to support the direction taken in this patch. We really need to start writing tests for drupal_http_request. One would think that PHP has such a function built-in.

I've done some basic testing for this, but it's tricky to check as you need a live website.

You can test it against a PHP script on your own server containing a sleep() call.

One other thing which perhaps needs discussion is whether you should have separate timeouts for the socket connection and the response read.

Perhaps some power users would like to tweak these values, but I assume most users would like to specify just one timeout for the entire call to drupal_http_request().

I've used the same value for both at present, so the actual timeout for the whole request could theoretically be > $timeout (e.g. if the socket connects taking $timeout / 2, then the read times out in $timeout seconds).

AFAIK the timeout set using stream_set_timeout() applies to every individual call to fread(), i.e. per every 1024 bytes. So for a 10 KB file, the total timeout would be $timeout * (10 + 1). This isn't really useful.

Instead I suggest that you add $expiry = time() + $timeout() at the beginning of drupal_http_request() and then add something like this inside the loop doing fread() (perhaps also elsewhere in the function):

You might want to use microtime(true) instead of time() to allow sub-second precision.

With update status in core, any network issues between the site and Drupal.org can cause the admin page to hang, so I think this is pretty important.

This is important.

To see how this can bring a server to its knees, read How relying on connections to third party servers can be detrimental to performance.

A default timeout of 20 to 30 seconds seems reasonable.

The patch fails to apply, here is a reroll that does apply.

The last submitted patch failed testing.

Subscribe

This patch treats the timeout as the maximum time of the entire function call, i.e. the sum of the time spent in fsockopen() and consecutive fread() calls, including redirects.

It's minor but in the test, $start is declared but not used. It's a dead variable.

I wonder about $time < 10. If the time-out is set to two seconds, can't we use something something closer to 2?

I changed the check to 1.8 < $time < 5. This allows a great amount of slack in the test machine. This is intentionally left very tolerant to avoid failures due to "hiccups" in the test bots. We may need to revise these bounds when the tests has been running on various old or heavily loaded machines.

The default values for $options should be populated prior to the fsockopen() call.

Looks great now. Committed to CVS HEAD. Thanks.

Automatically closed -- issue fixed for 2 weeks with no activity.