-
Notifications
You must be signed in to change notification settings - Fork 107
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
Handles closed db connections #1747
Conversation
3e86ce1
to
ab1bfff
Compare
Attached issue: https://pulp.plan.io/issues/9598 |
@@ -0,0 +1 @@ | |||
Fixed bug where the content app would stop working after a brief loss of connection to the database. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it be a backport? Or is the fix on main completely different?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a backport. I did not use the cherry-pick script. In addition to this backport, I had to backport a few other commits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This updated version is what I expected. Thanks!
This PR is made against a stable branch. So i expect the commits to be generated by the cherry-pick-script, which will clearly identify what has been cherry-picked, what was the original issue and what backport issue is fixed. |
I think we do want the clarity the backport script provides, but you can also do these commands manually to ammend your commit to look like what it would produce. |
fe3a78a
to
d60c1fd
Compare
[noissue] (cherry picked from commit d0dbc42)
When the authentication middleware was added in pulpcore 3.15, it became the first place in the content app that made an attempt to use the database. As a result, it is a convinient place to handle InterfaceError and OperationalError which are raised when the database connection has been closed. When this occurs, Handler._reset_db_connection() is called to clean up the database connection and the middleware tries to use the database again. If the database connection is closed later in the handling of the request by the content app, the user will still get a 500 error. However, the next request will be handled properly. This patch also adds a call to Handler._reset_db_connection() inside the heartbeat method. backports #9276 fixes: #9598 https://pulp.plan.io/issues/9598 (cherry picked from commit 3faa649)
[noissue]
SimpleTestCase does not allow database access. [noissue] (cherry picked from commit 342587a)
[noissue] (cherry picked from commit 08dd0ee)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great. Having done some backports recently, each of these changes looks right to me. Thanks! 🥇
When the authentication middleware was added in pulpcore 3.15, it became the first place
in the content app that made an attempt to use the database. As a result, it is a convinient
place to handle InterfaceError and OperationalError which are raised when the database
connection has been closed. When this occurs, Handler._reset_db_connection() is called to
clean up the database connection and the middleware tries to use the database again.
If the database connection is closed later in the handling of the request by the content app,
the user will still get a 500 error. However, the next request will be handled properly.
This patch also adds a call to Handler._reset_db_connection() inside the heartbeat method.
fixes: #9276
https://pulp.plan.io/issues/9276
Please be sure you have read our documentation on creating PRs:
https://docs.pulpproject.org/contributing/pull-request-walkthrough.html