Add 'read_timeout' option by arnaud-lb · Pull Request #1611 · guzzle/guzzle

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

                $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

+                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

+        $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

     */
    const TIMEOUT = 'timeout';

+    /**


Maybe mention that it's configured in PHP ini?

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

+        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

+        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

+                    $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

Uh oh!

Conversation

arnaud-lb commented Oct 5, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jeskew left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arnaud-lb Oct 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arnaud-lb commented Oct 7, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sagikazarmark left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arnaud-lb commented Oct 17, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arnaud-lb Oct 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sagikazarmark left a comment

Choose a reason for hiding this comment

Uh oh!

arnaud-lb commented Nov 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sagikazarmark commented Feb 23, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

arnaud-lb commented Oct 5, 2016 •

edited

Loading

arnaud-lb Oct 7, 2016 •

edited

Loading

arnaud-lb Oct 25, 2016 •

edited

Loading