WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 5 months ago

#13841 new defect (bug)

Some HTTP Transports do not respect transfer timeouts

Reported by: sivel Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 3.0
Component: HTTP API Keywords: has-patch needs-testing 3.4-early
Focuses: Cc:

Description

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.

Attachments (14)

13841.diff (758 bytes) - added by sivel 4 years ago.
13841.2.diff (851 bytes) - added by sivel 3 years ago.
Refreshed patch
13841.3.diff (1.6 KB) - added by aaroncampbell 3 years ago.
Re-refreshed patch
13841.patch (1.2 KB) - added by hakre 3 years ago.
Refreshed against trunk
13841.2.patch (822 bytes) - added by hakre 3 years ago.
stream_set_timeout()
13841.3.patch (822 bytes) - added by hakre 3 years ago.
stream_set_timeout()
13841.4.diff (1.8 KB) - added by sivel 3 years ago.
Refreshed patch
13841.5.diff (1.9 KB) - added by DH-Shredder 3 years ago.
Fixed loop to allow fsockopen timeout to check properly
13841.6.diff (1.3 KB) - added by DH-Shredder 3 years ago.
Refreshed patch due to changes in 18633
13841.unitTest.diff (1.7 KB) - added by DH-Shredder 3 years ago.
First pass at Unit Test - Only fails on fsockopen (and fixed by patch on ticket)
13841.7.diff (1.6 KB) - added by DH-Shredder 2 years ago.
Closes file handles and unsets variables appropriately before returning the timeout error.
13841-unit-test.diff (2.2 KB) - added by kurtpayne 2 years ago.
Alternate unit test for http timeouts
13841.8.diff (1.6 KB) - added by DH-Shredder 21 months ago.
Close $handle as well.
13841-unit-test.2.diff (2.0 KB) - added by DH-Shredder 21 months ago.
Ported unit test to use phpunit

Download all attachments as: .zip

Change History (48)

sivel4 years ago

comment:1 sivel4 years ago

  • Keywords has-patch dev-feedback early added

comment:2 sivel4 years ago

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.

comment:3 dd324 years ago

See #11468 and [12473] for related timeout issues

comment:4 jacobsantos4 years ago

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.

comment:5 sivel4 years ago

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

comment:6 sivel4 years ago

  • Version changed from 3.0 to 3.1

comment:7 sivel4 years ago

  • Milestone changed from Awaiting Triage to 3.1
  • Version changed from 3.1 to 3.0

comment:8 nacin3 years ago

  • Milestone changed from 3.1 to Future Release

sivel3 years ago

Refreshed patch

comment:9 sivel3 years ago

  • Keywords 3.2-early added; dev-feedback early removed

comment:10 sivel3 years ago

  • Keywords needs-refresh added
  • Milestone changed from Future Release to 3.2

aaroncampbell3 years ago

Re-refreshed patch

comment:11 aaroncampbell3 years ago

  • Keywords 3.2-early needs-refresh removed

Same patch, just refreshed for current trunk.

comment:12 sivel3 years ago

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.

comment:13 dd323 years ago

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.

comment:14 hakre3 years ago

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.

Last edited 3 years ago by hakre (previous) (diff)

hakre3 years ago

Refreshed against trunk

comment:15 follow-up: dd323 years ago

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

hakre3 years ago

stream_set_timeout()

hakre3 years ago

stream_set_timeout()

comment:16 in reply to: ↑ 15 hakre3 years ago

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.

comment:17 ryan3 years ago

  • Milestone changed from 3.2 to Future Release

Punted per bug scrub.

sivel3 years ago

Refreshed patch

comment:18 sivel3 years ago

  • Keywords needs-testing added

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

comment:19 sivel3 years ago

  • Milestone changed from Future Release to 3.3

DH-Shredder3 years ago

Fixed loop to allow fsockopen timeout to check properly

comment:20 DH-Shredder3 years ago

  • Cc mike.schroder@… added

DH-Shredder3 years ago

Refreshed patch due to changes in 18633

DH-Shredder3 years ago

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

comment:21 DH-Shredder3 years ago

First pass at unit test added to test current patch

comment:22 dd322 years ago

  • Keywords 3.4-early added
  • Milestone changed from 3.3 to Future Release

13841.6.diff

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

Too late in the cycle for this though.

comment:23 DH-Shredder2 years ago

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.

DH-Shredder2 years ago

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

comment:24 DH-Shredder2 years ago

Updated Patch for coding standards RE: whitespace

comment:26 sivel2 years ago

  • Milestone changed from Future Release to 3.4

comment:27 kurtpayne2 years ago

  • Cc kpayne@… added

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.

kurtpayne2 years ago

Alternate unit test for http timeouts

comment:28 nacin2 years ago

  • Milestone changed from 3.4 to Future Release

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

DH-Shredder21 months ago

Close $handle as well.

comment:29 DH-Shredder21 months ago

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.

DH-Shredder21 months ago

Ported unit test to use phpunit

comment:30 follow-up: DH-Shredder21 months ago

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

comment:31 in reply to: ↑ 30 kurtpayne21 months ago

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.

comment:32 thpani5 months ago

#26085 was marked as a duplicate.

comment:33 follow-up: coolmann5 months ago

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

comment:34 in reply to: ↑ 33 aaroncampbell5 months ago

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.

Note: See TracTickets for help on using tickets.