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.
Comment | File | Size | Author |
---|---|---|---|
#70 | drupal.http-timeout-156582-70-D6-5.patch | 3.87 KB | driki_ |
#69 | http-timeout-156582-69.patch | 3.87 KB | driki_ |
#67 | drupal-http-request-timeout-D6-4.patch | 5.45 KB | driki_ |
#57 | drupal-http-request-timeout-D6-3.patch | 5.45 KB | c960657 |
#54 | 156582-29-2.patch | 3.38 KB | zyxware |
Comments
Comment #1
kbahey CreditAttribution: kbahey commentedThis 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.
Comment #2
townxelliot CreditAttribution: townxelliot commentedI 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).
Comment #3
cburschkaThe 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...Comment #4
townxelliot CreditAttribution: townxelliot commentedI agree, but changing the function signature could cause problems with other modules which rely on this (e.g. aggregator.module).
Comment #5
cburschkaSure. That change would have to be postponed to 7.x because APIs are already frozen... ;)
Comment #6
Gerhard Killesreiter CreditAttribution: Gerhard Killesreiter commentedmoving, I support this feature request.
Comment #7
Damien Tournoud CreditAttribution: Damien Tournoud commentedMine, for some hours.
Comment #8
Damien Tournoud CreditAttribution: Damien Tournoud commentedA first shot at this. Completely untested at that point.
Comment #9
Dries CreditAttribution: Dries commentedHaven'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.Comment #10
c960657 CreditAttribution: c960657 commentedYou can test it against a PHP script on your own server containing a sleep() call.
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().
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.
Comment #11
catchWith 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.
Comment #12
kbahey CreditAttribution: kbahey commentedThis 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.
Comment #13
Anonymous (not verified) CreditAttribution: Anonymous commentedThe last submitted patch failed testing.
Comment #14
hass CreditAttribution: hass commentedSubscribe
Comment #15
c960657 CreditAttribution: c960657 commentedThis 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.
Comment #16
Dries CreditAttribution: Dries commentedIt'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?Comment #17
c960657 CreditAttribution: c960657 commentedI 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.
Comment #18
c960657 CreditAttribution: c960657 commentedThe default values for $options should be populated prior to the fsockopen() call.
Comment #19
Dries CreditAttribution: Dries commentedLooks great now. Committed to CVS HEAD. Thanks.
Comment #21
mikeytown2 CreditAttribution: mikeytown2 commentedI 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 doingini_set('default_socket_timeout', 60);
without changing the API.Comment #22
donquixote CreditAttribution: donquixote commentedPlease 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?
Comment #23
mikeytown2 CreditAttribution: mikeytown2 commented@donquixote
The above patch #21 is for 6.x
Comment #24
febbraro CreditAttribution: febbraro commentedsubscribe for D6
Comment #25
mikeytown2 CreditAttribution: mikeytown2 commentedThere 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.
Comment #26
chadd CreditAttribution: chadd commentedsubscribe for D6
Comment #27
driki_ CreditAttribution: driki_ commentedPatch for D6
Comment #28
driki_ CreditAttribution: driki_ commentedreupload for test, sorry
Comment #29
jpmckinney CreditAttribution: jpmckinney commentedHere 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.
Comment #30
jonvk CreditAttribution: jonvk commentedI'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.
Comment #31
pwolanin CreditAttribution: pwolanin commentedUgh, 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?
Comment #32
pwolanin CreditAttribution: pwolanin commentedoh, 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.
Comment #33
pwolanin CreditAttribution: pwolanin commentedIn 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
Comment #34
pwolanin CreditAttribution: pwolanin commentedSeems 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.
Comment #35
pwolanin CreditAttribution: pwolanin commentedFixes 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.
Comment #37
pwolanin CreditAttribution: pwolanin commentedAh, was passing the parameters incorrectly to stream_socket_client()
Comment #38
pwolanin CreditAttribution: pwolanin commentedfix error message
Comment #39
Dries CreditAttribution: Dries commentedLooking at the code, this looks RTBC. I have not tested it though.
Comment #40
dhthwy CreditAttribution: dhthwy commentedBtw 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.
Comment #41
pwolanin CreditAttribution: pwolanin commentedWow, that's unfortunate. This patch adds one call to stream_set_timeout(), but we already have several.
Comment #42
dhthwy CreditAttribution: dhthwy commentedYea 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.
Comment #43
pwolanin CreditAttribution: pwolanin commentedYes, I know it will be converted, but I thought making it an explicit float is more clear.
Comment #44
pwolanin CreditAttribution: pwolanin commented#38: 156582-timeout-context-38.patch queued for re-testing.
Comment #45
Dries CreditAttribution: Dries commentedThis looks good to go. Committed to CVS HEAD.
Comment #46
mikeytown2 CreditAttribution: mikeytown2 commented#29 needs to re sync with D7's latest changes.
Comment #47
arski CreditAttribution: arski commentedsubbing for a d6 backport, thanks!
Comment #48
XiaN Vizjereij CreditAttribution: XiaN Vizjereij commentedSubscribe
Comment #49
craig_ CreditAttribution: craig_ commentedan 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)
Comment #50
mikeytown2 CreditAttribution: mikeytown2 commentedIf 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.
Comment #51
c960657 CreditAttribution: c960657 commentedThis 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.
Comment #53
c960657 CreditAttribution: c960657 commentedThe patch still applies. The testbot fails due to #961172: All D6 Core patches are failing.
Comment #54
zyxware CreditAttribution: zyxware commentedHere is the patch 156582-29.patch corrected for Pressflow 6.20.97.
Comment #56
janusman CreditAttribution: janusman commentedMaybe I'm wrong, but shouldn't the call to drupal_http_request() on this hunk also specify the $timeout parameter?
Comment #57
c960657 CreditAttribution: c960657 commentedYou 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.Comment #59
c960657 CreditAttribution: c960657 commentedThe patch in #57 applies without problems. Please review it. The testbot fails due to #961172: All D6 Core patches are failing.
Comment #60
Renee S CreditAttribution: Renee S commented#57: drupal-http-request-timeout-D6-3.patch queued for re-testing.
Comment #62
c960657 CreditAttribution: c960657 commented#57: drupal-http-request-timeout-D6-3.patch queued for re-testing.
Comment #63
panhead490 CreditAttribution: panhead490 commentedSubscribing -- need a good patch for D6
Comment #64
mikeytown2 CreditAttribution: mikeytown2 commented@panhead490
Try the patch at comment #57. Report here if it works for you.
Comment #65
sunThe value of the constant could have used some more in-depth reviews here. See #1246376: HTTP_REQUEST_TIMEOUT constant value is not negative
Comment #66
sunAforementioned issue has been committed to D8 + D7, so this patch for D6 should be adjusted accordingly.
Comment #67
driki_ CreditAttribution: driki_ commentedError code changed to -1
Comment #69
driki_ CreditAttribution: driki_ commentedretrying with a git patch
Comment #70
driki_ CreditAttribution: driki_ commentedstatus
Comment #71
mikeytown2 CreditAttribution: mikeytown2 commented@drico
FYI, if your looking for something with more power in D6, check out the HTTP Parallel Request Library module.
Comment #72
c960657 CreditAttribution: c960657 commentedI 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.
Comment #73
catchLooks good to me.
Comment #74
Gábor HojtsyCommitted, pushed, thank you.
Comment #75
pwolanin CreditAttribution: pwolanin commenteddo we need to separate the timeouts for connection and response?