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

Adding retry logic and connection pooling may improve stability #41

Open
tauheedali opened this issue Dec 8, 2024 · 4 comments
Open

Comments

@tauheedali
Copy link

Hey! I've been using this package for the past month or so and it's helped me hit the ground running.

I've noticed at times there are http timeout errors (likely my logic) and part of my strategy right now is to wait and retry a set number of times with exponential backoff.

To that end, it may be useful if the client had retry logic for timeout errors built in so that users don't need to manage that themselves. At least for GET requests.

Also, how can I contribute to this project?

@tylerebowers
Copy link
Owner

I have considered this but I want to maintain a line of seperation between what the user implements and what is handled by the package. I.e. in this case a user may want to do something else if the call fails, e.g. if their internet is down then this call could repeat infinitely or not retry enough times. Also, you can add the parameter timeout=n where n is an int to adjust the timeout of api calls.

And yes, I do plan on using session requests instead of the current method, however I am currently very busy this week. I intend on releasing an update that implements this before the end of this month.

@tylerebowers
Copy link
Owner

After doing quite a bit of research it appears that sessions are not thread safe. Since a single client could be shared across threads it might not be a good idea to use one session across requests. I also don't know how stable they are, I considered making a new session every time a new access token is granted.

@tauheedali
Copy link
Author

Yeah, that is something important to consider. If you still wanted allow users to leverage connection pooling you could probably take a requests.session as an optional constructor argument and use the session if it exists. You might be able also use httpx to create a thread safe session, but that is probably a larger refactor from what you have now.

@ScottyB55
Copy link

Hi Tauheed and Tyler,

I enjoyed reading this discussion—both of you bring great points! Tauheed, the idea of passing a requests.session as an optional argument is a smart way to add flexibility, and Tyler, I appreciate the focus on keeping user logic separate from the package.

I’d love to collaborate and help contribute to the project. If either of you is open to a quick call to brainstorm or discuss next steps, feel free to reach out to me!

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

No branches or pull requests

3 participants