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

Fix operation error #32

Merged
merged 6 commits into from
May 22, 2019
Merged

Fix operation error #32

merged 6 commits into from
May 22, 2019

Conversation

ebreton
Copy link
Contributor

@ebreton ebreton commented May 21, 2019

Details of the bug can be found in issue #31

The solution makes use of the 'pessimistic' approach as described in https://docs.sqlalchemy.org/en/13/core/pooling.html#pool-disconnects

The approach adds a small bit of overhead to the connection checkout process, however is otherwise the most simple and reliable approach to completely eliminating database errors due to stale pooled connections. The calling application does not need to be concerned about organizing operations to be able to recover from stale connections checked out from the pool

@tiangolo
Copy link
Member

Thanks! That looks like a small change with a long time debugging and investigating 🤓

Were you able to test it? Did it solve the problem?

@ebreton
Copy link
Contributor Author

ebreton commented May 21, 2019

Hi @tiangolo , I do not have a reproductible test, yet here is what I have done:

  • before fix, restart the db container, and query the API backend -> 500 errors come at once, in numbers

  • after fix, restart the db container with same conditions -> there is a few-seconds lag in the first query (probably timeout and/or retrials from the connection pool), then a couple of 500 errors before the DB is up again

I have also checked the logs, and there are no more OperationalErrors after applying the fix (I will check again during the week)

From my point of view, the issue is fixed, but not automatically tested. Which does not really concern me, since the management of the connection cool is on the SQLAlchemy level, and therefore tested by them

@tiangolo tiangolo merged commit 1702317 into fastapi:master May 22, 2019
@tiangolo
Copy link
Member

Cool!

Thanks for the explanation and the local tests you did. That's good enough ✔️

Thanks for your work! 🎉 🚀 🍰

gusevyaroslove pushed a commit to gusevyaroslove/fastapi-template that referenced this pull request Aug 4, 2024
alejsdev pushed a commit that referenced this pull request Dec 19, 2024
Co-authored-by: Sebastián Ramírez <tiangolo@gmail.com>
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