Fixing locking issues for document type saves.#15854
Merged
Conversation
…l OnExecutingCommand and OnExecutedCommand when executing DbCommands
…hereby roundtrip) per lock instead of two
bergmania
added
release/10.8.5
release/12.3.8
release/13.2.1
and removed
release/10.9.0
release/13.3.0
type/feature
labels
bergmania
added a commit
that referenced
this pull request
Mar 15, 2024
* Added ExecuteNonQuery(DbCommand command) on database to ensure we call OnExecutingCommand and OnExecutedCommand when executing DbCommands * Added Cache Instructions lock, to avoid deadlocks * Optimized read locks for nucache when only one content type is rebuilt * Optimized the SqlServer locks, so only one command is executed (and thereby roundtrip) per lock instead of two * Avoid breaking changes * Cosmetic changes * Take locks if everything is rebuild * Use same lock in scopes, to avoid potential deadlocks between the two * Use eager locks in PublishedSnapshotService.cs * Added timeouts to some of the application locks * Revert "Use eager locks in PublishedSnapshotService.cs" This reverts commit 01873aa. * Revert "Added Cache Instructions lock, to avoid deadlocks" This reverts commit e3fca7c. * Use single readlock call to lock many * Use eager locks for reads * Eager write locks * Ignore test of lazy locks * Unique timeout exception messages --------- Co-authored-by: kjac <kja@umbraco.dk> (cherry picked from commit 2c23e67)
bergmania
added a commit
that referenced
this pull request
Mar 18, 2024
* Added ExecuteNonQuery(DbCommand command) on database to ensure we call OnExecutingCommand and OnExecutedCommand when executing DbCommands * Added Cache Instructions lock, to avoid deadlocks * Optimized read locks for nucache when only one content type is rebuilt * Optimized the SqlServer locks, so only one command is executed (and thereby roundtrip) per lock instead of two * Avoid breaking changes * Cosmetic changes * Take locks if everything is rebuild * Use same lock in scopes, to avoid potential deadlocks between the two * Use eager locks in PublishedSnapshotService.cs * Added timeouts to some of the application locks * Revert "Use eager locks in PublishedSnapshotService.cs" This reverts commit 01873aa. * Revert "Added Cache Instructions lock, to avoid deadlocks" This reverts commit e3fca7c. * Use single readlock call to lock many * Use eager locks for reads * Eager write locks * Ignore test of lazy locks * Unique timeout exception messages --------- Co-authored-by: kjac <kja@umbraco.dk> (cherry picked from commit 2c23e67)
bergmania
added a commit
that referenced
this pull request
Mar 18, 2024
* Added ExecuteNonQuery(DbCommand command) on database to ensure we call OnExecutingCommand and OnExecutedCommand when executing DbCommands * Added Cache Instructions lock, to avoid deadlocks * Optimized read locks for nucache when only one content type is rebuilt * Optimized the SqlServer locks, so only one command is executed (and thereby roundtrip) per lock instead of two * Avoid breaking changes * Cosmetic changes * Take locks if everything is rebuild * Use same lock in scopes, to avoid potential deadlocks between the two * Use eager locks in PublishedSnapshotService.cs * Added timeouts to some of the application locks * Revert "Use eager locks in PublishedSnapshotService.cs" This reverts commit 01873aa. * Revert "Added Cache Instructions lock, to avoid deadlocks" This reverts commit e3fca7c. * Use single readlock call to lock many * Use eager locks for reads * Eager write locks * Ignore test of lazy locks * Unique timeout exception messages --------- Co-authored-by: kjac <kja@umbraco.dk> (cherry picked from commit 2c23e67)
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Description
This PR revert to only use eager locks, to minimize deadlocks.
Further it optimizes what locks are taking when
NuCacheis rebuilt, and only takes the lock for document, media and members when needed.The SQL server lock is also optimized to only do one roundtrip to the database per lock instead of two.
Finally, the Sqlite lock did not call the OnExecutedCommand methods so was only passing a test by luck (A SqlCount was only 0 because it was never increased with the used command). This is not fixed by adding a new method on the
IUmbracoDatabasewith a default implementation, to avoid breaking changes.I really hope this is the root cause to #14195 🙈
Test
Things should still work, but one way to test it, is to load test the endpoints. This can quite easily be done by using the keyboard shortcuts in backoffice to save many times if you hold the short cuts down for like 30 sec.
Interesting things to test
All tests needs to be done both with SqlServer and SQLite.
Appendix
Document Type updates before this PR
Screen.Recording.2024-03-15.at.09.29.07.mov
Document Type updates after this PR
Screen.Recording.2024-03-15.at.09.21.05.mov
Document updates after this PR
Screen.Recording.2024-03-15.at.09.24.08.mov