Add 'read_timeout' option #1611

arnaud-lb · 2016-10-05T11:45:21Z

This adds a 'read_timeout' option, allowing the user to specify a read timeout for reads on the body of streamed requests.

Support is added only to the StreamHandler, since only this handler seems to support stream requests.

This fixes #1516

jeskew

Could you add a test?

jeskew · 2016-10-06T20:53:01Z

src/Handler/StreamHandler.php

                $resource = fopen((string) $request->getUri()->withFragment(''), 'r', null, $context);
                $this->lastHeaders = $http_response_header;
+
+                if (isset($options['read_timeout'])) {


This could use the RequestOptions constant defined in this PR.

I agree :)

Other uses of the $options array in this class don't make use of RequestOptions constants, so I tried to keep it consistent.

Do you think I still should use the RequestOptions constant ?

You're right; it's better to keep this consistent.

jeskew · 2016-10-06T20:54:41Z

src/Handler/StreamHandler.php

+                if (isset($options['read_timeout'])) {
+                    $readTimeout = $options['read_timeout'];
+                    $sec = (int) $readTimeout;
+                    $usec = ($readTimeout - $sec) * 1000000;


Wouldn't this always be 0? If a user supplies a readTimeout of 1, then $usec will be set to (1 - 1) * 1000000.

If will be > 0 if read_timeout is not an integer:

$readTimeout =1.5; $sec = (int) $readTimeout; // 1 $usec = ($readTimeout - $sec) * 1000000; // (1.5 - 1) * 1000000 == 5000000

However there is one extra 0 here, I'll fix this.

OIC. I didn't realize that stream_set_timeout required seconds and fractions of seconds to be passed in as separate parameters.

arnaud-lb · 2016-10-07T14:33:27Z

@jeskew: Thank you for the review. I updated the PR accordingly.

jeskew · 2016-10-10T18:27:32Z

tests/Handler/StreamHandlerTest.php

+        $this->assertEquals("sleeping 60 seconds ...\n", $line);
+        $line = fgets($body);
+        $this->assertFalse($line);
+        $this->assertFalse(feof($body));


Since you're using the test server, this should explicitly check whether the stream timed out, e.g., $this->assertTrue(stream_get_meta_data($body)['timed_out']);

Ok, done :) Is this guaranteed to work in all cases ?

According to the PHP docs, this value will be true if the stream has timed out. They don't mention any known exceptions.

sagikazarmark

Thanks for the PR. Can you please clarify the few things I mentioned? Thank you very much

sagikazarmark · 2016-10-12T08:20:24Z

src/RequestOptions.php

