WordPress.org

Make WordPress Core

Opened 5 years ago

Last modified 12 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 4.1-early needs-refresh
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 5 years ago.
13841.2.diff (851 bytes) - added by sivel 4 years ago.
Refreshed patch
13841.3.diff (1.6 KB) - added by aaroncampbell 4 years ago.
Re-refreshed patch
13841.patch (1.2 KB) - added by hakre 4 years ago.
Refreshed against trunk
13841.2.patch (822 bytes) - added by hakre 4 years ago.
stream_set_timeout()
13841.3.patch (822 bytes) - added by hakre 4 years ago.
stream_set_timeout()
13841.4.diff (1.8 KB) - added by sivel 4 years ago.
Refreshed patch
13841.5.diff (1.9 KB) - added by DH-Shredder 4 years ago.
Fixed loop to allow fsockopen timeout to check properly
13841.6.diff (1.3 KB) - added by DH-Shredder 4 years ago.
Refreshed patch due to changes in 18633
13841.unitTest.diff (1.7 KB) - added by DH-Shredder 4 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 4 years ago.
Closes file handles and unsets variables appropriately before returning the timeout error.
13841-unit-test.diff (2.2 KB) - added by kurtpayne 3 years ago.
Alternate unit test for http timeouts
13841.8.diff (1.6 KB) - added by DH-Shredder 3 years ago.
Close $handle as well.
13841-unit-test.2.diff (2.0 KB) - added by DH-Shredder 3 years ago.
Ported unit test to use phpunit

Download all attachments as: .zip

Change History (52)

@sivel5 years ago

comment:1 @sivel5 years ago

  • Keywords has-patch dev-feedback early added

comment:2 @sivel5 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 @dd325 years ago

See #11468 and [12473] for related timeout issues

comment:4 @jacobsantos5 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 @sivel5 years ago

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

comment:6 @sivel5 years ago

  • Version changed from 3.0 to 3.1

comment:7 @sivel5 years ago

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

comment:8 @nacin5 years ago

  • Milestone changed from 3.1 to Future Release

@sivel4 years ago

Refreshed patch

comment:9 @sivel4 years ago

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

comment:10 @sivel4 years ago

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

@aaroncampbell4 years ago

Re-refreshed patch

comment:11 @aaroncampbell4 years ago

  • Keywords 3.2-early needs-refresh removed

Same patch, just refreshed for current trunk.

comment:12 @sivel4 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 @dd324 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 @hakre4 years ago

See fclose().

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

Version 0, edited 4 years ago by hakre (next)

@hakre4 years ago

Refreshed against trunk

comment:15 follow-up: @dd324 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

@hakre4 years ago

stream_set_timeout()

@hakre4 years ago

stream_set_timeout()

comment:16 in reply to: ↑ 15 @hakre4 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 @ryan4 years ago

  • Milestone changed from 3.2 to Future Release

Punted per bug scrub.

@sivel4 years ago

Refreshed patch

comment:18 @sivel4 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 @sivel4 years ago

  • Milestone changed from Future Release to 3.3

@DH-Shredder4 years ago

Fixed loop to allow fsockopen timeout to check properly

comment:20 @DH-Shredder4 years ago

  • Cc mike.schroder@… added

@DH-Shredder4 years ago

Refreshed patch due to changes in 18633

@DH-Shredder4 years ago

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

comment:21 @DH-Shredder4 years ago

First pass at unit test added to test current patch

comment:22 @dd324 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-Shredder4 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-Shredder4 years ago

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

comment:24 @DH-Shredder4 years ago

Updated Patch for coding standards RE: whitespace

comment:26 @sivel3 years ago

  • Milestone changed from Future Release to 3.4

comment:27 @kurtpayne3 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.

@kurtpayne3 years ago

Alternate unit test for http timeouts

comment:28 @nacin3 years ago

  • Milestone changed from 3.4 to Future Release

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

@DH-Shredder3 years ago

Close $handle as well.

comment:29 @DH-Shredder3 years 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-Shredder3 years ago

Ported unit test to use phpunit

comment:30 follow-up: @DH-Shredder3 years 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 @kurtpayne3 years 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 @thpani20 months ago

#26085 was marked as a duplicate.

comment:33 follow-up: @coolmann19 months ago

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

comment:34 in reply to: ↑ 33 @aaroncampbell19 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.

comment:35 @coolmann14 months ago

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

comment:36 @SergeyBiryukov14 months ago

  • Milestone changed from Future Release to 4.0

comment:37 @coolmann14 months ago

Thank you, Sergey.

comment:38 @DrewAPicture12 months ago

  • Keywords 4.1-early needs-refresh added; 3.4-early removed
  • Milestone changed from 4.0 to Future Release

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

Note: See TracTickets for help on using tickets.