Transfer & connect timeouts, in seconds & milliseconds #97
Conversation
|
A few more or less related comments:
|
| @@ -198,7 +198,10 @@ public function request($url, $headers = array(), $data = array(), $options = ar | |||
| $options['hooks']->dispatch('fsockopen.after_request', array(&$fake_headers)); | |||
| return ''; | |||
| } | |||
| stream_set_timeout($fp, $options['timeout']); | |||
|
|
|||
| $timeout_sec = (int) floor( $options['timeout'] ); | |||
There was a problem hiding this comment.
Indentation looks wrong here?
|
Thanks! 🎆
Going to need tests :)
I meant to fix HHVM support properly but didn't get around to it. Whoops. (See: #93)
1.6.x is bug fixes only, but this is certainly good for 1.7. |
|
Release the Kraken ! Or, well, just 1.7, that'd be fine too. |
| stream_set_timeout($fp, $options['timeout']); | ||
|
|
||
| $timeout_sec = (int) floor( $options['timeout'] ); | ||
| $timeout_msec = $timeout_sec == $options['timeout'] ? 0 : 1000000 * $options['timeout'] % 1000000; |
There was a problem hiding this comment.
What does this number represent? We should move it to a class constant instead.
There was a problem hiding this comment.
It's just a million to compute the microsecond part of the timeout...
$options['timeout'] = 2.15;
$timeout_sec = (int) floor( $options['timeout'] );
$timeout_msec = $timeout_sec == $options['timeout'] ? 0 : 1000000 * $options['timeout'] % 1000000;
var_dump( $timeout_sec, $timeout_msec ); // "int 2" and "int 150000"Not sure there's a point in using a class const, I think a comment would be better then :) I'm always reluctant to add comments in Request, as there is very few of them in here
There was a problem hiding this comment.
For the most part I try and make it self-explanatory, but the issue here is that it's easy to miss a 0. A class constant ensures that there's no error usually. (Something like SECOND_IN_MICROSECONDS would work well here, IMO)
There was a problem hiding this comment.
Okay -- const in Requests.php or, since it's only used in fsockopen, in Transport/fsockopen.php ?
There was a problem hiding this comment.
Class constant for Requests_Transport_fsockopen, since it's only used there. :)
|
Thanks! I'll merge once the small coding standards bits are fixed. 🍝 |
|
✨ Thanks for that! |
Transfer & connect timeouts, in seconds & milliseconds
|
Neat, thanks a lot. I needed this for the next iteration of YOURLS. (By the way, if you haven't seen it already, I did some pimping for Requests on YOURLS blog recently) Next on your agenda: hhvm tests in their own branch, and tagging + release :) |
This PR supersedes #58 & #83
timeoutandconnect_timeout'timeout' => 3or'timeout' => 3.55)I've added a cool example also.
I didn't add tests, as that would just slow down things, but it's possible with the same URL used in the example