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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

kbahey’s picture

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.

townxelliot’s picture

FileSize
2.53 KB

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

cburschka’s picture

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

townxelliot’s picture

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

cburschka’s picture

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

Gerhard Killesreiter’s picture

Version: 6.x-dev » 7.x-dev

moving, I support this feature request.

Damien Tournoud’s picture

Assigned: Unassigned » Damien Tournoud

Mine, for some hours.

Damien Tournoud’s picture

Assigned: Damien Tournoud » Unassigned
Status: Needs work » Needs review
FileSize
9.2 KB

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

Dries’s picture

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.

c960657’s picture

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.

catch’s picture

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.

kbahey’s picture

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.

Anonymous’s picture

Status: Needs review » Needs work

The last submitted patch failed testing.

hass’s picture

Subscribe

c960657’s picture

Status: Needs work » Needs review
FileSize
9.52 KB

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.

Dries’s picture

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?

c960657’s picture

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.

c960657’s picture

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

Dries’s picture

Status: Needs review » Fixed

Looks great now. Committed to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)

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

mikeytown2’s picture

Version: 7.x-dev » 6.x-dev
Category: feature » bug
Status: Closed (fixed) » Active
FileSize
1.19 KB

I consider this a bug, as it totally ignores the servers setting for default_socket_timeout. Some requests can take longer then 15 seconds, and this behavior is not well documented. Example: #553994: drupal_http_request timing out. Below is a patch that uses ini_get("default_socket_timeout"), the default for http://php.net/fsockopen. I made the ssl add in an additional 5 seconds just like it does already. This way one can control this by doing ini_set('default_socket_timeout', 60); without changing the API.

donquixote’s picture

Please port this to 6.x !!
Why mark something as fixed before it is backported? What's the correct procedure that does not encourage to forget the backport?

mikeytown2’s picture

Status: Active » Needs review

@donquixote
The above patch #21 is for 6.x

febbraro’s picture

subscribe for D6

mikeytown2’s picture

There might be a bug with the 7.x implementation of this, depending on the server setup.
See #577578: boost_cron function failing, due to fsockopen() not respecting user given timeout value

I lifted some code from drupal_http_request() and called it boost_drupal_http_request(); allows for timeouts like it does in 7.x only thing is it doesn't work 100% of the time. Had to add in ini_set() and now timeout works.

chadd’s picture

subscribe for D6

driki_’s picture

FileSize
911 bytes

Patch for D6

driki_’s picture

reupload for test, sorry

jpmckinney’s picture

FileSize
3.4 KB

Here is a more of less direct patch from the D7 version in #18. (I hand-wrote the diff header, but it still applies cleanly.)

The only question about this patch is whether it's kosher to add an optional argument to drupal_http_request.

jonvk’s picture

Status: Needs review » Reviewed & tested by the community

I've tested on a site running 6.16, and this works fine.

The constant definition for the timeout HTTP_REQUEST_TIMEOUT does not create a problem for core modules. I haven't checked how contrib modules interpret the $result->code though. D7 changed the timeout default to 30 seconds and this patch does too. I don't see any harm in it though.

pwolanin’s picture

Status: Reviewed & tested by the community » Needs work

Ugh, a bunch of the Drupal 7 changes are not very good and should be altered there and not back ported. For example - why would we use the full timeout as the fsockopen() connection timeout?

pwolanin’s picture

Version: 6.x-dev » 7.x-dev

oh, I see - we are counting the connect time as part of the timeout. I still don't think it makes sense to treat these both as part of the same timeout, since it's possible the connect timeout should fail on a very different time scale than the read timeout.

pwolanin’s picture

In fact, if we want the stream to possibly time out on writing data (sending the request) we need to call stream_set_timeout() earlier in the function as well.

http://us.php.net/manual/en/function.stream-set-timeout.php

pwolanin’s picture

Seems like #21 may be relevant to Drupal 7 not just 6? however, the default value is 60, which seems way too long for a connect timeout.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
4.15 KB

Fixes two bugs:

1) not setting a stream timeout before calling fwrite().

2) failure to utilize the better PHP 5 function stream_socket_client() which allows the use of a context resource if desired.

Status: Needs review » Needs work

The last submitted patch, 156582-timeout-context-35.patch, failed testing.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
4.31 KB

Ah, was passing the parameters incorrectly to stream_socket_client()

pwolanin’s picture

fix error message

Dries’s picture

Status: Needs review » Reviewed & tested by the community

Looking at the code, this looks RTBC. I have not tested it though.

dhthwy’s picture

Btw setting a timeout on an SSL stream socket is broken http://bugs.php.net/bug.php?id=43796. I ran into this problem myself on PHP 5.3.2, but it might be fixed in late versions of 5.2. You can't get it around it by setting the socket to non-blocking either because stream_select is also broken http://bugs.php.net/bug.php?id=41489, unless you want to do manual polling and flood read/write system calls to see if data is available for reading/ data can be written. Don't know if this applicable here but I thought I'd chime in and mention it.

pwolanin’s picture

Wow, that's unfortunate. This patch adds one call to stream_set_timeout(), but we already have several.

dhthwy’s picture

Yea there's really nothing we can do about it. If it's SSL and timeout is broken on the user's PHP version it will just be a NOOP -- as if there were no timeout set at all. The code looks fine to me and having a timeout set is extremely important to avoid scripts hanging. So +1 to RTBC.

