Some HTTP Transports do not respect transfer timeouts

[sivel]

While doing some testing I found that fopen, fsockopen and streams do not respect transfer timeouts.

Out fopen transport cannot support a transfer timeout properly because we do not pass the 3rd optional parameter to fopen for stream context. We don't do this on purpose because the 3rd parameter was not added until php 5, and that is partially what the streams transport is designed to cover. So in short, unless we want to conditionally use the streams context here for php versions > 5, we cannot reliably use transfer timeouts.

The streams timeout was missing the context for timeout, which does support "microtime". However, unsure at this point why, but the timeout needs to be divided by 2, so that the timeout happens requested timeout.

With fsockopen, we need to actually test for the timeout during the while loop, because the stream_set_timeout applies to each pass of fread and not the total process.

[sivel]

Some info about my test setup:

Some code for your themes functions.php:

//add_filter('use_streams_transport', '__return_false');
add_filter('use_fsockopen_transport', '__return_false');
add_filter('use_fopen_transport', '__return_false');
add_filter('use_http_extension_transport', '__return_false');
add_filter('use_curl_transport', '__return_false');
$response = wp_remote_get('http://localhost/timeout.php', array('timeout' => 5.5));
var_dump($response);

timeout.php

<?php
$i = 0;
while ( $i < 15 ) {
        sleep(1);
        $i++;
}
?>

Comment out the transport that you want to use.

[dd32]

See #11468 and [12473] for related timeout issues

[jacobsantos]

