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.

Files: 
CommentFileSizeAuthor
#70 drupal.http-timeout-156582-70-D6-5.patch3.87 KBdrico
PASSED: [[SimpleTest]]: [MySQL] 190 pass(es).
[ View ]
#69 http-timeout-156582-69.patch3.87 KBdrico
PASSED: [[SimpleTest]]: [MySQL] 190 pass(es).
[ View ]
#67 drupal-http-request-timeout-D6-4.patch5.45 KBdrico
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-http-request-timeout-D6-4.patch. See the log in the details link for more information.
[ View ]
#57 drupal-http-request-timeout-D6-3.patch5.45 KBc960657
PASSED: [[SimpleTest]]: [MySQL] 190 pass(es).
[ View ]
#54 156582-29-2.patch3.38 KBzyxware
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 156582-29-2.patch.
[ View ]
#51 drupal-http-request-timeout-D6-2.patch5.31 KBc960657
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-http-request-timeout-D6-2.patch.
[ View ]
#49 core_common_httptimeout.patch1.32 KBcraig_
FAILED: [[SimpleTest]]: [MySQL] Invalid patch format in core_common_httptimeout.patch.
[ View ]
#38 156582-timeout-context-38.patch4.28 KBpwolanin
PASSED: [[SimpleTest]]: [MySQL] 22,820 pass(es).
[ View ]
#37 156582-timeout-context-37.patch4.31 KBpwolanin
PASSED: [[SimpleTest]]: [MySQL] 21,118 pass(es).
[ View ]
#35 156582-timeout-context-35.patch4.15 KBpwolanin
FAILED: [[SimpleTest]]: [MySQL] 20,740 pass(es), 233 fail(s), and 140 exception(es).
[ View ]
#29 156582-29.patch3.4 KBjpmckinney
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 156582-29.patch.
[ View ]
#28 drupal-http-request-timeout-4.patch911 bytesdrico
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-http-request-timeout-4.patch.
[ View ]
#27 patch.txt911 bytesdrico
#21 drupal_http_request-156582.patch1.19 KBmikeytown2
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_http_request-156582.patch.
[ View ]
#18 drupal-http-request-timeout-3.patch10.11 KBc960657
Passed: 11865 passes, 0 fails, 0 exceptions
[ View ]
#17 drupal-http-request-timeout-2.patch9.7 KBc960657
Failed: 11758 passes, 102 fails, 13 exceptions
[ View ]
#15 drupal-http-request-timeout-1.patch9.52 KBc960657
Passed: 11865 passes, 0 fails, 0 exceptions
[ View ]
#12 156582-12-drupal-http-request.patch9.5 KBkbahey
Failed: Failed to apply patch.
[ View ]
#8 156582-drupal-http-request.patch9.2 KBDamien Tournoud
Failed: Failed to apply patch.
[ View ]
#2 common.inc__6.patch2.53 KBtownxelliot
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch common.inc__6.patch.
[ View ]
common.inc__5.patch2.05 KBtownxelliot
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch common.inc__5.patch.
[ View ]

Comments

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.

StatusFileSize
new2.53 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch common.inc__6.patch.
[ View ]

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

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

moving, I support this feature request.

Assigned:Unassigned» Damien Tournoud

Mine, for some hours.

Assigned:Damien Tournoud» Unassigned
Status:Needs work» Needs review
StatusFileSize
new9.2 KB
Failed: Failed to apply patch.
[ View ]

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

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

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.

StatusFileSize
new9.5 KB
Failed: Failed to apply patch.
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch failed testing.

Subscribe

Status:Needs work» Needs review
StatusFileSize
new9.52 KB
Passed: 11865 passes, 0 fails, 0 exceptions
[ View ]

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?

StatusFileSize
new9.7 KB
Failed: 11758 passes, 102 fails, 13 exceptions
[ View ]

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.

StatusFileSize
new10.11 KB
Passed: 11865 passes, 0 fails, 0 exceptions
[ View ]

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

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.

Version:7.x-dev» 6.x-dev
Category:feature» bug
Status:Closed (fixed)» Active
StatusFileSize
new1.19 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_http_request-156582.patch.
[ View ]

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.

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?

Status:Active» Needs review

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

subscribe for D6

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.

subscribe for D6

StatusFileSize
new911 bytes

Patch for D6

StatusFileSize
new911 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-http-request-timeout-4.patch.
[ View ]

reupload for test, sorry

StatusFileSize
new3.4 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 156582-29.patch.
[ View ]

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.

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.

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?

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.

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

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.

Status:Needs work» Needs review
StatusFileSize
new4.15 KB
FAILED: [[SimpleTest]]: [MySQL] 20,740 pass(es), 233 fail(s), and 140 exception(es).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new4.31 KB
PASSED: [[SimpleTest]]: [MySQL] 21,118 pass(es).
[ View ]

Ah, was passing the parameters incorrectly to stream_socket_client()

StatusFileSize
new4.28 KB
PASSED: [[SimpleTest]]: [MySQL] 22,820 pass(es).
[ View ]

fix error message

Status:Needs review» Reviewed & tested by the community

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

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.

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

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.

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

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

Status:Reviewed & tested by the community» Fixed

This looks good to go. Committed to CVS HEAD.

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

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

subbing for a d6 backport, thanks!

Subscribe

StatusFileSize
new1.32 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid patch format in core_common_httptimeout.patch.
[ View ]

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)

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.

Status:Needs work» Needs review
StatusFileSize
new5.31 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-http-request-timeout-D6-2.patch.
[ View ]

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.

Status:Needs work» Needs review

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

StatusFileSize
new3.38 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 156582-29-2.patch.
[ View ]

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.

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

<?php
@@ -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;
       }
?>

Status:Needs work» Needs review
StatusFileSize
new5.45 KB
PASSED: [[SimpleTest]]: [MySQL] 190 pass(es).
[ View ]

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.

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

Status:Needs work» Needs review

Status:Needs review» Needs work

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

Status:Needs work» Needs review

Subscribing -- need a good patch for D6

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

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

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.

Status:Needs work» Needs review
StatusFileSize
new5.45 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-http-request-timeout-D6-4.patch. See the log in the details link for more information.
[ View ]

Error code changed to -1

Status:Needs review» Needs work

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

StatusFileSize
new3.87 KB
PASSED: [[SimpleTest]]: [MySQL] 190 pass(es).
[ View ]

retrying with a git patch

Status:Needs work» Needs review
StatusFileSize
new3.87 KB
PASSED: [[SimpleTest]]: [MySQL] 190 pass(es).
[ View ]

status

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

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.

Status:Needs review» Reviewed & tested by the community

Looks good to me.

Status:Reviewed & tested by the community» Fixed

Committed, pushed, thank you.

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.