-
-
Notifications
You must be signed in to change notification settings - Fork 65
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add inactivity timeout to SetRequestTimeout interceptor #362
base: 5.x
Are you sure you want to change the base?
Add inactivity timeout to SetRequestTimeout interceptor #362
Conversation
Thanks! It looks like the test you added is failing due to a mis-matched message. Could you take a look at that? |
657c30d
to
d9dbf89
Compare
Oops. Sorry, it was late! 馃槀 Fixed it! 馃檹 |
@@ -10,15 +10,18 @@ public function __construct( | |||
float $tcpConnectTimeout = 10, | |||
float $tlsHandshakeTimeout = 10, | |||
float $transferTimeout = 10, | |||
float $inactivityTimeout = 10, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this default to null
to prevent modifying the inactivity timeout if there's already a custom timeout present?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean for BC?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed!
d9dbf89
to
369937b
Compare
I figured out that the inactivity timeout was not present in the default interceptor in charge of updating the timeouts.
Of course it's not a big deal and here is a workaround:
But I decided maybe it's a good first contribution here 馃槃 . I also updated a bit the documentation because it was missing in the interceptor list! However, I'm sorry I don't know how to fix the test.