@@ -238,6 +238,12 @@
    const TIMEOUT = 'timeout';

    /**
+     * read_timeout: (float, default=default_socket_timeout) Float describing


Maybe mention that it's configured in PHP ini?

sagikazarmark · 2016-10-12T08:22:03Z

tests/server.js

+        res.write("sleeping 60 seconds ...\n");
+        setTimeout(function () {
+          res.end("slept 60 seconds\n");
+        }, 10000);


Is this 60 seconds?

arnaud-lb · 2016-10-17T09:08:53Z

@sagikazarmark Thank you for the review, I've updated the PR accordingly

sagikazarmark · 2016-10-23T09:37:16Z

tests/server.js

+        res.write("sleeping 60 seconds ...\n");
+        setTimeout(function () {
+          res.end("slept 60 seconds\n");
+        }, 60*1000);


I am still not 100% sure what's happening here: it says 60 seconds, but in the test the read timeout is only one.

The server takes 60 seconds to send the end of the response, and the client has a 1 second read timeout. So the client times out while reading the response.

Ah, okay, thank

sagikazarmark

@arnaud-lb can you please add some documentation? Apart from that, ready to go.

arnaud-lb · 2016-11-09T17:01:38Z

@sagikazarmark: Awesome, added documentation

sagikazarmark · 2016-12-22T22:05:55Z

src/Handler/StreamHandler.php

+                    $readTimeout = $options['read_timeout'];
+                    $sec = (int) $readTimeout;
+                    $usec = ($readTimeout - $sec) * 100000;
+                    stream_set_timeout($resource, $sec, $usec);


To be honest I wonder if it would be better to pass the timeout to stream_context_create. It supports float values so it wouldn't require the magic above. One could even argue that the same functionality is already available via the undocumented stream_context option. WDYT @arnaud-lb ?

If it makes the code clearer, I'm all for it :) Which stream context option are you thinking about ? The "http.timeout" context option is used to implement Guzzle's "timeout" option, and I believe that it affects only the connection + reading the headers.

Hm, indeed, you are right, missed that.

sagikazarmark · 2017-02-23T20:32:24Z

Thanks for working on this, sorry for the long wait.

jeskew suggested changes Oct 6, 2016

View reviewed changes

arnaud-lb force-pushed the read-timeout branch from 18b9bf5 to f03ae42 Compare October 7, 2016 08:13

jeskew reviewed Oct 10, 2016

View reviewed changes

arnaud-lb force-pushed the read-timeout branch from 14bef88 to 55526fd Compare October 11, 2016 07:54

jeskew approved these changes Oct 11, 2016

View reviewed changes

sagikazarmark requested changes Oct 12, 2016

View reviewed changes

arnaud-lb force-pushed the read-timeout branch 2 times, most recently from a8e2b71 to be3ab76 Compare October 17, 2016 09:08

sagikazarmark approved these changes Oct 17, 2016

View reviewed changes

sagikazarmark requested changes Oct 23, 2016

View reviewed changes

sagikazarmark approved these changes Oct 26, 2016

View reviewed changes

sagikazarmark requested changes Nov 6, 2016

View reviewed changes

Add 'read_timeout' option

ed1d1bd

arnaud-lb force-pushed the read-timeout branch from be3ab76 to ed1d1bd Compare November 9, 2016 17:00

sagikazarmark approved these changes Nov 9, 2016

View reviewed changes

sagikazarmark reviewed Dec 22, 2016

View reviewed changes

sagikazarmark modified the milestone: 6.3.0 Jan 13, 2017

sagikazarmark merged commit 437c0c3 into guzzle:master Feb 23, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add 'read_timeout' option #1611

Add 'read_timeout' option #1611

arnaud-lb commented Oct 5, 2016 •

edited

Loading

jeskew left a comment

jeskew Oct 6, 2016

arnaud-lb Oct 7, 2016 •

edited

Loading

jeskew Oct 10, 2016

jeskew Oct 6, 2016

arnaud-lb Oct 7, 2016

jeskew Oct 10, 2016

arnaud-lb commented Oct 7, 2016

jeskew Oct 10, 2016

arnaud-lb Oct 11, 2016

jeskew Oct 11, 2016

sagikazarmark left a comment

sagikazarmark Oct 12, 2016

sagikazarmark Oct 12, 2016

arnaud-lb commented Oct 17, 2016

sagikazarmark Oct 23, 2016

arnaud-lb Oct 25, 2016 •

edited

Loading

sagikazarmark Oct 26, 2016

sagikazarmark left a comment

arnaud-lb commented Nov 9, 2016

sagikazarmark Dec 22, 2016

arnaud-lb Dec 23, 2016

sagikazarmark Dec 23, 2016

sagikazarmark commented Feb 23, 2017

Add 'read_timeout' option #1611

Add 'read_timeout' option #1611

Conversation

arnaud-lb commented Oct 5, 2016 • edited Loading

jeskew left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arnaud-lb Oct 7, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arnaud-lb commented Oct 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sagikazarmark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arnaud-lb commented Oct 17, 2016

Choose a reason for hiding this comment

arnaud-lb Oct 25, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sagikazarmark left a comment

Choose a reason for hiding this comment

arnaud-lb commented Nov 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sagikazarmark commented Feb 23, 2017

arnaud-lb commented Oct 5, 2016 •

edited

Loading

arnaud-lb Oct 7, 2016 •

edited

Loading

arnaud-lb Oct 25, 2016 •

edited

Loading