drupal_http_request() fails

The problem is this stupid, non-type safe and unneeded $chunk = fread($fp, 1024).

Drupal 7 doesn't have that, and is probably not affected.

Let's just remove the stupidity.

That certainly seems to make sense, but the code comment at http://www.php.net/manual/en/function.fread.php#93250 suggests this can lead to an infinite loop? (Perhaps it's a separate issue.)

I remember researching this stuff once before and it got very confusing very fast :(

the issue was also reported here: http://drupal.org/node/609836

with the suggested fix:

while (!feof($fp)) {
   $response .= fgets($fp);
}

but likely the same possible problem? Could be an issue for D7 also? Isnt' there some problem with IIS not sending EOF?

This would maybe fix it? Are we relying on fread returning false or an empty string?

--- includes/common.inc
+++ includes/common.inc
@@ -538,8 +538,8 @@ function drupal_http_request($url, $headers = array(), $method = 'GET', $data =
 
   // Fetch response.
   $response = '';
+ $active = TRUE;
-  while (!feof($fp) && $chunk = fread($fp, 1024)) {
-    $response .= $chunk;
+  while (!feof($fp) && $active) {
+    $chunk = fread($fp, 1024);
+    $active = ($chunk !== FALSE) && (strlen($chunk) > 0);
+    if ($active) {
+      $response .= $chunk;
+    }
   }
   fclose($fp);

@David_Rothstein: there are a lot of FUD in the comments of both fread() and feof(). I'm convinced that none of those are necessary. After all, a FALSE cast to a string is the empty string, so who cares if loop one additional time?

@DamZ - why do you claim there would just be an extra loop? it seems the problem is rather that feof() is not reliable?

There are several bugs open on bugs.php.net about feof() weirdnesses. The most important one is probably #43782 - feof() does not detect timeout on socket, and it's sister issue #46049 - feof() hangs. If I understand those correctly, the behavior changed in 5.2.7 and was reverted in 5.2.8.

It means that we will probably have to fallback to using stream_get_meta_data() at each iteration of the loop (which is ugly and sad).

ok, so something like:

    $info = stream_get_meta_data($fp);
    $alive = !$info['eof'] && !$info['timed_out'];
    $response = '';

    while ($alive) {
        $chunk = fread($fp, 1024);
        $response .= $chunk;
        $info = stream_get_meta_data($fp);
        $alive = !$info['eof'] && !$info['timed_out'] && $chunk;
    }

I'm really not sure if I am on the right track here but I have a similar issue and I'm wondering if my D-Link G604T virtual server setup is somehow to blame.

My understanding is that this router does not do loopback, and I have a feeling that this might have an impact on your problem.

Sorry if I'm a little unclear; basically I'm suggesting that hardware might be the issue.

so likely this bug needs to be handled in 7.x?

Just encountered this peculiar issue on a Drupal 6 site running with PHP 5.2.11 on Solaris (SunOS 5.10). It was truncating some JSON responses from Solr. Peter's code in #7 solved it.

ok, so I'l roll #7 as a patch?

This version seems to fix at least some of the test failures - seems we we not setting the stream timeout, not caching all timeout conditions to return an error code.

Would like this to be backported to D6, too.

Committed to CVS HEAD. Thanks.

Looks like #156582: drupal_http_request() should support timeout setting needs to go in first (in D6).

I'm not sure if Gabor will want to alter the API in #156582: drupal_http_request() should support timeout setting.

On the other hand, timing out the request is a little problematic if we always force the default of 30 seconds.

Here's a sort of combination patch that makes no API changes to Drupal 6. An alternate maximum time for http requests can be set via a variable if needed.

works well

Someone tested this on PHP 4.3.x? By carefully checking the docs, this should theoretically work the same there, but checking would be really useful.

Hui... bad. Linkchecker does HEAD by default and rely on the core function. Have not seen it myself but subscribe now

Subscribing.

Subscribing.

@Gabor - what kind of further review or testing is needed? Just that these functions work on 4.3/4.4?

I can try on 4.4.8, but given that even PHP 5.2 is EOL, perhaps we should up the version requirement for 6.x core to at least 4.4, or preferably 5.2.4 like 7.x?

@pwolanin: see above, I've had a request about 8 months ago without any replies yet. http://drupal.org/node/617126#comment-3665732

@pwolanin: it is helpful for future deconstruction of threads if you don't go back and edit your posts. Minimum PHP version requirement for Drupal 6 is 4.3.5, every core feature should work there too. If not, it is a regression. See: http://drupalcode.org/project/drupal.git/blob/refs/heads/6.x:/modules/sy... If you'd like that changed, please open another issue. I don't consider blocking out hosts running old PHP versions from future bugfixes and security updates to the Drupal 6 branch as something a stable backwards compatible Drupal version should do.

I reinstalled Drupal 6 w/ PHP 4.4.8 and applied the patch. Git patch attached.

Update status fetch seems to work.

I also enabled aggregator module and successfully fetched feed items from

http://drupal.org/project/issues/search/rss?text=&projects=Drupal%20core...

and from

http://drupal.org/planet/rss.xml

I applied the patch with Drupal 6.22 and PHP 5.2 and all is well.

For me, #30 seems to break apachesolr search, both with the standard module and with apachesolr_views. In my testing, Drupal and Apachesolr are on the same server. Viewing the apachesolr config page at admin/settings/apachesolr/settings shows that it has successfully contacted the server, however no search results are returned. I receive no results found for all solr searches. Backing out #30 fixes this for me. I am using Pressflow 6.22.104 with PHP 5.3.8 for what it's worth.