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

add request session object #45

Closed
wants to merge 5 commits into from
Closed

add request session object #45

wants to merge 5 commits into from

Conversation

wallies
Copy link

@wallies wallies commented Aug 4, 2021

Fixes #44 and #30

@rcoup
Copy link

rcoup commented Aug 4, 2021

IMO this should default to off/disabled with a sensible timeout with no retries — I don't want a webserver sitting there doing try after try to Chargebee with a client waiting for it. The application needs to handle Chargebee errors regardless, so it doesn't simplify logic in the application at all.

Should probably handle 429 errors from hitting the rate limits as well?

Not sure if Chargebee currently supports idempotency keys ala Stripe but that would allow retries for POST/PUT/DELETE requests too.

@wallies
Copy link
Author

wallies commented Aug 4, 2021

@rcoup ive disabled retries by default, but imho retries with backoff should be default behavior. It should be enabled by default in requests, to give as many people as possible this functionality, and then people who dont want it can turn it off. Network is never reliable enough to not have something like this and for me it greatly simplifies my application.

@cjrh
Copy link

cjrh commented Aug 4, 2021

Another idea here is to add a new kwarg with a user-specified requests Session object, defaulting to None. Then only this part (I think) of the existing code needs to change:

def request(method, url, env, params=None, headers=None, session=None):
    ...
    if session:
        response = session.request(**request_args)
    else:
        response = requests.request(**request_args)

This way users can set up the retry facility, as well as anything else that sessions allow, however they want.

As a side effect, this also makes testing a bit easier because of the dependency injection. You wouldn't need to mock requests implicitly.

@wallies wallies changed the title add timeout and retry add request session object Aug 4, 2021
@cb-rakesh cb-rakesh requested a review from cb-goutham August 11, 2021 14:59
@wallies
Copy link
Author

wallies commented Oct 21, 2021

@cb-rakesh @cb-goutham how do we get this merged, as we have had quite a few

Remote end closed connection and connection reset by peer lately, which we can deal with a lot easier having the request session available.

@cjrh
Copy link

cjrh commented Oct 22, 2021

The PR looks good 👍🏼

@ashwini-balnaves
Copy link

I would also really like to merge this PR. I'd like to be able to configure the retry behavior.

@Aiden0
Copy link

Aiden0 commented Oct 22, 2021

I too would like this PR to be merged please.

@ashwini-balnaves
Copy link

ashwini-balnaves commented Oct 26, 2021

We got more 500s overnight. Please can we expose this so we can configure the retry behavior? 🙏

@wallies
Copy link
Author

wallies commented Nov 3, 2021

We have had so many more 504 & 500 errors recently, we decided to upload the fork to pypi for now https://pypi.org/project/secure-chargebee/

@cb-prajaktachavan
Copy link
Contributor

cb-prajaktachavan commented Dec 10, 2021

You can set own http connect and read timeouts using chargebee.update_connect_timeout_secs() and chargebee.update_read_timeout_secs().

@wallies wallies closed this by deleting the head repository Aug 18, 2022
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.

Retry, Timeouts & Backoff support
6 participants