fix: reduce request transaction lifetime #3299
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dnephin
requested review from
ssoroka,
BruceMacD,
jmorganca and
pdevine
as code owners
dnephin
commented
Sep 23, 2022
| @@ -86,6 +92,7 @@ func authenticateRequest(c *gin.Context, tx *data.Transaction, srv *Server) erro | |||
| return internal.ErrBadRequest | |||
| } | |||
|
|
|||
| // TODO: move to caller | |||
There was a problem hiding this comment.
Now that the transaction is separate, the only thing these two functions provide is the access.Authenticated struct. So I think we can return that directly instead of mutating the gin.Context.
I'm going to look to address these TODOs in a follow up PR. It requires a few changes to handleInfraDestinationHeader.
ssoroka
approved these changes
Sep 23, 2022
| @@ -75,7 +75,13 @@ func handleInfraDestinationHeader(c *gin.Context) error { | |||
| // gin.Context. | |||
| // See validateRequestOrganization for a related function used for unauthenticated | |||
| // routes. | |||
| func authenticateRequest(c *gin.Context, tx *data.Transaction, srv *Server) error { | |||
| func authenticateRequest(c *gin.Context, srv *Server) error { | |||
| tx, err := srv.db.Begin(c.Request.Context()) | |||
There was a problem hiding this comment.
this will still work without explicitly starting a transaction, right?
There was a problem hiding this comment.
Ah yes, I guess that is true. I haven't looked at all the queries, but it might be ok without an explicit transaction.
There was a problem hiding this comment.
Do you think there is any advantage to not using a transaction? I thought generally we would want to put things into short lived transactions.
| @@ -131,7 +142,13 @@ func validateOrgMatchesRequest(req *http.Request, tx data.GormTxn, accessKeyOrg | |||
| // | |||
| // validateRequestOrganization is also responsible for adding RequestContext to the | |||
| // gin.Context. | |||
| func validateRequestOrganization(c *gin.Context, tx *data.Transaction, srv *Server) error { | |||
| func validateRequestOrganization(c *gin.Context, srv *Server) error { | |||
| tx, err := srv.db.Begin(c.Request.Context()) | |||
PostgreSQL row and table locks are held until the end of the transaction. Previously we were using a single transaction for the entire lifetime of the request. One of the first things we did with that transaction was update the access key and user record associated with the request. That update would block any other requests from that user until the first request finished. For short lived requests that was fine, but longer requests (anything making HTTP requests to an IDP, or blocking and waiting for updates) would prevent the user from making concurrent requests. This commit fixes the problem by committing the "middleware" transaction first, then starting a new transaction for the request handler. The middleware transaction should be short lived, because does only a few database operations and returns. It doesn't block or make external requests.
dnephin
force-pushed
the
dnephin/api-separate-transaction-for-middleware
branch
from
b697a95
to
ee92dec
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PostgreSQL row and table locks are held until the end of the transaction.
Previously we were using a single transaction for the entire lifetime of the request. One of the first things we did with that transaction was update the access key and user record associated with the request.
That update would block any other requests from that user until the first request finished. For short lived requests that was fine, but longer requests (anything making HTTP requests to an IDP, or blocking and waiting for updates) would prevent the user from making concurrent requests.
This commit fixes the problem by committing the "middleware" transaction first, then starting a new transaction for the request handler. The middleware transaction should be short lived, because it only does a few database operations and returns. It doesn't block or make external requests.