Note for future reference that using floating point numbers for the timeout/or whatever isn't necessary since in C if you give a float an integer it will be implicitly converted to float. So for example, passing in 30 to a variable of type float will simply be converted to 30.0. This works the other way around too, so setting an integer to 30.8 will lose its precision and be converted or truncated down to 30. This applies to PHP as well since the function it's calling is written in C. This might work differently in Java and other languages.

pwolanin’s picture

Yes, I know it will be converted, but I thought making it an explicit float is more clear.

pwolanin’s picture

#38: 156582-timeout-context-38.patch queued for re-testing.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

This looks good to go. Committed to CVS HEAD.

mikeytown2’s picture

Version: 7.x-dev » 6.x-dev
Status: Fixed » Needs work

#29 needs to re sync with D7's latest changes.

arski’s picture

subbing for a d6 backport, thanks!

XiaN Vizjereij’s picture

Subscribe

craig_’s picture

an additional way to test this (for those running solr servers):
1. stop solr server
2. $solr = apachesolr_get_solr();
3. $ping = $solr->ping(1);

this adds the 15s delay to page render for searches, as well as non-search pages with MLT blocks, if the server is offline

this highlights the difference stated in #32 @pwolanin , about connect vs. read

+1 for d6 backport

for the d6 fix, could this be as simple as the intention of the initial patch @townxelliot + #21 @mikeytown?
add $timeout=15 to the signature, and make ssl = timeout+5. then, only applications needing to do an explicit connect timeout, can just smack a timeout onto the end of their calls

(sorry if i missed something above. the ongoing patch effort seems to be focused on v7 and otherwise changing system-wide settings)

mikeytown2’s picture

If we are going for an API change then #49 is the best way to accomplish this. stream_socket_client is a PHP 5+ function thus we can't use it here.

c960657’s picture

Status: Needs work » Needs review
FileSize
5.31 KB

This is the patch from #29 with a backport of item 1 from #35 (item 2 cannot be backported, because stream_socket_client() requires PHP5).

Perhaps we should set the default timeout to something very high (e.g. 5 minutes) rather than 30 seconds to avoid breaking existing code that rely on calling some very slow services.

Status: Needs review » Needs work

The last submitted patch, drupal-http-request-timeout-D6-2.patch, failed testing.

c960657’s picture

Status: Needs work » Needs review

The patch still applies. The testbot fails due to #961172: All D6 Core patches are failing.

zyxware’s picture

FileSize
3.38 KB

Here is the patch 156582-29.patch corrected for Pressflow 6.20.97.

Status: Needs review » Needs work

The last submitted patch, 156582-29-2.patch, failed testing.

janusman’s picture

Maybe I'm wrong, but shouldn't the call to drupal_http_request() on this hunk also specify the $timeout parameter?

@@ -591,8 +612,12 @@ function drupal_http_request($url, $headers = array(), $method = 'GET', $data =
     case 302: // Moved temporarily
     case 307: // Moved temporarily
       $location = $result->headers['Location'];
-
-      if ($retry) {
+      $timeout -= timer_read(__FUNCTION__) / 1000;
+      if ($timeout <= 0) {
+        $result->code = HTTP_REQUEST_TIMEOUT;
+        $result->error = 'request timed out';
+      }
+      elseif ($retry) {
         $result = drupal_http_request($result->headers['Location'], $headers, $method, $data, --$retry);
         $result->redirect_code = $result->code;
       }
c960657’s picture

Status: Needs work » Needs review
FileSize
5.45 KB

You are right. I have updated the patch. Please take another look.

I also replaced an occurrence of $timeout -= timer_read(__FUNCTION__) / 1000; with $time_left = $timeout - timer_read(__FUNCTION__) / 1000;, so that we don't change the value of the argument that was passed to drupal_http_request() except immediately before the recursive call - similar to how it works in the D7 patch.

Status: Needs review » Needs work

The last submitted patch, drupal-http-request-timeout-D6-3.patch, failed testing.

c960657’s picture

The patch in #57 applies without problems. Please review it. The testbot fails due to #961172: All D6 Core patches are failing.

Renee S’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, drupal-http-request-timeout-D6-3.patch, failed testing.

c960657’s picture

Status: Needs work » Needs review
panhead490’s picture

Subscribing -- need a good patch for D6

mikeytown2’s picture

@panhead490
Try the patch at comment #57. Report here if it works for you.

sun’s picture

The value of the constant could have used some more in-depth reviews here. See #1246376: HTTP_REQUEST_TIMEOUT constant value is not negative

sun’s picture

Status: Needs review » Needs work
Issue tags: +Needs backport to D6

Aforementioned issue has been committed to D8 + D7, so this patch for D6 should be adjusted accordingly.

driki_’s picture

Status: Needs work » Needs review
FileSize
5.45 KB

Error code changed to -1

Status: Needs review » Needs work

The last submitted patch, drupal-http-request-timeout-D6-4.patch, failed testing.

driki_’s picture

retrying with a git patch

driki_’s picture

Status: Needs work » Needs review
FileSize
3.87 KB

status

mikeytown2’s picture

@drico
FYI, if your looking for something with more power in D6, check out the HTTP Parallel Request Library module.

c960657’s picture

I have verified that the latest patch is indeed identical to the patch in #57, except for the changed value of the constant (and the fact that it has been rolled with git instead of CVS). But since I created the patch in #57, I'm not really in a position to mark it as RTBC.

catch’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Committed, pushed, thank you.

pwolanin’s picture

Version: 6.x-dev » 8.x-dev

do we need to separate the timeouts for connection and response?

Status: Fixed » Closed (fixed)
Issue tags: -Needs backport to D6

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