Add the ability to change the SQL Write Lock TimeOut #9744
Conversation
There was a problem hiding this comment.
Tested with running the default values and also adding a new key to the web.config. Values were applied and published worked without any issues. Tested with a plain text string in a document type and also with Nest Content elements.
Value set to -1 to ensure that the lock timeout was being activated.
There was a problem hiding this comment.
I have left some comments on the code.
Before this is released, we need to test what happens with high timeout values compared to what the db connection and command timeout is - because we aren't sure what happens if the configured lock timeout exceeds either of those, we may very well end up with other different more vague errors which would be worse.
| { | ||
| try | ||
| { | ||
| return int.Parse(ConfigurationManager.AppSettings[Constants.AppSettings.SqlWriteLockTimeOut]); |
There was a problem hiding this comment.
Should be tryparse and not have a try/catch, it's very easy to do error checking here without any need for try/catch. This should also store the result of this in a backing field so that it's not re-reading from config and re-parsing this value on every single usage of this - which will be a lot because this will be accessed for every read/write lock (lots).
There was a problem hiding this comment.
I was just repeating the code above! 😅
Good point in adding a backing field here, I was under the assumption these settings were cached somewhere. 😄
| } | ||
| catch | ||
| { | ||
| return 1800; |
There was a problem hiding this comment.
Default should be changed to perhaps 5000.
There was a problem hiding this comment.
I kept it the same so that it will not change any behavior for now unless specifically configured.
Should we limit it to a max value so people can't inflict too much pain on themselves? |
Default value is now 5000 There's a limit to the range of the timeout values Introduced backing field so as not to keep looking up the same value over and over again from the config
|
Sounds like a great , we struggled with the time , cant wait for this fix - great work guys |
|
We're running into this issue a lot with Umbraco Cloud. Error logs getting spammed with |
|
@nul800sebastiaan any chance of RC of 8.12 ? so we can test this function ? |
|
@afinn-tech The 8.12 RC is out now: https://our.umbraco.com/download/releases/8120 / https://www.nuget.org/packages/UmbracoCms/8.12.0-rc |
|
Is the |
|
@stefanoberetta this lock is a distributed lock that uses the database. If you have 2 websites pointing to the same database you are Load Balancing with more than one main server which is not explicitly supported (but people do it). This error isn't specific to load balancing, this error will occur on a single server or multiple servers where a write lock is held for too long while another request or process is trying to acquire another read/write lock with the same ID but it times out. here's a description: #9594 (comment) |
|
We are having a similar problem with a 8.6+ build (now on 8.12). Single server setup iis and sql on the same box. After 6/7 saves we get this error, worse with multiple editors, nothing special running at all, tried increasing timeout using writelock setting, content started to disappear across the site. It’s a big problem we have two sites waiting to go live this week with 20 editors and they can’t use the site. Never been a problem before on over 80 deployments. |
|
@timmather is it using Vendr perhaps? |
|
Unfortunately not, it’s actually a very simple build apart from having around 2k nodes. |
When acquiring a SQL write lock, we currently have hardcoded this to 1.8 seconds (1800ms). This has been the case since version 8.4.0 - #6688
In some cases, people seem to be getting the error
Lock request time out period exceeded.. From what I gather from Shannon this indicates the followin:From what I understand from conversations at HQ, the value of 1800ms is rather arbitrary and there's no objection to increasing it to 5000. This PR is a first step towards that, we have seen various issues with locks being exceeded.
Lock request time out period exceeded., it might be related but there's not enough info yet.With this PR merged, it will be possible to at least experiment with the lock timeout and alleviate some pain for now. It is still a good idea to investigate what's holding locks and why the lock time is not enough for some operations.