Skip to content
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

Open
wants to merge 1 commit into
base: 5.x
Choose a base branch
from

Conversation

Nek-
Copy link

@Nek- Nek- commented Apr 26, 2024

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:

$this->httpClient = (new HttpClientBuilder)
    ->interceptNetwork(
        new ModifyRequest(static function (Request $request) {
            $request->setInactivityTimeout(500);

            return $request;
        })
    )
    ->build();

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.

@trowski
Copy link
Member

trowski commented May 10, 2024

Thanks! It looks like the test you added is failing due to a mis-matched message. Could you take a look at that?

@Nek- Nek- force-pushed the feature/add-new-inactivity-timeout-to-interceptors branch from 657c30d to d9dbf89 Compare May 14, 2024 12:39
@Nek-
Copy link
Author

Nek- commented May 14, 2024

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,
Copy link
Member

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean for BC?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed!

@Nek- Nek- force-pushed the feature/add-new-inactivity-timeout-to-interceptors branch from d9dbf89 to 369937b Compare May 21, 2024 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants