Run session token renewals in a database transaction #34379
Merged
Conversation
This file contains hidden or 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
ChristophWurst
requested review from
AndyScherzinger,
CarlSchwan,
PVince81,
juliusknorr and
nickvergessen
rullzer
approved these changes
Oct 2, 2022
st3iny
approved these changes
Oct 11, 2022
ChristophWurst
added
4. to release
AndyScherzinger
approved these changes
Oct 11, 2022
|
@ChristophWurst please fix for php-cs |
The session token renewal does 1) Read the old token 2) Write a new token 3) Delete the old token If two processes succeed to read the old token there can be two new tokens because the queries were not run in a transaction. This is particularly problematic on clustered DBs where 1) would go to a read node and 2) and 3) go to a write node. Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
ChristophWurst
force-pushed
the
fix/transactional-session-token-renewal
branch
from
c721b4e to
c5922e6
Compare
|
@ChristophWurst @PVince81 how far back would we want to backport this? Asking since this might help with some large scale installations but might also hurt some in cases were long(er) running transactions might cause issues with raising the probability of deadlocks in a cluster setup. |
|
The used API exists since 24 |
|
Wrapping these related queries in a query is one of the proposed solution we received from the mariadb experts. Write and delete will always run on the main database node. The transaction will only cause the read to happen on the same. I think this should be fine. |
|
@ChristophWurst can you cover a backport to 24+ then? Thanks in advance 🙏 |
|
/backport to stable25 |
|
/backport to stable24 |
This was referenced Nov 3, 2022
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.
The session token renewal does
If two processes succeed to read the old token there can be two new tokens because the queries were not run in a transaction. This is particularly problematic on clustered DBs where 1) would go to a read node and 2) and 3) go to a write node.
Wrapping this code in a transaction makes sure that it all goes through or the insert/delete never happen, as it should.
This problem came up in a discussion with @rullzer.
This is best reviewed as https://github.com/nextcloud/server/pull/34379/files?w=1.