drupal_http_request() should support timeout setting

townxelliot - July 3, 2007 - 22:53
Project:Drupal
Version:7.x-dev
Component:base system
Category:feature request
Priority:normal
Assigned:Unassigned
Status:closed
Description

At present, when using the drupal_http_request() function, there is no way to set a custom timeout for the request, either on the time the socket waits to connect (currently 15 seconds for HTTP and 20 seconds for HTTPS) or the time it waits when writing data to the stream (not set at all). I suggest adding a $timeout argument which can be used to set a custom value for both simultaneously. This helps prevent Drupal from hanging for 30 seconds or so if a remote web service is unavailable.

Patch is against CVS HEAD.

AttachmentSize
common.inc__5.patch2.05 KB

#1

kbahey - July 4, 2007 - 01:16
Status:needs review» needs work

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.

#2

townxelliot - July 5, 2007 - 12:02

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).

AttachmentSize
common.inc__6.patch 2.53 KB

#3

Arancaytar - July 5, 2007 - 13:42

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...

#4

townxelliot - July 5, 2007 - 13:57

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

#5

Arancaytar - July 5, 2007 - 14:06

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

#6

Gerhard Killesreiter - August 12, 2008 - 16:21
Version:6.x-dev» 7.x-dev

moving, I support this feature request.

#7

Damien Tournoud - August 12, 2008 - 16:36
Assigned to:Anonymous» Damien Tournoud

Mine, for some hours.

#8

Damien Tournoud - August 12, 2008 - 21:47
Assigned to:Damien Tournoud» Anonymous
Status:needs work» needs review

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

AttachmentSize
156582-drupal-http-request.patch 9.2 KB
Testbed results
156582-drupal-http-request.patchfailedFailed: Failed to apply patch. Detailed results

#9

Dries - August 13, 2008 - 06:49

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.

#10

c960657 - August 14, 2008 - 21:35

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):

$t = $expiry - time();
if ($t <= 0) {
  $result->error = 'Timeout';
  return $result; 
}
stream_set_timeout($t}

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

#11

catch - October 11, 2008 - 08:50

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.

#12

kbahey - October 11, 2008 - 17:18

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.

AttachmentSize
156582-12-drupal-http-request.patch 9.5 KB
Testbed results
156582-12-drupal-http-request.patchfailedFailed: Failed to apply patch. Detailed results

#13

Anonymous (not verified) - November 12, 2008 - 12:50
Status:needs review» needs work

The last submitted patch failed testing.

#14

hass - February 9, 2009 - 11:42

Subscribe

#15

c960657 - June 5, 2009 - 22:07
Status:needs work» needs review

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.

AttachmentSize
drupal-http-request-timeout-1.patch 9.52 KB
Testbed results
drupal-http-request-timeout-1.patchpassedPassed: 11865 passes, 0 fails, 0 exceptions Detailed results

#16

Dries - June 6, 2009 - 07:52

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?

#17

c960657 - June 6, 2009 - 11:54

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.

AttachmentSize
drupal-http-request-timeout-2.patch 9.7 KB
Testbed results
drupal-http-request-timeout-2.patchfailedFailed: 11758 passes, 102 fails, 13 exceptions Detailed results

#18

c960657 - June 6, 2009 - 12:03

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

AttachmentSize
drupal-http-request-timeout-3.patch 10.11 KB
Testbed results
drupal-http-request-timeout-3.patchpassedPassed: 11865 passes, 0 fails, 0 exceptions Detailed results

#19

Dries - June 6, 2009 - 15:45
Status:needs review» fixed

Looks great now. Committed to CVS HEAD. Thanks.

#20

System Message - June 20, 2009 - 15:50
Status:fixed» closed

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

 
 

Drupal is a registered trademark of Dries Buytaert.