Using the 'timeout' option for streams is kind of iffy because it is only supported on PHP5.2.1+. Granted majority of PHP5 hosts are going to have upgraded to at least PHP5.2.3 by now, there are likely still enough people on dedicated or shared hosts that are running PHP5.1.6 (since CentOS doesn't have an official release for PHP5.2.x yet or if they do it is a recent one which still might not have people upgrade to it yet).

I have a fix for only adding Fopen Transport when PHP4 is used and not when PHP5 is used. I think right now, it will go Streams -> Fopen -> Fsockopen, which is not how it should be on PHP5. It should be Streams -> Fsockopen. There is really only so much you can do for Fopen, so if that set_stream_timeout isn't working then I'm not exactly sure what the fix is for it. I'm surprised really that it isn't working because that is how PHP says you are supposed to set the Timeout for stream connections and for PHP4 connections.

I'm thinking about implementing the Socket way, but I believe that is also PHP5+. It may fix a lot of the issues with the Streams and provide another reliable transport.

Thanks for the additional information. When I first got into this, I figured, "Well, it isn't like the Language itself is going to have bugs, so I just follow the normal way of implementing this. Right?" Wrong. Thankfully, WordPress has many people working on the HTTP API, because I don't believe one person alone could have discovered all of these problems alone in PHP or at least discovered workarounds.

[sivel]

Also see #13915 which is to disable fopen for PHP 5+

[sivel]

Refreshed patch

[aaroncampbell]

Re-refreshed patch

[aaroncampbell]

Same patch, just refreshed for current trunk.

[sivel]

This should likely go in after #16236. At which point it will require another refresh, since a fairly large amount of code changes will happen to the transports in that commit.

[dd32]

In the case of the Streams transport, do we need to be good citizens and close the file handle when it times out?

There's code such as this being mentioned on php.net:

    while (!feof($sock) && !$info['timed_out']) {
         $file .= fgets($sock, 4096);
         $info = stream_get_meta_data($sock);
     }

     fclose($sock);

Do you have some form of reference for the / 2 timeout as well? It'd be useful to have something to point to.

[hakre]

See ​fclose().

See ​stream_get_meta_data() as well as ​stream_set_timeout()

When the stream times out, the 'timed_out' key of the array returned by stream_get_meta_data() is set to TRUE, although no error/warning is generated.

See ​HTTP context options:

timeout float

Read timeout in seconds, specified by a float (e.g. 10.5).
By default the default_socket_timeout php.ini setting is used.

Dividing by 2 makes not much sense to me, maybe sivel can say more. I've kept that in the refreshed patch.

Next to all this, not much left in patch.

[hakre]

Refreshed against trunk

[dd32]

I'm finding that both fsockopen and Streams ignore the timeouts, as well as curl in some cases

fsockopen: By adding the usage of stream_get_meta_data() I'm questioning it's compatibility, the Streams class should generally be used in preference to this to start wtih, so the compatibility I'm questioning is systems where Streams is not available and it ultimately falls back to FSockopen.

streams: the /2 does indeed seem to work as advertised, passing 4 in causes it to timeout after ~8s, with /2 it times out ~4seconds in.

curl: seems to have problems with file streaming and timeouts

[hakre]

stream_set_timeout()

[hakre]

stream_set_timeout()

[hakre]

Replying to dd32:

streams: the /2 does indeed seem to work as advertised, passing 4 in causes it to timeout after ~8s, with /2 it times out ~4seconds in.

Thanks for looking into this, I think this is really an interesting point worth to look more into. I've seen that it's made use of stream_set_timeout() in parallel just some lines later on. I started to review it (re-wrote those three lines as I found the calculation somewhat complicated, see example in patch) but could not finish so far. I'll try to test in isolation to find out if it's intended or a side-effect.

[ryan]

Punted per bug scrub.

[sivel]

Refreshed patch

[sivel]

I have refreshed the patch, so that we can do testing and get this in early in the 3.3 dev cycle.

[kirasong]

Fixed loop to allow fsockopen timeout to check properly

[kirasong]

Refreshed patch due to changes in 18633

[kirasong]

First pass at Unit Test - Only fails on fsockopen (and fixed by patch on ticket)

[kirasong]

First pass at unit test added to test current patch

[dd32]

13841.6.diff

That doesn't look like it closes/deletes/whatever the $stream_handle

Too late in the cycle for this though.

[kirasong]

Patch closes file handles and unsets response string before returning the error (since these would have both happened had the timeout not occurred).

I noticed shortly after uploading that there are a couple of whitespace changes needed for coding standards, but don't have a chance to update it at the moment -- should be able to do so tomorrow.

[kirasong]

Closes file handles and unsets variables appropriately before returning the timeout error.

[kirasong]

Updated Patch for coding standards RE: whitespace

[johnbillion]

Related: #18738

[kurtpayne]

Patch 13841.7.diff​ works for me. I was able to replicate the divide-by-two results, too. I went in a different direction with the unit test, though.

There's no need for a full fledged server, you just need a socket to ignore your request. The unit test code is longer, but it runs faster and there's no dependence on a third party server.

[kurtpayne]

Alternate unit test for http timeouts

[nacin]

Looks good, but had no discussion during 3.4. Pushing to 3.5.

[kirasong]

Close $handle as well.

[kirasong]

Haven't had a chance to update the unit test yet, but added an additional handle close in 13841.8.diff​ that was missing in the previous patch.

[kirasong]

Ported unit test to use phpunit

[kirasong]

Ported unit test to phpunit, in 13841-unit-test.2.diff​, but instead of the test failing gracefully without the proper patches, it just hangs, since the SimpleSocketServer is designed not to respond (ever).

[kurtpayne]

Replying to DH-Shredder:

Ported unit test to phpunit, in 13841-unit-test.2.diff​, but instead of the test failing gracefully without the proper patches, it just hangs, since the SimpleSocketServer is designed not to respond (ever).

Yep. If the test is running without the patch and WP_TESTS_FORCE_KNOWN_BUGS is true, then the tests will hang. I tried to get fancy here to avoid another external call when writing the test.

I can move this off to a local fork, or child process. It would probably be best to just move it to the new ​http://api.wordpress.org/core/tests site, though.

The test just needs a URL that will take "a while" (> 2 seconds) to respond so we can test the timeouts, but will eventually respond so the tests won't hang if the patch isn't available.

[thpani]

#26085 was marked as a duplicate.

[coolmann]

Any chances to see this added to WordPress 3.8.1? :)

[aaroncampbell]

Replying to coolmann:

Any chances to see this added to WordPress 3.8.1? :)

No, 3.8.1 will just be for regressions (things that worked in 3.7 and got broke in 3.8) and security fixes. Looks like this will be 3.9 at the soonest.

[coolmann]

Bringing this up again. It would be great to see this added to 3.9.x. Thank you.

[coolmann]

Thank you, Sergey.

[DrewAPicture]

Patch needs to be refreshed, and too late for this in 4.0. Punting.

[dd32]

Marking as invalid due to no traction in 6 years and we no longer use these transports since the switch to ​Requests. Requests may suffer from some of the same issues, and if so, should be fixed upstream.