Added Timeout-Promise to Transaction #90
Conversation
|
@Rakdar Thank you for taking the time to file this PR! I've just posted a small recap for this feature in #1 (comment), what do you think about this? I'm currently undecided if we can apply a "default timeout" here and if so, what its timeout value should be. Any input is welcome! 👍 |
src/Io/Transaction.php
Outdated
| public function __construct(RequestInterface $request, Sender $sender, array $options = array(), MessageFactory $messageFactory) | ||
| { | ||
| /** @var int $timeout */ | ||
| private $timeout = 10; |
There was a problem hiding this comment.
Not sure about this default value, how about using http://php.net/manual/en/filesystem.configuration.php#ini.default-socket-timeout instead? (defaults to 60s)
There was a problem hiding this comment.
Yes, you're right. Using the ini-default-value as default will be better. (I personally just don't like a 60 seconds timeout; what server takes 60 seconds to send the headers? ... in 2018?)
src/Io/Transaction.php
Outdated
| function (ResponseInterface $response) use ($request, $that) { | ||
| return $that->onResponse($response, $request); | ||
| } | ||
| return Promise\Timer\timeout( |
There was a problem hiding this comment.
This only times out a single request within this "transaction". In other words: This timeout will be restarted for each redirection and the consumer has no control over applying a total timeout.
It's my understanding that this timeout should apply to the whole "transaction", i.e. simply move this up to the send() method instead of the next() method.
What do you think about this?
There was a problem hiding this comment.
Now i don't get it myself why i implemented the timeout within the next() method.
- Moving timeout-promise one method up to send() instead of next().
2 participants
In relation to #1, i implemented a promise-timer to support timeouts.
The default is 10 seconds and the API implemented as suggested: