-
Notifications
You must be signed in to change notification settings - Fork 32
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
Conversation
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 Not sure if Chargebee currently supports idempotency keys ala Stripe but that would allow retries for POST/PUT/DELETE requests too. |
@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. |
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. |
@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. |
The PR looks good 👍🏼 |
I would also really like to merge this PR. I'd like to be able to configure the retry behavior. |
I too would like this PR to be merged please. |
We got more 500s overnight. Please can we expose this so we can configure the retry behavior? 🙏 |
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/ |
You can set own http connect and read timeouts using chargebee.update_connect_timeout_secs() and chargebee.update_read_timeout_secs(). |
Fixes #44 and #30