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

Already on GitHub? Sign in to your account

feat: Add timeouts for Client::reqwest calls [SYNC-3289] #329

Merged
merged 1 commit into from
Aug 23, 2022

Conversation

jrconlin
Copy link
Member

These are used for routing and the like.
Currently default for:

// Initial connection timeout
connection_timeout_millis: 1000
// Total request timeout
request_timeout_millis: 3000

Closes: #CONSVC-1632

These are used for routing and the like.
Currently default for:

```
// Initial connection timeout
connection_timeout_millis: 1000
// Total request timeout
request_timeout_millis: 3000
```
Closes: #CONSVC-1632
@jrconlin jrconlin requested review from a team August 23, 2022 18:03
Copy link
Member

@pjenvey pjenvey left a comment

Choose a reason for hiding this comment

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

👍 but unfortunately this doesn't help the APNS router because a2 uses its own hyper client internally with currently no way to set timeouts

@jrconlin
Copy link
Member Author

Yep, working on a separate patch for a2 to include passing a custom ClientBuilder, which can have timeouts.

@jrconlin jrconlin merged commit e0c370a into master Aug 23, 2022
@jrconlin jrconlin deleted the 3289-timeouts branch August 23, 2022 19:45
@jrconlin jrconlin changed the title feat: Add timeouts for Client::reqwest calls [CONSVC-3289] feat: Add timeouts for Client::reqwest calls [SYNC-3289] Aug 24, 2022
@jrconlin
Copy link
Member Author

Follow up:
It does not appear that adding timeouts to the a2 client code is a straightforward thing. a2 calls hyper directly to create a pool of http2 connections. hyper requires the calling library to create any timeouts. This would require passing a tokio compatible handle down to a2 in order to create a proper timeout handler, however the version of actix that endpoint is using does not readily disclose the tokio handle, at least not without doing a lot of extra work. (There may be an easier solution that I'm not seeing, but later versions of actix do expose the handle, so I'm guessing that it was a future feature request.) Updating actix is desirable, but out of the scope of this PR.

@pjenvey
Copy link
Member

pjenvey commented Aug 24, 2022

@jrconlin Looks like it would be easier to move a2 to reqwest if the maintainer's are up for it. Only reason I can think they wouldn't is if it adds more dependency bloat to a2.

@jrconlin jrconlin mentioned this pull request Jan 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants