Skip to content

AmpHttpClient missing a new timeout function call #36911

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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
richard4339 opened this issue May 22, 2020 · 3 comments
Closed

AmpHttpClient missing a new timeout function call #36911

richard4339 opened this issue May 22, 2020 · 3 comments

Comments

@richard4339
Copy link

Symfony version(s) affected: 5.1.0-RC1

Description
AmpHttpClient's request method sets these timeout values:

$request->setTcpConnectTimeout(1000 * $options['timeout']);
$request->setTlsHandshakeTimeout(1000 * $options['timeout']);
$request->setTransferTimeout(1000 * $options['max_duration']);

However, the amphp/http-client module introduced a fourth timeout in version 4.3.0 released on May 3rd, an inactivity timeout, as seen here: amphp/http-client#263

How to reproduce
Make a request and sleep for 11 seconds before calling getContent().

Possible Solution
Adding $request->setInactivityTimeout(1000 * $options['max_duration']); works, but I'm not sure that is the appropriate $options value to use (timeout would make sense, as would a non-existent new option).
Another temporary option could be to restrict the package to version 4.2.2 when Symfony 5.1 is released?

@nicolas-grekas
Copy link
Member

nicolas-grekas commented May 23, 2020

Just to confirm, did you actually run into any issue related to this?
Because the code already handles inactivity timeouts to me, this new method is not needed to implement them.

If yes, can you please provide a full reproducer? E.g. one using httpbin.org

@richard4339
Copy link
Author

richard4339 commented May 31, 2020

I’ll work on a simple reproducer, it’s difficult to provide on the api I was using because of oauth requirements.

Gist of the problem was making 1000 request calls in an array and getting rate limited and needing to sleep for about 30 seconds 800 requests in. Next code call is a new for loop with a getContents() call. Because of the sleep, by the time it got to the calls right before the sleep, more then ten seconds passed, and the new Amp inactivity timeout kicked in. Reverting from 4.3 to 4.2.2 that didn’t have that new baked in inactivity timeout of ten seconds resolved the problem. My point in this issue is that I think that Amp’s inactivity timeout is overriding the Symfony one, and because I can’t manually touch the actual Amp client, I can’t manually call their new method or pass an option through to touch it.

And don’t get me wrong, I may be using a bad pattern here too.

@nicolas-grekas
Copy link
Member

Thanks for the useful details.

FYI, Symfony 5.2 will provide support for pausing without calling sleep, see #37136

About inactivity timeouts, the issue is that there is now a default value set to 10s.

Would you mind sending a PR (branch 5.1) to disable this timeout by setting it to 0 when the method exists?

@fabpot fabpot closed this as completed Jun 11, 2020
fabpot added a commit that referenced this issue Jun 11, 2020
…h it on our own already (nicolas-grekas)

This PR was merged into the 5.1 branch.

Discussion
----------

[HttpClient] disable AMP's inactivity timeout, we deal with it on our own already

| Q             | A
| ------------- | ---
| Branch?       | 5.1
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #36911
| License       | MIT
| Doc PR        | -

I encountered this today also.

Commits
-------

a7b18ff [HttpClient] disable AMP's inactivity timeout, we deal with it on our own already
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants