fix(profile): Set profile config cache directly after writing it to DB #37170
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
tcitworld
added
bug
3. to review
tcitworld
requested review from
ArtificialOwl,
icewind1991 and
blizzz
and removed request for
a team
|
This makes |
tcitworld
commented
Mar 10, 2023
| @@ -385,13 +385,15 @@ public function getProfileConfig(IUser $targetUser, ?IUser $visitingUser): array | |||
| $this->filterNotStoredProfileConfig($config->getConfigArray()), | |||
| )); | |||
| $this->configMapper->update($config); | |||
There was a problem hiding this comment.
suggestion: BTW, do we really need to update the config at each call? Can't we just check if the merged config has been updated and only update it in this case?
tcitworld
force-pushed
the
fix/profile/set-config-cache-after-write
branch
from
1361e88 to
ac9a2e4
Compare
In ProfileManager::getProfileParams we make a lot of indirect calls to ProfileManager::getProfileConfig. The first time we get the call we get into the catch and insert data, and the next time we miss the cache and do the select. There's a possibility of read-after-write issue here (the database r/o-mirror doesn't have yet the inserted config), which leads to conflicts when it tries to reinsert the config. To avoid that, let's put the config value in the configcache directly when it's written. Signed-off-by: Thomas Citharel <[email protected]>
AndyScherzinger
force-pushed
the
fix/profile/set-config-cache-after-write
branch
from
ac9a2e4 to
dc8aecb
Compare
This was referenced Mar 12, 2024
Merged
Merged
Merged
skjnldsv
added
2. developing
Merged
Merged
Merged
|
I think this is fixed already |
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.
In
ProfileManager::getProfileParamswe make a lot of indirect calls toProfileManager::getProfileConfig. The first time we get the call we get into the catch and insert data, and the next time we miss the cache and do the select. There's a possibility of read-after-write issue here (the database r/o-mirror doesn't have the inserted config yet), which leads to conflicts when it tries to reinsert the config. To avoid that, let's put the config value in the configcache directly when it's written.Fixes:
Checklist