From 988be21990ef56c6b452fb4a83b28cfa67a08b0c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Niels=20Lyngs=C3=B8?= Date: Wed, 13 Feb 2019 13:20:01 +0100 Subject: [PATCH 01/23] making the grid work in splitview --- .../src/less/components/umb-grid.less | 37 +++++----- .../propertyeditors/grid/grid.controller.js | 68 +++++++++++-------- .../src/views/propertyeditors/grid/grid.html | 40 +++++------ 3 files changed, 81 insertions(+), 64 deletions(-) diff --git a/src/Umbraco.Web.UI.Client/src/less/components/umb-grid.less b/src/Umbraco.Web.UI.Client/src/less/components/umb-grid.less index e7f24d08fa1d..6bf8cd612a97 100644 --- a/src/Umbraco.Web.UI.Client/src/less/components/umb-grid.less +++ b/src/Umbraco.Web.UI.Client/src/less/components/umb-grid.less @@ -128,9 +128,10 @@ position: relative; margin-bottom: 40px; padding-top: 10px; + border: 1px solid @grayLighter; &:hover { - background-color: @grayLighter; + border-color: @grayLight; } &[ng-click], @@ -161,15 +162,16 @@ } .umb-grid .umb-row .umb-cell-placeholder { - min-height: 130px; - background-color: @gray-10; - border-width: 2px; + min-height: 88px; + border-width: 1px; border-style: dashed; - border-color: @gray-8; + border-color: @ui-action-disgrete-border; + color: @ui-action-disgrete-type; transition: border-color 100ms linear; &:hover { - border-color: @blueMid; + border-color: @ui-action-disgrete-border-hover; + color: @ui-action-disgrete-type-hover; cursor: pointer; } } @@ -207,9 +209,9 @@ } .umb-grid .cell-tools-add { - color: @ui-action-type; + color: @ui-action-disgrete-type; &:focus, &:hover { - color: @ui-action-type-hover; + color: @ui-action-disgrete-type-hover; text-decoration: none; } } @@ -219,16 +221,18 @@ top: 50%; left: 50%; transform: translate(-50%, -50%); - color: @ui-action-type; + color: @ui-action-disgrete-type; } .umb-grid .cell-tools-add.-bar { display: block; - background: @gray-10; text-align: center; padding: 5px; - border: 1px dashed @gray-7; + border: 1px dashed @ui-action-disgrete-border; margin: 10px; + &:focus, &:hover { + border-color: @ui-action-disgrete-border-hover; + } } @@ -249,7 +253,6 @@ .umb-grid .cell-tools-edit { display: inline-block; - color: @white; } .umb-grid .drop-overlay { @@ -412,12 +415,12 @@ // Row states .umb-grid .umb-row.-active { - background-color: @ui-action-type; + border-color: @ui-action-type; .umb-row-title-bar { cursor: move; } - + /* .row-tool, .umb-row-title { color: @white; @@ -436,12 +439,13 @@ .umb-cell .umb-cell-content { border-color: transparent; } + */ } .umb-grid .umb-row.-active-child { background-color: @gray-10; - + .umb-row-title-bar { cursor: inherit; } @@ -449,7 +453,7 @@ .umb-row-title { color: @gray-3; } - + /* .row-tool { color: fade(@black, 23); } @@ -465,6 +469,7 @@ border-color: fade(@gray, 44); } } + */ } diff --git a/src/Umbraco.Web.UI.Client/src/views/propertyeditors/grid/grid.controller.js b/src/Umbraco.Web.UI.Client/src/views/propertyeditors/grid/grid.controller.js index b40305f5f5f7..0ea5842612e7 100644 --- a/src/Umbraco.Web.UI.Client/src/views/propertyeditors/grid/grid.controller.js +++ b/src/Umbraco.Web.UI.Client/src/views/propertyeditors/grid/grid.controller.js @@ -14,11 +14,16 @@ angular.module("umbraco") // Grid status variables var placeHolder = ""; var currentForm = angularHelper.getCurrentForm($scope); - + + $scope.currentRowWithActiveChild = null; + $scope.currentCellWithActiveChild = null; + $scope.active = null; + $scope.currentRow = null; $scope.currentCell = null; $scope.currentToolsControl = null; $scope.currentControl = null; + $scope.openRTEToolbarId = null; $scope.hasSettings = false; $scope.showRowConfigurations = true; @@ -235,10 +240,16 @@ angular.module("umbraco") }, 500, false); $scope.$apply(function () { - + + console.log("$apply function called...") + var cell = $(e.target).scope().area; - cell.hasActiveChild = hasActiveChild(cell, cell.controls); - cell.active = false; + + if(hasActiveChild(cell, cell.controls)) { + $scope.currentCellWithActiveChild = cell; + } + $scope.active = cell; + }); } @@ -307,12 +318,13 @@ angular.module("umbraco") // Row management function // ********************************************* - $scope.clickRow = function(index, rows) { - rows[index].active = true; - }; - - $scope.clickOutsideRow = function(index, rows) { - rows[index].active = false; + $scope.clickRow = function(index, rows, $event) { + console.log("clickRow") + //rows[index].active = true; + $scope.currentRowWithActiveChild = null; + $scope.active = rows[index]; + + $event.stopPropagation(); }; function getAllowedLayouts(section) { @@ -359,6 +371,7 @@ angular.module("umbraco") if (section.rows.length > 0) { section.rows.splice($index, 1); $scope.currentRow = null; + $scope.currentRowWithActiveChild = null; $scope.openRTEToolbarId = null; currentForm.$setDirty(); } @@ -513,14 +526,13 @@ angular.module("umbraco") // Area management functions // ********************************************* - $scope.clickCell = function(index, cells, row) { - cells[index].active = true; - row.hasActiveChild = true; - }; - - $scope.clickOutsideCell = function(index, cells, row) { - cells[index].active = false; - row.hasActiveChild = hasActiveChild(row, cells); + $scope.clickCell = function(index, cells, row, $event) { + + $scope.currentCellWithActiveChild = null; + + $scope.active = cells[index]; + $scope.currentRowWithActiveChild = row; + $event.stopPropagation(); }; $scope.cellPreview = function (cell) { @@ -536,14 +548,14 @@ angular.module("umbraco") // ********************************************* // Control management functions // ********************************************* - $scope.clickControl = function (index, controls, cell) { - controls[index].active = true; - cell.hasActiveChild = true; - }; - - $scope.clickOutsideControl = function (index, controls, cell) { - controls[index].active = false; - cell.hasActiveChild = hasActiveChild(cell, controls); + $scope.clickControl = function (index, controls, cell, $event) { + + console.log("clickControl"); + + $scope.active = controls[index]; + $scope.currentCellWithActiveChild = cell; + + $event.stopPropagation(); }; function hasActiveChild(item, children) { @@ -594,8 +606,8 @@ angular.module("umbraco") if (index === undefined) { index = cell.controls.length; } - - newControl.active = true; + + $scope.active = newControl; //populate control $scope.initControl(newControl, index + 1); diff --git a/src/Umbraco.Web.UI.Client/src/views/propertyeditors/grid/grid.html b/src/Umbraco.Web.UI.Client/src/views/propertyeditors/grid/grid.html index 22e72daff900..eeb70718f9b9 100644 --- a/src/Umbraco.Web.UI.Client/src/views/propertyeditors/grid/grid.html +++ b/src/Umbraco.Web.UI.Client/src/views/propertyeditors/grid/grid.html @@ -69,13 +69,13 @@
@@ -89,7 +89,7 @@
-
+
@@ -123,17 +123,17 @@ ng-style="{width: area.$percentage + '%'}" ng-class="{ '-has-config': area.hasConfig, - '-active': area.active, - '-active-child': area.hasActiveChild}" + '-active': area === active, + '-active-child': area === currentCellWithActiveChild}" ng-model="area.controls" - ng-click="clickCell($index, row.areas, row)" - on-outside-click="clickOutsideCell($index, row.areas, row)" - bind-click-on="{{area.active}}"> + ng-click="clickCell($index, row.areas, row, $event)" + + bind-click-on="{{area === active}}">
@@ -153,7 +153,7 @@
-
+
@@ -172,14 +172,14 @@
-
+
{{control.editor.name}} @@ -189,11 +189,11 @@
-
+
{{control.editor.name}}
-
+
From 60fa931f3f6c64a97f5350421461f14b99bca507 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Niels=20Lyngs=C3=B8?= Date: Wed, 13 Feb 2019 14:38:25 +0100 Subject: [PATCH 02/23] Grid-RTE got a unique ID, which fixes TineMCE code, which is dependent on a unique ID. #4011 --- .../components/grid/grid.rte.directive.js | 14 ++++++-------- .../src/views/components/grid/grid-rte.html | 2 +- 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/src/Umbraco.Web.UI.Client/src/common/directives/components/grid/grid.rte.directive.js b/src/Umbraco.Web.UI.Client/src/common/directives/components/grid/grid.rte.directive.js index 6472dd3d3841..dbbb0e98a2eb 100644 --- a/src/Umbraco.Web.UI.Client/src/common/directives/components/grid/grid.rte.directive.js +++ b/src/Umbraco.Web.UI.Client/src/common/directives/components/grid/grid.rte.directive.js @@ -13,6 +13,10 @@ angular.module("umbraco.directives") // TODO: A lot of the code below should be shared between the grid rte and the normal rte var promises = []; + + var d = new Date(); + var n = d.getTime(); + scope.textAreaHtmlId = scope.uniqueId + "_" + n + "_rte"; //queue file loading if (typeof (tinymce) === "undefined") { @@ -28,7 +32,7 @@ angular.module("umbraco.directives") var tinyMceEditor = null; promises.push(tinyMceService.getTinyMceEditorConfig({ - htmlId: scope.uniqueId, + htmlId: scope.textAreaHtmlId, stylesheets: scope.configuration ? scope.configuration.stylesheets : null, toolbar: toolbar, mode: scope.configuration.mode @@ -132,17 +136,11 @@ angular.module("umbraco.directives") } }); - - //listen for formSubmitting event (the result is callback used to remove the event subscription) - var formSubmittingListener = scope.$on("formSubmitting", function () { - scope.value = tinyMceEditor ? tinyMceEditor.getContent() : null; - }); - + //when the element is disposed we need to unsubscribe! // NOTE: this is very important otherwise if this is part of a modal, the listener still exists because the dom // element might still be there even after the modal has been hidden. scope.$on('$destroy', function () { - formSubmittingListener(); eventsService.unsubscribe(tabShownListener); //ensure we unbind this in case the blur doesn't fire above $('.umb-panel-body').off('scroll', pinToolbar); diff --git a/src/Umbraco.Web.UI.Client/src/views/components/grid/grid-rte.html b/src/Umbraco.Web.UI.Client/src/views/components/grid/grid-rte.html index f3b854a0621c..afa9869676e0 100644 --- a/src/Umbraco.Web.UI.Client/src/views/components/grid/grid-rte.html +++ b/src/Umbraco.Web.UI.Client/src/views/components/grid/grid-rte.html @@ -1,3 +1,3 @@ 
-
+
From 13a326b4673a89fa5694ed81de0ff06470110ece Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Niels=20Lyngs=C3=B8?= Date: Mon, 18 Feb 2019 10:58:59 +0100 Subject: [PATCH 03/23] remove logs --- .../src/views/propertyeditors/grid/grid.controller.js | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/Umbraco.Web.UI.Client/src/views/propertyeditors/grid/grid.controller.js b/src/Umbraco.Web.UI.Client/src/views/propertyeditors/grid/grid.controller.js index 0ea5842612e7..650717b7c098 100644 --- a/src/Umbraco.Web.UI.Client/src/views/propertyeditors/grid/grid.controller.js +++ b/src/Umbraco.Web.UI.Client/src/views/propertyeditors/grid/grid.controller.js @@ -240,9 +240,6 @@ angular.module("umbraco") }, 500, false); $scope.$apply(function () { - - console.log("$apply function called...") - var cell = $(e.target).scope().area; if(hasActiveChild(cell, cell.controls)) { @@ -319,7 +316,6 @@ angular.module("umbraco") // ********************************************* $scope.clickRow = function(index, rows, $event) { - console.log("clickRow") //rows[index].active = true; $scope.currentRowWithActiveChild = null; $scope.active = rows[index]; @@ -550,8 +546,6 @@ angular.module("umbraco") // ********************************************* $scope.clickControl = function (index, controls, cell, $event) { - console.log("clickControl"); - $scope.active = controls[index]; $scope.currentCellWithActiveChild = cell; From 2e4091145066435bf8eba2d9e7bce5d1399fbe36 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Niels=20Lyngs=C3=B8?= Date: Mon, 18 Feb 2019 10:59:30 +0100 Subject: [PATCH 04/23] get uniq ID by using String.CreateGuid() --- .../directives/components/grid/grid.rte.directive.js | 9 +++++---- .../src/views/propertyeditors/rte/rte.controller.js | 4 +--- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/src/Umbraco.Web.UI.Client/src/common/directives/components/grid/grid.rte.directive.js b/src/Umbraco.Web.UI.Client/src/common/directives/components/grid/grid.rte.directive.js index dbbb0e98a2eb..0f933a4cccc6 100644 --- a/src/Umbraco.Web.UI.Client/src/common/directives/components/grid/grid.rte.directive.js +++ b/src/Umbraco.Web.UI.Client/src/common/directives/components/grid/grid.rte.directive.js @@ -14,10 +14,11 @@ angular.module("umbraco.directives") var promises = []; - var d = new Date(); - var n = d.getTime(); - scope.textAreaHtmlId = scope.uniqueId + "_" + n + "_rte"; - + //To id the html textarea we need to use the datetime ticks because we can have multiple rte's per a single property alias + // because now we have to support having 2x (maybe more at some stage) content editors being displayed at once. This is because + // we have this mini content editor panel that can be launched with MNTP. + scope.textAreaHtmlId = scope.uniqueId + "_" + String.CreateGuid(); + //queue file loading if (typeof (tinymce) === "undefined") { promises.push(assetsService.loadJs("lib/tinymce/tinymce.min.js", scope)); diff --git a/src/Umbraco.Web.UI.Client/src/views/propertyeditors/rte/rte.controller.js b/src/Umbraco.Web.UI.Client/src/views/propertyeditors/rte/rte.controller.js index 6099c5dad4a4..43da0cb66fe3 100644 --- a/src/Umbraco.Web.UI.Client/src/views/propertyeditors/rte/rte.controller.js +++ b/src/Umbraco.Web.UI.Client/src/views/propertyeditors/rte/rte.controller.js @@ -9,9 +9,7 @@ angular.module("umbraco") //To id the html textarea we need to use the datetime ticks because we can have multiple rte's per a single property alias // because now we have to support having 2x (maybe more at some stage) content editors being displayed at once. This is because // we have this mini content editor panel that can be launched with MNTP. - var d = new Date(); - var n = d.getTime(); - $scope.textAreaHtmlId = $scope.model.alias + "_" + n + "_rte"; + $scope.textAreaHtmlId = $scope.model.alias + "_" + String.CreateGuid(); var editorConfig = $scope.model.config ? $scope.model.config.editor : null; if (!editorConfig || angular.isString(editorConfig)) { From 44b1a738bf5a4d9ca04b105399d61990ac469bd2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Niels=20Lyngs=C3=B8?= Date: Mon, 18 Feb 2019 10:59:40 +0100 Subject: [PATCH 05/23] clean up css for grid --- .../src/less/components/umb-grid.less | 38 +------------------ 1 file changed, 1 insertion(+), 37 deletions(-) diff --git a/src/Umbraco.Web.UI.Client/src/less/components/umb-grid.less b/src/Umbraco.Web.UI.Client/src/less/components/umb-grid.less index 6bf8cd612a97..5283e259c70b 100644 --- a/src/Umbraco.Web.UI.Client/src/less/components/umb-grid.less +++ b/src/Umbraco.Web.UI.Client/src/less/components/umb-grid.less @@ -420,26 +420,6 @@ .umb-row-title-bar { cursor: move; } - /* - .row-tool, - .umb-row-title { - color: @white; - } - - .umb-grid-has-config { - color: fade(@white, 66); - } - - .umb-cell { - .umb-grid-has-config { - color: fade(@black, 44); - } - } - - .umb-cell .umb-cell-content { - border-color: transparent; - } - */ } @@ -453,23 +433,7 @@ .umb-row-title { color: @gray-3; } - /* - .row-tool { - color: fade(@black, 23); - } - - .umb-grid-has-config { - color: fade(@black, 44); - } - - .umb-cell-content.-placeholder { - border-color: @gray-8; - - &:hover { - border-color: fade(@gray, 44); - } - } - */ + } From 3f13baa8e773d9435ae9eab239c7d883881bd193 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Niels=20Lyngs=C3=B8?= Date: Mon, 18 Feb 2019 11:00:47 +0100 Subject: [PATCH 06/23] make Embed and Media work for Grid in SplitView when grid is not allowed to vary. --- .../grid/editors/embed.controller.js | 21 +++++------------- .../propertyeditors/grid/editors/embed.html | 4 ++-- .../grid/editors/media.controller.js | 22 ++++++------------- .../propertyeditors/grid/editors/media.html | 4 ++-- 4 files changed, 17 insertions(+), 34 deletions(-) diff --git a/src/Umbraco.Web.UI.Client/src/views/propertyeditors/grid/editors/embed.controller.js b/src/Umbraco.Web.UI.Client/src/views/propertyeditors/grid/editors/embed.controller.js index beb8edab204a..17916299bcc5 100644 --- a/src/Umbraco.Web.UI.Client/src/views/propertyeditors/grid/editors/embed.controller.js +++ b/src/Umbraco.Web.UI.Client/src/views/propertyeditors/grid/editors/embed.controller.js @@ -2,24 +2,16 @@ angular.module("umbraco") .controller("Umbraco.PropertyEditors.Grid.EmbedController", function ($scope, $timeout, $sce, editorService) { - function onInit() { - $scope.trustedValue = null; - $scope.trustedValue = $sce.trustAsHtml($scope.control.value); - - if(!$scope.control.value) { - $timeout(function(){ - if($scope.control.$initializing){ - $scope.setEmbed(); - } - }, 200); - } + $scope.hasEmbed = function(){ + return $scope.control.value !== null; + } + $scope.getEmbed = function(){ + return $sce.trustAsHtml($scope.control.value); } - $scope.setEmbed = function(){ var embed = { submit: function(model) { $scope.control.value = model.embed.preview; - $scope.trustedValue = $sce.trustAsHtml(model.embed.preview); editorService.close(); }, close: function() { @@ -28,6 +20,5 @@ angular.module("umbraco") }; editorService.embed(embed); }; - - onInit(); + }); diff --git a/src/Umbraco.Web.UI.Client/src/views/propertyeditors/grid/editors/embed.html b/src/Umbraco.Web.UI.Client/src/views/propertyeditors/grid/editors/embed.html index 87109e1eb9ee..32ebfd32570b 100644 --- a/src/Umbraco.Web.UI.Client/src/views/propertyeditors/grid/editors/embed.html +++ b/src/Umbraco.Web.UI.Client/src/views/propertyeditors/grid/editors/embed.html @@ -1,10 +1,10 @@
-
+
Click to embed
-
+
diff --git a/src/Umbraco.Web.UI.Client/src/views/propertyeditors/grid/editors/media.controller.js b/src/Umbraco.Web.UI.Client/src/views/propertyeditors/grid/editors/media.controller.js index e267133cf4f5..c290b8ae7d3a 100644 --- a/src/Umbraco.Web.UI.Client/src/views/propertyeditors/grid/editors/media.controller.js +++ b/src/Umbraco.Web.UI.Client/src/views/propertyeditors/grid/editors/media.controller.js @@ -8,13 +8,7 @@ angular.module("umbraco") $scope.model.config.startNodeIsVirtual = userData.startMediaIds.length !== 1; }); } - - function onInit() { - if($scope.control.value){ - $scope.setUrl(); - } - } - + $scope.setImage = function(){ var startNodeId = $scope.model.config && $scope.model.config.startNodeId ? $scope.model.config.startNodeId : undefined; var startNodeIsVirtual = startNodeId ? $scope.model.config.startNodeIsVirtual : undefined; @@ -34,10 +28,8 @@ angular.module("umbraco") id: selectedImage.id, udi: selectedImage.udi, image: selectedImage.image, - altText: selectedImage.altText - }; - - $scope.setUrl(); + caption: selectedImage.altText + }; editorService.close(); }, @@ -49,7 +41,7 @@ angular.module("umbraco") editorService.mediaPicker(mediaPicker); }; - $scope.setUrl = function(){ + $scope.getThumbnailUrl = function(){ if($scope.control.value.image){ var url = $scope.control.value.image; @@ -70,10 +62,10 @@ angular.module("umbraco") { url += "?width=800&upscale=false&animationprocessmode=false" } - $scope.url = url; + return url; } + + return ""; }; - onInit(); - }); diff --git a/src/Umbraco.Web.UI.Client/src/views/propertyeditors/grid/editors/media.html b/src/Umbraco.Web.UI.Client/src/views/propertyeditors/grid/editors/media.html index 7ffb26d83173..1a91fe52b83a 100644 --- a/src/Umbraco.Web.UI.Client/src/views/propertyeditors/grid/editors/media.html +++ b/src/Umbraco.Web.UI.Client/src/views/propertyeditors/grid/editors/media.html @@ -7,9 +7,9 @@
From 6f673184324a90f10109a18e87e60e2b65f76ee8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Niels=20Lyngs=C3=B8?= Date: Tue, 19 Feb 2019 10:18:01 +0100 Subject: [PATCH 07/23] corrected color variables --- .../src/less/components/umb-grid.less | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/Umbraco.Web.UI.Client/src/less/components/umb-grid.less b/src/Umbraco.Web.UI.Client/src/less/components/umb-grid.less index 3aab0340a87e..5a14294ebf7f 100644 --- a/src/Umbraco.Web.UI.Client/src/less/components/umb-grid.less +++ b/src/Umbraco.Web.UI.Client/src/less/components/umb-grid.less @@ -542,14 +542,13 @@ display: inline-block; cursor: pointer; border-radius: 200px; - background: @gray-10; - border: 1px solid @gray-7; + border: 1px solid @ui-action-disgrete-border; margin: 2px; &:hover, &:hover * { - background: @ui-action-type-hover !important; + background: @ui-action-disgrete-type-hover !important; color: @white !important; - border-color: @ui-action-type-hover !important; + border-color: @ui-action-disgrete-border-hover !important; text-decoration: none; } } From 592d8db3e72194eeafa7a52229287ce538e7974c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Niels=20Lyngs=C3=B8?= Date: Tue, 19 Feb 2019 10:18:13 +0100 Subject: [PATCH 08/23] removed comment --- .../src/views/propertyeditors/grid/grid.controller.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Umbraco.Web.UI.Client/src/views/propertyeditors/grid/grid.controller.js b/src/Umbraco.Web.UI.Client/src/views/propertyeditors/grid/grid.controller.js index 650717b7c098..ff01c8cc719e 100644 --- a/src/Umbraco.Web.UI.Client/src/views/propertyeditors/grid/grid.controller.js +++ b/src/Umbraco.Web.UI.Client/src/views/propertyeditors/grid/grid.controller.js @@ -316,7 +316,7 @@ angular.module("umbraco") // ********************************************* $scope.clickRow = function(index, rows, $event) { - //rows[index].active = true; + $scope.currentRowWithActiveChild = null; $scope.active = rows[index]; From 564117e62edd23f5e786c9619f02a9f72ad60455 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Niels=20Lyngs=C3=B8?= Date: Tue, 19 Feb 2019 10:19:02 +0100 Subject: [PATCH 09/23] Grid editor to use $watch for values that needs to transform for presentation --- .../grid/editors/embed.controller.js | 24 ++++++++++++++----- .../propertyeditors/grid/editors/embed.html | 2 +- .../grid/editors/media.controller.js | 22 +++++++++++++---- .../propertyeditors/grid/editors/media.html | 5 ++-- 4 files changed, 38 insertions(+), 15 deletions(-) diff --git a/src/Umbraco.Web.UI.Client/src/views/propertyeditors/grid/editors/embed.controller.js b/src/Umbraco.Web.UI.Client/src/views/propertyeditors/grid/editors/embed.controller.js index 17916299bcc5..8e9ea06a84fe 100644 --- a/src/Umbraco.Web.UI.Client/src/views/propertyeditors/grid/editors/embed.controller.js +++ b/src/Umbraco.Web.UI.Client/src/views/propertyeditors/grid/editors/embed.controller.js @@ -1,14 +1,26 @@ angular.module("umbraco") .controller("Umbraco.PropertyEditors.Grid.EmbedController", function ($scope, $timeout, $sce, editorService) { - - $scope.hasEmbed = function(){ - return $scope.control.value !== null; - } - $scope.getEmbed = function(){ + + + + function getEmbed() { return $sce.trustAsHtml($scope.control.value); } - $scope.setEmbed = function(){ + + + $scope.embedHtml = getEmbed(); + $scope.$watch('control.value', function(newValue, oldValue) { + if(angular.equals(newValue, oldValue)){ + return; // simply skip that + } + + $scope.embedHtml = getEmbed(); + }, false); + $scope.hasEmbed = function() { + return $scope.control.value !== null; + } + $scope.setEmbed = function() { var embed = { submit: function(model) { $scope.control.value = model.embed.preview; diff --git a/src/Umbraco.Web.UI.Client/src/views/propertyeditors/grid/editors/embed.html b/src/Umbraco.Web.UI.Client/src/views/propertyeditors/grid/editors/embed.html index 32ebfd32570b..0178c3b3ec30 100644 --- a/src/Umbraco.Web.UI.Client/src/views/propertyeditors/grid/editors/embed.html +++ b/src/Umbraco.Web.UI.Client/src/views/propertyeditors/grid/editors/embed.html @@ -5,6 +5,6 @@
Click to embed
-
+
diff --git a/src/Umbraco.Web.UI.Client/src/views/propertyeditors/grid/editors/media.controller.js b/src/Umbraco.Web.UI.Client/src/views/propertyeditors/grid/editors/media.controller.js index c290b8ae7d3a..eb1032a9c7fd 100644 --- a/src/Umbraco.Web.UI.Client/src/views/propertyeditors/grid/editors/media.controller.js +++ b/src/Umbraco.Web.UI.Client/src/views/propertyeditors/grid/editors/media.controller.js @@ -1,7 +1,11 @@ angular.module("umbraco") .controller("Umbraco.PropertyEditors.Grid.MediaController", function ($scope, $timeout, userService, editorService) { - + + + $scope.thumbnailUrl = getThumbnailUrl(); + + if (!$scope.model.config.startNodeId) { userService.getCurrentUser().then(function (userData) { $scope.model.config.startNodeId = userData.startMediaIds.length !== 1 ? -1 : userData.startMediaIds[0]; @@ -40,10 +44,18 @@ angular.module("umbraco") editorService.mediaPicker(mediaPicker); }; + + $scope.$watch('control.value', function(newValue, oldValue) { + if(angular.equals(newValue, oldValue)){ + return; // simply skip that + } + + $scope.thumbnailUrl = getThumbnailUrl(); + }, true); + + function getThumbnailUrl() { - $scope.getThumbnailUrl = function(){ - - if($scope.control.value.image){ + if($scope.control.value && $scope.control.value.image) { var url = $scope.control.value.image; if($scope.control.editor.config && $scope.control.editor.config.size){ @@ -65,7 +77,7 @@ angular.module("umbraco") return url; } - return ""; + return null; }; }); diff --git a/src/Umbraco.Web.UI.Client/src/views/propertyeditors/grid/editors/media.html b/src/Umbraco.Web.UI.Client/src/views/propertyeditors/grid/editors/media.html index 1a91fe52b83a..184f707ebfb6 100644 --- a/src/Umbraco.Web.UI.Client/src/views/propertyeditors/grid/editors/media.html +++ b/src/Umbraco.Web.UI.Client/src/views/propertyeditors/grid/editors/media.html @@ -5,11 +5,10 @@
Click to insert image
-
+
From 2bfa81c7079d28625a7f4b65bc5e6dbaa50d3b96 Mon Sep 17 00:00:00 2001 From: Shannon Date: Tue, 26 Mar 2019 20:07:42 +1100 Subject: [PATCH 10/23] fixes 4627 + makes front-end validations appear as soon as they are invalid again. --- .../directives/validation/valpropertymsg.directive.js | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/Umbraco.Web.UI.Client/src/common/directives/validation/valpropertymsg.directive.js b/src/Umbraco.Web.UI.Client/src/common/directives/validation/valpropertymsg.directive.js index 9ee83dc2ba3f..c027e0778ee6 100644 --- a/src/Umbraco.Web.UI.Client/src/common/directives/validation/valpropertymsg.directive.js +++ b/src/Umbraco.Web.UI.Client/src/common/directives/validation/valpropertymsg.directive.js @@ -62,8 +62,8 @@ function valPropertyMsg(serverValidationManager) { if (!watcher) { watcher = scope.$watch("currentProperty.value", function (newValue, oldValue) { - - if (!newValue || angular.equals(newValue, oldValue)) { + + if (angular.equals(newValue, oldValue)) { return; } @@ -78,10 +78,12 @@ function valPropertyMsg(serverValidationManager) { // based on other errors. We'll also check if there's no other validation errors apart from valPropertyMsg, if valPropertyMsg // is the only one, then we'll clear. - if ((errCount === 1 && angular.isArray(formCtrl.$error.valPropertyMsg)) || (formCtrl.$invalid && angular.isArray(formCtrl.$error.valServer))) { + if (errCount === 0 || (errCount === 1 && angular.isArray(formCtrl.$error.valPropertyMsg)) || (formCtrl.$invalid && angular.isArray(formCtrl.$error.valServer))) { scope.errorMsg = ""; formCtrl.$setValidity('valPropertyMsg', true); - stopWatch(); + } else if (showValidation && scope.errorMsg === "") { + formCtrl.$setValidity('valPropertyMsg', false); + scope.errorMsg = getErrorMsg(); } }, true); } @@ -152,6 +154,7 @@ function valPropertyMsg(serverValidationManager) { showValidation = true; if (hasError && scope.errorMsg === "") { scope.errorMsg = getErrorMsg(); + startWatch(); } else if (!hasError) { scope.errorMsg = ""; From 080ace90b208b29ecca5e34bd0046fc7019d56ce Mon Sep 17 00:00:00 2001 From: Shannon Date: Tue, 26 Mar 2019 20:09:12 +1100 Subject: [PATCH 11/23] Bugfix: https://github.com/umbraco/Umbraco-CMS/issues/4827 - Fixed MediaPickerValueConverter when config is Only Images --- .../MediaPickerValueConverter.cs | 58 +++++++++++++------ 1 file changed, 39 insertions(+), 19 deletions(-) diff --git a/src/Umbraco.Web/PropertyEditors/ValueConverters/MediaPickerValueConverter.cs b/src/Umbraco.Web/PropertyEditors/ValueConverters/MediaPickerValueConverter.cs index 802f42ed9e8a..fb3134c9a3bb 100644 --- a/src/Umbraco.Web/PropertyEditors/ValueConverters/MediaPickerValueConverter.cs +++ b/src/Umbraco.Web/PropertyEditors/ValueConverters/MediaPickerValueConverter.cs @@ -1,4 +1,5 @@ using System; +using System.Collections; using System.Collections.Generic; using System.Linq; using Umbraco.Core; @@ -14,11 +15,18 @@ namespace Umbraco.Web.PropertyEditors.ValueConverters [DefaultPropertyValueConverter] public class MediaPickerValueConverter : PropertyValueConverterBase { + // hard-coding "image" here but that's how it works at UI level too + private const string ImageTypeAlias = "image"; + + private readonly IPublishedModelFactory _publishedModelFactory; private readonly IPublishedSnapshotAccessor _publishedSnapshotAccessor; - public MediaPickerValueConverter(IPublishedSnapshotAccessor publishedSnapshotAccessor) + public MediaPickerValueConverter(IPublishedSnapshotAccessor publishedSnapshotAccessor, + IPublishedModelFactory publishedModelFactory) { - _publishedSnapshotAccessor = publishedSnapshotAccessor ?? throw new ArgumentNullException(nameof(publishedSnapshotAccessor)); + _publishedSnapshotAccessor = publishedSnapshotAccessor ?? + throw new ArgumentNullException(nameof(publishedSnapshotAccessor)); + _publishedModelFactory = publishedModelFactory; } public override bool IsConverter(PublishedPropertyType propertyType) @@ -31,15 +39,19 @@ public override Type GetPropertyValueType(PublishedPropertyType propertyType) var isMultiple = IsMultipleDataType(propertyType.DataType); var isOnlyImages = IsOnlyImagesDataType(propertyType.DataType); - // hard-coding "image" here but that's how it works at UI level too - return isMultiple - ? (isOnlyImages ? typeof(IEnumerable<>).MakeGenericType(ModelType.For("image")) : typeof(IEnumerable)) - : (isOnlyImages ? ModelType.For("image") : typeof(IPublishedContent)); + ? isOnlyImages + ? typeof(IEnumerable<>).MakeGenericType(ModelType.For(ImageTypeAlias)) + : typeof(IEnumerable) + : isOnlyImages + ? ModelType.For(ImageTypeAlias) + : typeof(IPublishedContent); } public override PropertyCacheLevel GetPropertyCacheLevel(PublishedPropertyType propertyType) - => PropertyCacheLevel.Snapshot; + { + return PropertyCacheLevel.Snapshot; + } private bool IsMultipleDataType(PublishedDataType dataType) { @@ -53,26 +65,31 @@ private bool IsOnlyImagesDataType(PublishedDataType dataType) return config.OnlyImages; } - public override object ConvertSourceToIntermediate(IPublishedElement owner, PublishedPropertyType propertyType, object source, bool preview) + public override object ConvertSourceToIntermediate(IPublishedElement owner, PublishedPropertyType propertyType, + object source, bool preview) { if (source == null) return null; var nodeIds = source.ToString() - .Split(new[] { "," }, StringSplitOptions.RemoveEmptyEntries) + .Split(new[] {","}, StringSplitOptions.RemoveEmptyEntries) .Select(Udi.Parse) .ToArray(); return nodeIds; } - public override object ConvertIntermediateToObject(IPublishedElement owner, PublishedPropertyType propertyType, PropertyCacheLevel cacheLevel, object source, bool preview) + public override object ConvertIntermediateToObject(IPublishedElement owner, PublishedPropertyType propertyType, + PropertyCacheLevel cacheLevel, object source, bool preview) { - if (source == null) - { - return null; - } + var isMultiple = IsMultipleDataType(propertyType.DataType); + var isOnlyImages = IsOnlyImagesDataType(propertyType.DataType); + + var udis = (Udi[]) source; + var mediaItems = isOnlyImages + ? _publishedModelFactory.CreateModelList(ImageTypeAlias) + : new List(); + + if (source == null) return isMultiple ? mediaItems : null; - var udis = (Udi[])source; - var mediaItems = new List(); if (udis.Any()) { foreach (var udi in udis) @@ -84,12 +101,15 @@ public override object ConvertIntermediateToObject(IPublishedElement owner, Publ mediaItems.Add(item); } - if (IsMultipleDataType(propertyType.DataType)) - return mediaItems; - return mediaItems.FirstOrDefault(); + return isMultiple ? mediaItems : FirstOrDefault(mediaItems); } return source; } + + private object FirstOrDefault(IList mediaItems) + { + return mediaItems.Count == 0 ? null : mediaItems[0]; + } } } From 4fcf2ce7bdac14b49ca318021335d4fdd89ea252 Mon Sep 17 00:00:00 2001 From: Bjarke Berg Date: Tue, 12 Mar 2019 08:17:09 +0100 Subject: [PATCH 12/23] Fixed issue when creating variant content templates, but the properties is invariant. Before this change an exception was thrown --- src/Umbraco.Core/Services/Implement/ContentService.cs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/Umbraco.Core/Services/Implement/ContentService.cs b/src/Umbraco.Core/Services/Implement/ContentService.cs index 5bdc0959daf3..82ebd92f0b2f 100644 --- a/src/Umbraco.Core/Services/Implement/ContentService.cs +++ b/src/Umbraco.Core/Services/Implement/ContentService.cs @@ -2875,7 +2875,14 @@ public IContent CreateContentFromBlueprint(IContent blueprint, string name, int { foreach (var property in blueprint.Properties) { - content.SetValue(property.Alias, property.GetValue(culture), culture); + if (property.PropertyType.VariesByCulture()) + { + content.SetValue(property.Alias, property.GetValue(culture), culture); + } + else + { + content.SetValue(property.Alias, property.GetValue()); + } } content.Name = blueprint.Name; From 8a56b3db163620a6c0e04bba6f0f1ca015eb06ee Mon Sep 17 00:00:00 2001 From: Kenn Jacobsen Date: Sun, 17 Mar 2019 20:51:42 +0100 Subject: [PATCH 13/23] Fix the "Language" dropdown in the "Culture and Hostnames" dialog --- .../src/views/content/content.assigndomain.controller.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/Umbraco.Web.UI.Client/src/views/content/content.assigndomain.controller.js b/src/Umbraco.Web.UI.Client/src/views/content/content.assigndomain.controller.js index 0f27f3046cc8..5fa1eebd0b2d 100644 --- a/src/Umbraco.Web.UI.Client/src/views/content/content.assigndomain.controller.js +++ b/src/Umbraco.Web.UI.Client/src/views/content/content.assigndomain.controller.js @@ -46,8 +46,7 @@ if (data.language !== "undefined") { var lang = vm.languages.filter(function (l) { - return matchLanguageById(l, data.language.Id); - + return matchLanguageById(l, data.language); }); if (lang.length > 0) { vm.language = lang[0]; From a70cb51559a225f6e13c38be3893759867c4e3e2 Mon Sep 17 00:00:00 2001 From: Stephan Date: Fri, 22 Feb 2019 15:27:22 +0100 Subject: [PATCH 14/23] NuCache: cleanup scoped objects --- .../Scoping/ScopeContextualBase.cs | 33 +++++++++++++++-- .../NuCache/PublishedSnapshotService.cs | 36 +++++++++---------- 2 files changed, 48 insertions(+), 21 deletions(-) diff --git a/src/Umbraco.Core/Scoping/ScopeContextualBase.cs b/src/Umbraco.Core/Scoping/ScopeContextualBase.cs index 746114223452..1f2b6155e692 100644 --- a/src/Umbraco.Core/Scoping/ScopeContextualBase.cs +++ b/src/Umbraco.Core/Scoping/ScopeContextualBase.cs @@ -2,24 +2,43 @@ namespace Umbraco.Core.Scoping { - // base class for an object that will be enlisted in scope context, if any. it - // must be used in a 'using' block, and if not scoped, released when disposed, - // else when scope context runs enlisted actions + /// + /// Provides a base class for scope contextual objects. + /// + /// + /// A scope contextual object is enlisted in the current scope context, + /// if any, and released when the context exists. It must be used in a 'using' + /// block, and will be released when disposed, if not part of a scope. + /// public abstract class ScopeContextualBase : IDisposable { private bool _using, _scoped; + /// + /// Gets a contextual object. + /// + /// The type of the object. + /// A scope provider. + /// A context key for the object. + /// A function producing the contextual object. + /// The contextual object. + /// + /// + /// public static T Get(IScopeProvider scopeProvider, string key, Func ctor) where T : ScopeContextualBase { + // no scope context = create a non-scoped object var scopeContext = scopeProvider.Context; if (scopeContext == null) return ctor(false); + // create & enlist the scoped object var w = scopeContext.Enlist("ScopeContextualBase_" + key, () => ctor(true), (completed, item) => { item.Release(completed); }); + // the object can be 'used' only once at a time if (w._using) throw new InvalidOperationException("panic: used."); w._using = true; w._scoped = true; @@ -27,6 +46,10 @@ public static T Get(IScopeProvider scopeProvider, string key, Func c return w; } + /// + /// + /// If not scoped, then this releases the contextual object. + /// public void Dispose() { _using = false; @@ -35,6 +58,10 @@ public void Dispose() Release(true); } + /// + /// Releases the contextual object. + /// + /// A value indicating whether the scoped operation completed. public abstract void Release(bool completed); } } diff --git a/src/Umbraco.Web/PublishedCache/NuCache/PublishedSnapshotService.cs b/src/Umbraco.Web/PublishedCache/NuCache/PublishedSnapshotService.cs index e19531a25b19..541ff2ea2336 100755 --- a/src/Umbraco.Web/PublishedCache/NuCache/PublishedSnapshotService.cs +++ b/src/Umbraco.Web/PublishedCache/NuCache/PublishedSnapshotService.cs @@ -330,14 +330,15 @@ public override bool EnsureEnvironment(out IEnumerable errors) private void LockAndLoadContent(Action action) { + // first get a writer, then a scope + // if there already is a scope, the writer will attach to it + // otherwise, it will only exist here - cheap using (_contentStore.GetWriter(_scopeProvider)) + using (var scope = _scopeProvider.CreateScope()) { - using (var scope = _scopeProvider.CreateScope()) - { - scope.ReadLock(Constants.Locks.ContentTree); - action(scope); - scope.Complete(); - } + scope.ReadLock(Constants.Locks.ContentTree); + action(scope); + scope.Complete(); } } @@ -399,14 +400,13 @@ private void LoadContentFromLocalDbLocked(IScope scope) private void LockAndLoadMedia(Action action) { + // see note in LockAndLoadContent using (_mediaStore.GetWriter(_scopeProvider)) + using (var scope = _scopeProvider.CreateScope()) { - using (var scope = _scopeProvider.CreateScope()) - { - scope.ReadLock(Constants.Locks.MediaTree); - action(scope); - scope.Complete(); - } + scope.ReadLock(Constants.Locks.MediaTree); + action(scope); + scope.Complete(); } } @@ -528,14 +528,13 @@ private void LoadMediaFromLocalDbLocked(IScope scope) private void LockAndLoadDomains() { + // see note in LockAndLoadContent using (_domainStore.GetWriter(_scopeProvider)) + using (var scope = _scopeProvider.CreateScope()) { - using (var scope = _scopeProvider.CreateScope()) - { - scope.ReadLock(Constants.Locks.Domains); - LoadDomainsLocked(); - scope.Complete(); - } + scope.ReadLock(Constants.Locks.Domains); + LoadDomainsLocked(); + scope.Complete(); } } @@ -858,6 +857,7 @@ public override void Notify(DomainCacheRefresher.JsonPayload[] payloads) if (_isReady == false) return; + // see note in LockAndLoadContent using (_domainStore.GetWriter(_scopeProvider)) { foreach (var payload in payloads) From 13a8548d3a19e6de90ef180a9f87115c80abf1c3 Mon Sep 17 00:00:00 2001 From: Stephan Date: Fri, 22 Feb 2019 15:30:55 +0100 Subject: [PATCH 15/23] NuCache: rename things --- .../Cache/SnapDictionaryTests.cs | 10 ++-- .../PublishedCache/NuCache/ContentStore.cs | 56 ++++++++--------- .../PublishedCache/NuCache/SnapDictionary.cs | 60 +++++++++---------- 3 files changed, 63 insertions(+), 63 deletions(-) diff --git a/src/Umbraco.Tests/Cache/SnapDictionaryTests.cs b/src/Umbraco.Tests/Cache/SnapDictionaryTests.cs index 013dbadbb855..0ee0d180c3c8 100644 --- a/src/Umbraco.Tests/Cache/SnapDictionaryTests.cs +++ b/src/Umbraco.Tests/Cache/SnapDictionaryTests.cs @@ -388,8 +388,8 @@ public async Task EventuallyCollectNulls() // collect liveGen GC.Collect(); - SnapDictionary.GenerationObject genObj; - Assert.IsTrue(d.Test.GenerationObjects.TryPeek(out genObj)); + SnapDictionary.GenObj genObj; + Assert.IsTrue(d.Test.GenObjs.TryPeek(out genObj)); genObj = null; // in Release mode, it works, but in Debug mode, the weak reference is still alive @@ -399,14 +399,14 @@ public async Task EventuallyCollectNulls() GC.Collect(); #endif - Assert.IsTrue(d.Test.GenerationObjects.TryPeek(out genObj)); - Assert.IsFalse(genObj.WeakReference.IsAlive); // snapshot is gone, along with its reference + Assert.IsTrue(d.Test.GenObjs.TryPeek(out genObj)); + Assert.IsFalse(genObj.WeakGenRef.IsAlive); // snapshot is gone, along with its reference await d.CollectAsync(); Assert.AreEqual(0, d.Test.GetValues(1).Length); // null value is gone Assert.AreEqual(0, d.Count); // item is gone - Assert.AreEqual(0, d.Test.GenerationObjects.Count); + Assert.AreEqual(0, d.Test.GenObjs.Count); Assert.AreEqual(0, d.SnapCount); // snapshot is gone Assert.AreEqual(0, d.GenCount); // and generation has been dequeued } diff --git a/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs b/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs index b3996050a6e9..e78c4d419e88 100644 --- a/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs +++ b/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs @@ -29,8 +29,8 @@ internal class ContentStore private readonly ILogger _logger; private BPlusTree _localDb; - private readonly ConcurrentQueue _genRefRefs; - private GenRefRef _genRefRef; + private readonly ConcurrentQueue _genObjs; + private GenObj _genObj; private readonly object _wlocko = new object(); private readonly object _rlocko = new object(); private long _liveGen, _floorGen; @@ -64,8 +64,8 @@ public ContentStore( _contentTypesByAlias = new ConcurrentDictionary>(StringComparer.InvariantCultureIgnoreCase); _xmap = new ConcurrentDictionary(); - _genRefRefs = new ConcurrentQueue(); - _genRefRef = null; // no initial gen exists + _genObjs = new ConcurrentQueue(); + _genObj = null; // no initial gen exists _liveGen = _floorGen = 0; _nextGen = false; // first time, must create a snapshot _collectAuto = true; // collect automatically by default @@ -836,8 +836,8 @@ public Snapshot CreateSnapshot() // if no next generation is required, and we already have one, // use it and create a new snapshot - if (_nextGen == false && _genRefRef != null) - return new Snapshot(this, _genRefRef.GetGenRef() + if (_nextGen == false && _genObj != null) + return new Snapshot(this, _genObj.GetGenRef() #if DEBUG , _logger #endif @@ -852,15 +852,15 @@ public Snapshot CreateSnapshot() var snapGen = _nextGen ? _liveGen - 1 : _liveGen; // create a new gen ref unless we already have it - if (_genRefRef == null) - _genRefRefs.Enqueue(_genRefRef = new GenRefRef(snapGen)); - else if (_genRefRef.Gen != snapGen) + if (_genObj == null) + _genObjs.Enqueue(_genObj = new GenObj(snapGen)); + else if (_genObj.Gen != snapGen) throw new Exception("panic"); } else { // not write-locked, can use latest gen, create a new gen ref - _genRefRefs.Enqueue(_genRefRef = new GenRefRef(_liveGen)); + _genObjs.Enqueue(_genObj = new GenObj(_liveGen)); _nextGen = false; // this is the ONLY thing that triggers a _liveGen++ } @@ -873,7 +873,7 @@ public Snapshot CreateSnapshot() // - the genRefRef weak ref is dead because all snapshots have been collected // in both cases, we will dequeue and collect - var snapshot = new Snapshot(this, _genRefRef.GetGenRef() + var snapshot = new Snapshot(this, _genObj.GetGenRef() #if DEBUG , _logger #endif @@ -930,10 +930,10 @@ private void Collect() #if DEBUG _logger.Debug("Collect."); #endif - while (_genRefRefs.TryPeek(out GenRefRef genRefRef) && (genRefRef.Count == 0 || genRefRef.WGenRef.IsAlive == false)) + while (_genObjs.TryPeek(out var genObj) && (genObj.Count == 0 || genObj.WeakGenRef.IsAlive == false)) { - _genRefRefs.TryDequeue(out genRefRef); // cannot fail since TryPeek has succeeded - _floorGen = genRefRef.Gen; + _genObjs.TryDequeue(out genObj); // cannot fail since TryPeek has succeeded + _floorGen = genObj.Gen; #if DEBUG //_logger.Debug("_floorGen=" + _floorGen + ", _liveGen=" + _liveGen); #endif @@ -1009,9 +1009,9 @@ public async Task WaitForPendingCollect() await task; } - public long GenCount => _genRefRefs.Count; + public long GenCount => _genObjs.Count; - public long SnapCount => _genRefRefs.Sum(x => x.Count); + public long SnapCount => _genObjs.Sum(x => x.Count); #endregion @@ -1100,7 +1100,7 @@ internal Snapshot(ContentStore store, GenRef genRef _store = store; _genRef = genRef; _gen = genRef.Gen; - Interlocked.Increment(ref genRef.GenRefRef.Count); + Interlocked.Increment(ref genRef.GenObj.Count); //_thisCount = _count++; #if DEBUG @@ -1201,46 +1201,46 @@ public void Dispose() { if (_gen < 0) return; #if DEBUG - _logger.Debug("Dispose snapshot ({Snapshot})", _genRef?.GenRefRef.Count.ToString() ?? "live"); + _logger.Debug("Dispose snapshot ({Snapshot})", _genRef?.GenObj.Count.ToString() ?? "live"); #endif _gen = -1; if (_genRef != null) - Interlocked.Decrement(ref _genRef.GenRefRef.Count); + Interlocked.Decrement(ref _genRef.GenObj.Count); GC.SuppressFinalize(this); } } - internal class GenRefRef + internal class GenObj { - public GenRefRef(long gen) + public GenObj(long gen) { Gen = gen; - WGenRef = new WeakReference(null); + WeakGenRef = new WeakReference(null); } public GenRef GetGenRef() { // not thread-safe but always invoked from within a lock - var genRef = (GenRef) WGenRef.Target; + var genRef = (GenRef) WeakGenRef.Target; if (genRef == null) - WGenRef.Target = genRef = new GenRef(this, Gen); + WeakGenRef.Target = genRef = new GenRef(this, Gen); return genRef; } public readonly long Gen; - public readonly WeakReference WGenRef; + public readonly WeakReference WeakGenRef; public int Count; } internal class GenRef { - public GenRef(GenRefRef genRefRef, long gen) + public GenRef(GenObj genObj, long gen) { - GenRefRef = genRefRef; + GenObj = genObj; Gen = gen; } - public readonly GenRefRef GenRefRef; + public readonly GenObj GenObj; public readonly long Gen; } diff --git a/src/Umbraco.Web/PublishedCache/NuCache/SnapDictionary.cs b/src/Umbraco.Web/PublishedCache/NuCache/SnapDictionary.cs index 30f6e7e63815..eebbe3122fba 100644 --- a/src/Umbraco.Web/PublishedCache/NuCache/SnapDictionary.cs +++ b/src/Umbraco.Web/PublishedCache/NuCache/SnapDictionary.cs @@ -21,8 +21,8 @@ internal class SnapDictionary // Readers are lock-free private readonly ConcurrentDictionary _items; - private readonly ConcurrentQueue _generationObjects; - private GenerationObject _generationObject; + private readonly ConcurrentQueue _genObjs; + private GenObj _genObj; private readonly object _wlocko = new object(); private readonly object _rlocko = new object(); private long _liveGen, _floorGen; @@ -41,8 +41,8 @@ internal class SnapDictionary public SnapDictionary() { _items = new ConcurrentDictionary(); - _generationObjects = new ConcurrentQueue(); - _generationObject = null; // no initial gen exists + _genObjs = new ConcurrentQueue(); + _genObj = null; // no initial gen exists _liveGen = _floorGen = 0; _nextGen = false; // first time, must create a snapshot _collectAuto = true; // collect automatically by default @@ -339,8 +339,8 @@ public Snapshot CreateSnapshot() // if no next generation is required, and we already have one, // use it and create a new snapshot - if (_nextGen == false && _generationObject != null) - return new Snapshot(this, _generationObject.GetReference()); + if (_nextGen == false && _genObj != null) + return new Snapshot(this, _genObj.GetGenRef()); // else we need to try to create a new gen ref // whether we are wlocked or not, noone can rlock while we do, @@ -351,15 +351,15 @@ public Snapshot CreateSnapshot() var snapGen = _nextGen ? _liveGen - 1 : _liveGen; // create a new gen ref unless we already have it - if (_generationObject == null) - _generationObjects.Enqueue(_generationObject = new GenerationObject(snapGen)); - else if (_generationObject.Gen != snapGen) + if (_genObj == null) + _genObjs.Enqueue(_genObj = new GenObj(snapGen)); + else if (_genObj.Gen != snapGen) throw new Exception("panic"); } else { // not write-locked, can use latest gen, create a new gen ref - _generationObjects.Enqueue(_generationObject = new GenerationObject(_liveGen)); + _genObjs.Enqueue(_genObj = new GenObj(_liveGen)); _nextGen = false; // this is the ONLY thing that triggers a _liveGen++ } @@ -372,7 +372,7 @@ public Snapshot CreateSnapshot() // - the genRefRef weak ref is dead because all snapshots have been collected // in both cases, we will dequeue and collect - var snapshot = new Snapshot(this, _generationObject.GetReference()); + var snapshot = new Snapshot(this, _genObj.GetGenRef()); // reading _floorGen is safe if _collectTask is null if (_collectTask == null && _collectAuto && _liveGen - _floorGen > CollectMinGenDelta) @@ -416,10 +416,10 @@ private Task CollectAsyncLocked() private void Collect() { // see notes in CreateSnapshot - while (_generationObjects.TryPeek(out GenerationObject generationObject) && (generationObject.Count == 0 || generationObject.WeakReference.IsAlive == false)) + while (_genObjs.TryPeek(out var genObj) && (genObj.Count == 0 || genObj.WeakGenRef.IsAlive == false)) { - _generationObjects.TryDequeue(out generationObject); // cannot fail since TryPeek has succeeded - _floorGen = generationObject.Gen; + _genObjs.TryDequeue(out genObj); // cannot fail since TryPeek has succeeded + _floorGen = genObj.Gen; } Collect(_items); @@ -490,9 +490,9 @@ private void Collect(ConcurrentDictionary dict) // await task; } - public long GenCount => _generationObjects.Count; + public long GenCount => _genObjs.Count; - public long SnapCount => _generationObjects.Sum(x => x.Count); + public long SnapCount => _genObjs.Sum(x => x.Count); #endregion @@ -520,7 +520,7 @@ public bool CollectAuto set => _dict._collectAuto = value; } - public ConcurrentQueue GenerationObjects => _dict._generationObjects; + public ConcurrentQueue GenObjs => _dict._genObjs; public Snapshot LiveSnapshot => new Snapshot(_dict, _dict._liveGen); @@ -586,8 +586,8 @@ internal Snapshot(SnapDictionary store, GenerationReference genera { _store = store; _generationReference = generationReference; - _gen = generationReference.GenerationObject.Gen; - _generationReference.GenerationObject.Reference(); + _gen = generationReference.GenObj.Gen; + _generationReference.GenObj.Reference(); } internal Snapshot(SnapDictionary store, long gen) @@ -634,30 +634,30 @@ public void Dispose() { if (_gen < 0) return; _gen = -1; - _generationReference?.GenerationObject.Release(); + _generationReference?.GenObj.Release(); GC.SuppressFinalize(this); } } - internal class GenerationObject + internal class GenObj { - public GenerationObject(long gen) + public GenObj(long gen) { Gen = gen; - WeakReference = new WeakReference(null); + WeakGenRef = new WeakReference(null); } - public GenerationReference GetReference() + public GenerationReference GetGenRef() { // not thread-safe but always invoked from within a lock - var generationReference = (GenerationReference) WeakReference.Target; + var generationReference = (GenerationReference) WeakGenRef.Target; if (generationReference == null) - WeakReference.Target = generationReference = new GenerationReference(this); + WeakGenRef.Target = generationReference = new GenerationReference(this); return generationReference; } public readonly long Gen; - public readonly WeakReference WeakReference; + public readonly WeakReference WeakGenRef; public int Count; public void Reference() @@ -673,12 +673,12 @@ public void Release() internal class GenerationReference { - public GenerationReference(GenerationObject generationObject) + public GenerationReference(GenObj genObj) { - GenerationObject = generationObject; + GenObj = genObj; } - public readonly GenerationObject GenerationObject; + public readonly GenObj GenObj; } #endregion From c56b8863eec4eb64504da614828a35bf4ec53e65 Mon Sep 17 00:00:00 2001 From: Stephan Date: Fri, 22 Feb 2019 16:03:39 +0100 Subject: [PATCH 16/23] NuCache: reorg some classes --- .../Cache/SnapDictionaryTests.cs | 4 +- .../PublishedCache/NuCache/ContentStore.cs | 53 +----------- .../PublishedCache/NuCache/Snap/GenObj.cs | 37 +++++++++ .../PublishedCache/NuCache/Snap/GenRef.cs | 13 +++ .../PublishedCache/NuCache/Snap/LinkedNode.cs | 20 +++++ .../PublishedCache/NuCache/SnapDictionary.cs | 80 +++---------------- src/Umbraco.Web/Umbraco.Web.csproj | 3 + 7 files changed, 87 insertions(+), 123 deletions(-) create mode 100644 src/Umbraco.Web/PublishedCache/NuCache/Snap/GenObj.cs create mode 100644 src/Umbraco.Web/PublishedCache/NuCache/Snap/GenRef.cs create mode 100644 src/Umbraco.Web/PublishedCache/NuCache/Snap/LinkedNode.cs diff --git a/src/Umbraco.Tests/Cache/SnapDictionaryTests.cs b/src/Umbraco.Tests/Cache/SnapDictionaryTests.cs index 0ee0d180c3c8..82c27ebc38b6 100644 --- a/src/Umbraco.Tests/Cache/SnapDictionaryTests.cs +++ b/src/Umbraco.Tests/Cache/SnapDictionaryTests.cs @@ -5,6 +5,7 @@ using NUnit.Framework; using Umbraco.Core.Scoping; using Umbraco.Web.PublishedCache.NuCache; +using Umbraco.Web.PublishedCache.NuCache.Snap; namespace Umbraco.Tests.Cache { @@ -388,8 +389,7 @@ public async Task EventuallyCollectNulls() // collect liveGen GC.Collect(); - SnapDictionary.GenObj genObj; - Assert.IsTrue(d.Test.GenObjs.TryPeek(out genObj)); + Assert.IsTrue(d.Test.GenObjs.TryPeek(out var genObj)); genObj = null; // in Release mode, it works, but in Debug mode, the weak reference is still alive diff --git a/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs b/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs index e78c4d419e88..dd5805daa1fb 100644 --- a/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs +++ b/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs @@ -8,6 +8,7 @@ using Umbraco.Core.Logging; using Umbraco.Core.Models.PublishedContent; using Umbraco.Core.Scoping; +using Umbraco.Web.PublishedCache.NuCache.Snap; namespace Umbraco.Web.PublishedCache.NuCache { @@ -1061,24 +1062,6 @@ public Tuple[] GetValues(int id) #region Classes - private class LinkedNode - where TValue: class - { - public LinkedNode(TValue value, long gen, LinkedNode next = null) - { - Value = value; - Gen = gen; - Next = next; - } - - internal readonly long Gen; - - // reading & writing references is thread-safe on all .NET platforms - // mark as volatile to ensure we always read the correct value - internal volatile TValue Value; - internal volatile LinkedNode Next; - } - public class Snapshot : IDisposable { private readonly ContentStore _store; @@ -1210,40 +1193,6 @@ public void Dispose() } } - internal class GenObj - { - public GenObj(long gen) - { - Gen = gen; - WeakGenRef = new WeakReference(null); - } - - public GenRef GetGenRef() - { - // not thread-safe but always invoked from within a lock - var genRef = (GenRef) WeakGenRef.Target; - if (genRef == null) - WeakGenRef.Target = genRef = new GenRef(this, Gen); - return genRef; - } - - public readonly long Gen; - public readonly WeakReference WeakGenRef; - public int Count; - } - - internal class GenRef - { - public GenRef(GenObj genObj, long gen) - { - GenObj = genObj; - Gen = gen; - } - - public readonly GenObj GenObj; - public readonly long Gen; - } - #endregion } } diff --git a/src/Umbraco.Web/PublishedCache/NuCache/Snap/GenObj.cs b/src/Umbraco.Web/PublishedCache/NuCache/Snap/GenObj.cs new file mode 100644 index 000000000000..b69dab7dac48 --- /dev/null +++ b/src/Umbraco.Web/PublishedCache/NuCache/Snap/GenObj.cs @@ -0,0 +1,37 @@ +using System; +using System.Threading; + +namespace Umbraco.Web.PublishedCache.NuCache.Snap +{ + internal class GenObj + { + public GenObj(long gen) + { + Gen = gen; + WeakGenRef = new WeakReference(null); + } + + public GenRef GetGenRef() + { + // not thread-safe but always invoked from within a lock + var genRef = (GenRef)WeakGenRef.Target; + if (genRef == null) + WeakGenRef.Target = genRef = new GenRef(this); + return genRef; + } + + public readonly long Gen; + public readonly WeakReference WeakGenRef; + public int Count; + + public void Reference() + { + Interlocked.Increment(ref Count); + } + + public void Release() + { + Interlocked.Decrement(ref Count); + } + } +} diff --git a/src/Umbraco.Web/PublishedCache/NuCache/Snap/GenRef.cs b/src/Umbraco.Web/PublishedCache/NuCache/Snap/GenRef.cs new file mode 100644 index 000000000000..ade0251b8d6f --- /dev/null +++ b/src/Umbraco.Web/PublishedCache/NuCache/Snap/GenRef.cs @@ -0,0 +1,13 @@ +namespace Umbraco.Web.PublishedCache.NuCache.Snap +{ + internal class GenRef + { + public GenRef(GenObj genObj) + { + GenObj = genObj; + } + + public readonly GenObj GenObj; + public long Gen => GenObj.Gen; + } +} diff --git a/src/Umbraco.Web/PublishedCache/NuCache/Snap/LinkedNode.cs b/src/Umbraco.Web/PublishedCache/NuCache/Snap/LinkedNode.cs new file mode 100644 index 000000000000..20d7e7ddcd2e --- /dev/null +++ b/src/Umbraco.Web/PublishedCache/NuCache/Snap/LinkedNode.cs @@ -0,0 +1,20 @@ +namespace Umbraco.Web.PublishedCache.NuCache.Snap +{ + internal class LinkedNode + where TValue : class + { + public LinkedNode(TValue value, long gen, LinkedNode next = null) + { + Value = value; + Gen = gen; + Next = next; + } + + public readonly long Gen; + + // reading & writing references is thread-safe on all .NET platforms + // mark as volatile to ensure we always read the correct value + public volatile TValue Value; + public volatile LinkedNode Next; + } +} \ No newline at end of file diff --git a/src/Umbraco.Web/PublishedCache/NuCache/SnapDictionary.cs b/src/Umbraco.Web/PublishedCache/NuCache/SnapDictionary.cs index eebbe3122fba..f117a395b5e3 100644 --- a/src/Umbraco.Web/PublishedCache/NuCache/SnapDictionary.cs +++ b/src/Umbraco.Web/PublishedCache/NuCache/SnapDictionary.cs @@ -5,6 +5,7 @@ using System.Threading; using System.Threading.Tasks; using Umbraco.Core.Scoping; +using Umbraco.Web.PublishedCache.NuCache.Snap; namespace Umbraco.Web.PublishedCache.NuCache { @@ -20,7 +21,7 @@ internal class SnapDictionary // This class is optimized for many readers, few writers // Readers are lock-free - private readonly ConcurrentDictionary _items; + private readonly ConcurrentDictionary> _items; private readonly ConcurrentQueue _genObjs; private GenObj _genObj; private readonly object _wlocko = new object(); @@ -40,7 +41,7 @@ internal class SnapDictionary public SnapDictionary() { - _items = new ConcurrentDictionary(); + _items = new ConcurrentDictionary>(); _genObjs = new ConcurrentQueue(); _genObj = null; // no initial gen exists _liveGen = _floorGen = 0; @@ -198,9 +199,9 @@ private void Release(ReadLockInfo lockInfo) public int Count => _items.Count; - private LinkedNode GetHead(TKey key) + private LinkedNode GetHead(TKey key) { - _items.TryGetValue(key, out LinkedNode link); // else null + _items.TryGetValue(key, out var link); // else null return link; } @@ -221,7 +222,7 @@ public void Set(TKey key, TValue value) // for an older gen - if value is different then insert a new // link for the new gen, with the new value if (link.Value != value) - _items.TryUpdate(key, new LinkedNode(value, _liveGen, link), link); + _items.TryUpdate(key, new LinkedNode(value, _liveGen, link), link); } else { @@ -235,7 +236,7 @@ public void Set(TKey key, TValue value) } else { - _items.TryAdd(key, new LinkedNode(value, _liveGen)); + _items.TryAdd(key, new LinkedNode(value, _liveGen)); } } finally @@ -261,7 +262,7 @@ public void Clear() { if (kvp.Value.Gen < _liveGen) { - var link = new LinkedNode(null, _liveGen, kvp.Value); + var link = new LinkedNode(null, _liveGen, kvp.Value); _items.TryUpdate(kvp.Key, link, kvp.Value); } else @@ -425,7 +426,7 @@ private void Collect() Collect(_items); } - private void Collect(ConcurrentDictionary dict) + private void Collect(ConcurrentDictionary> dict) { // it is OK to enumerate a concurrent dictionary and it does not lock // it - and here it's not an issue if we skip some items, they will be @@ -460,7 +461,7 @@ private void Collect(ConcurrentDictionary dict) // not live, null value, no next link = remove that one -- but only if // the dict has not been updated, have to do it via ICollection<> (thanks // Mr Toub) -- and if the dict has been updated there is nothing to collect - var idict = dict as ICollection>; + var idict = dict as ICollection>>; /*var removed =*/ idict.Remove(kvp); //Console.WriteLine("remove (" + (removed ? "true" : "false") + ")"); continue; @@ -526,7 +527,7 @@ public bool CollectAuto public GenVal[] GetValues(TKey key) { - _dict._items.TryGetValue(key, out LinkedNode link); // else null + _dict._items.TryGetValue(key, out var link); // else null if (link == null) return new GenVal[0]; @@ -559,23 +560,6 @@ public GenVal(long gen, TValue value) #region Classes - private class LinkedNode - { - public LinkedNode(TValue value, long gen, LinkedNode next = null) - { - Value = value; - Gen = gen; - Next = next; - } - - internal readonly long Gen; - - // reading & writing references is thread-safe on all .NET platforms - // mark as volatile to ensure we always read the correct value - internal volatile TValue Value; - internal volatile LinkedNode Next; - } - public class Snapshot : IDisposable { private readonly SnapDictionary _store; @@ -639,48 +623,6 @@ public void Dispose() } } - internal class GenObj - { - public GenObj(long gen) - { - Gen = gen; - WeakGenRef = new WeakReference(null); - } - - public GenerationReference GetGenRef() - { - // not thread-safe but always invoked from within a lock - var generationReference = (GenerationReference) WeakGenRef.Target; - if (generationReference == null) - WeakGenRef.Target = generationReference = new GenerationReference(this); - return generationReference; - } - - public readonly long Gen; - public readonly WeakReference WeakGenRef; - public int Count; - - public void Reference() - { - Interlocked.Increment(ref Count); - } - - public void Release() - { - Interlocked.Decrement(ref Count); - } - } - - internal class GenerationReference - { - public GenerationReference(GenObj genObj) - { - GenObj = genObj; - } - - public readonly GenObj GenObj; - } - #endregion } } diff --git a/src/Umbraco.Web/Umbraco.Web.csproj b/src/Umbraco.Web/Umbraco.Web.csproj index 09cc7d856a0a..c6cbc7cbaae9 100755 --- a/src/Umbraco.Web/Umbraco.Web.csproj +++ b/src/Umbraco.Web/Umbraco.Web.csproj @@ -205,6 +205,9 @@ + + + From 7e413ed4060b5aab7c4197be79214a77e8e0f050 Mon Sep 17 00:00:00 2001 From: Stephan Date: Fri, 22 Feb 2019 15:43:37 +0100 Subject: [PATCH 17/23] NuCache: troubleshooting --- .../Cache/SnapDictionaryTests.cs | 174 ++++++++++++++++-- .../PublishedCache/NuCache/ContentStore.cs | 7 +- .../PublishedCache/NuCache/SnapDictionary.cs | 105 +++++++---- 3 files changed, 233 insertions(+), 53 deletions(-) diff --git a/src/Umbraco.Tests/Cache/SnapDictionaryTests.cs b/src/Umbraco.Tests/Cache/SnapDictionaryTests.cs index 82c27ebc38b6..eb034eec2674 100644 --- a/src/Umbraco.Tests/Cache/SnapDictionaryTests.cs +++ b/src/Umbraco.Tests/Cache/SnapDictionaryTests.cs @@ -712,16 +712,102 @@ public void WriteLocking() } [Test] - public void NestedWriteLocking() + public void NestedWriteLocking1() { var d = new SnapDictionary(); - d.Test.CollectAuto = false; + var t = d.Test; + t.CollectAuto = false; + + Assert.AreEqual(0, d.CreateSnapshot().Gen); + + // no scope context: writers nest, last one to be disposed commits var scopeProvider = GetScopeProvider(); - using (d.GetWriter(scopeProvider)) + + using (var w1 = d.GetWriter(scopeProvider)) + { + Assert.AreEqual(1, t.LiveGen); + Assert.AreEqual(1, t.WLocked); + Assert.IsTrue(t.NextGen); + + using (var w2 = d.GetWriter(scopeProvider)) + { + Assert.AreEqual(1, t.LiveGen); + Assert.AreEqual(2, t.WLocked); + Assert.IsTrue(t.NextGen); + + Assert.AreNotSame(w1, w2); // get a new writer each time + + d.Set(1, "one"); + + Assert.AreEqual(0, d.CreateSnapshot().Gen); + } + + Assert.AreEqual(1, t.LiveGen); + Assert.AreEqual(1, t.WLocked); + Assert.IsTrue(t.NextGen); + + Assert.AreEqual(0, d.CreateSnapshot().Gen); + } + + Assert.AreEqual(1, t.LiveGen); + Assert.AreEqual(0, t.WLocked); + Assert.IsTrue(t.NextGen); + + Assert.AreEqual(1, d.CreateSnapshot().Gen); + } + + [Test] + public void NestedWriteLocking2() + { + var d = new SnapDictionary(); + d.Test.CollectAuto = false; + + Assert.AreEqual(0, d.CreateSnapshot().Gen); + + // scope context: writers enlist + + var scopeContext = new ScopeContext(); + var scopeProvider = GetScopeProvider(scopeContext); + + using (var w1 = d.GetWriter(scopeProvider)) { - using (d.GetWriter(scopeProvider)) + using (var w2 = d.GetWriter(scopeProvider)) { + Assert.AreSame(w1, w2); + + d.Set(1, "one"); + } + } + } + + [Test] + public void NestedWriteLocking3() + { + var d = new SnapDictionary(); + var t = d.Test; + t.CollectAuto = false; + + Assert.AreEqual(0, d.CreateSnapshot().Gen); + + var scopeContext = new ScopeContext(); + var scopeProvider1 = GetScopeProvider(); + var scopeProvider2 = GetScopeProvider(scopeContext); + + using (var w1 = d.GetWriter(scopeProvider1)) + { + Assert.AreEqual(1, t.LiveGen); + Assert.AreEqual(1, t.WLocked); + Assert.IsTrue(t.NextGen); + + using (var w2 = d.GetWriter(scopeProvider2)) + { + Assert.AreEqual(1, t.LiveGen); + Assert.AreEqual(2, t.WLocked); + Assert.IsTrue(t.NextGen); + + Assert.AreNotSame(w1, w2); + d.Set(1, "one"); } } @@ -846,7 +932,8 @@ public void ScopeLocking1() Assert.AreEqual(2, s2.Gen); Assert.AreEqual("uno", s2.Get(1)); - var scopeProvider = GetScopeProvider(true); + var scopeContext = new ScopeContext(); + var scopeProvider = GetScopeProvider(scopeContext); using (d.GetWriter(scopeProvider)) { @@ -867,7 +954,7 @@ public void ScopeLocking1() Assert.AreEqual(2, s4.Gen); Assert.AreEqual("uno", s4.Get(1)); - ((ScopeContext) scopeProvider.Context).ScopeExit(true); + scopeContext.ScopeExit(true); var s5 = d.CreateSnapshot(); Assert.AreEqual(3, s5.Gen); @@ -878,7 +965,8 @@ public void ScopeLocking1() public void ScopeLocking2() { var d = new SnapDictionary(); - d.Test.CollectAuto = false; + var t = d.Test; + t.CollectAuto = false; // gen 1 d.Set(1, "one"); @@ -891,10 +979,11 @@ public void ScopeLocking2() Assert.AreEqual(2, s2.Gen); Assert.AreEqual("uno", s2.Get(1)); - var scopeProviderMock = new Mock(); + Assert.AreEqual(2, t.LiveGen); + Assert.IsFalse(t.NextGen); + var scopeContext = new ScopeContext(); - scopeProviderMock.Setup(x => x.Context).Returns(scopeContext); - var scopeProvider = scopeProviderMock.Object; + var scopeProvider = GetScopeProvider(scopeContext); using (d.GetWriter(scopeProvider)) { @@ -905,18 +994,72 @@ public void ScopeLocking2() Assert.AreEqual(2, s3.Gen); Assert.AreEqual("uno", s3.Get(1)); + // we made some changes, so a next gen is required + Assert.AreEqual(3, t.LiveGen); + Assert.IsTrue(t.NextGen); + Assert.AreEqual(1, t.WLocked); + // but live snapshot contains changes - var ls = d.Test.LiveSnapshot; + var ls = t.LiveSnapshot; Assert.AreEqual("ein", ls.Get(1)); Assert.AreEqual(3, ls.Gen); } + // nothing is committed until scope exits + Assert.AreEqual(3, t.LiveGen); + Assert.IsTrue(t.NextGen); + Assert.AreEqual(1, t.WLocked); + + // no changes until exit var s4 = d.CreateSnapshot(); Assert.AreEqual(2, s4.Gen); Assert.AreEqual("uno", s4.Get(1)); + // fixme - remove debugging code + /* + Exception caught = null; + var genFlip = 0; + var lckFlip = 0; + var thread = new System.Threading.Thread(() => + { + try + { + for (var i = 0; i < 20; i++) + { + if (t.LiveGen == 2 && genFlip == 0) genFlip = i; // flips at 1 + if (t.WLocked == 0 && lckFlip == 0) lckFlip = i; // flips at 10 ie 5s, as expected + d.CreateSnapshot(); + System.Threading.Thread.Sleep(500); + } + } + catch (Exception e) + { + caught = e; + } + }); + thread.Start(); + */ + scopeContext.ScopeExit(false); + // fixme - remove debugging code + /* + thread.Join(); + + Assert.IsNull(caught); // but then how can it be not null? + + Console.WriteLine(genFlip); + Console.WriteLine(lckFlip); + Assert.AreEqual(1, genFlip); + Assert.AreEqual(10, lckFlip); + */ + + // now things have changed + Assert.AreEqual(2, t.LiveGen); + Assert.IsFalse(t.NextGen); + Assert.AreEqual(0, t.WLocked); + + // no changes since not completed var s5 = d.CreateSnapshot(); Assert.AreEqual(2, s5.Gen); Assert.AreEqual("uno", s5.Get(1)); @@ -955,12 +1098,11 @@ public void GetAll() Assert.AreEqual("four", all[3]); } - private IScopeProvider GetScopeProvider(bool withContext = false) + private IScopeProvider GetScopeProvider(ScopeContext scopeContext = null) { - var scopeProviderMock = new Mock(); - var scopeContext = withContext ? new ScopeContext() : null; - scopeProviderMock.Setup(x => x.Context).Returns(scopeContext); - var scopeProvider = scopeProviderMock.Object; + var scopeProvider = Mock.Of(); + Mock.Get(scopeProvider) + .Setup(x => x.Context).Returns(scopeContext); return scopeProvider; } } diff --git a/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs b/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs index dd5805daa1fb..2548046e2301 100644 --- a/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs +++ b/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs @@ -95,7 +95,8 @@ private class WriteLockInfo private class ContentStoreWriter : ScopeContextualBase { private readonly WriteLockInfo _lockinfo = new WriteLockInfo(); - private ContentStore _store; + private readonly ContentStore _store; + private int _released; public ContentStoreWriter(ContentStore store, bool scoped) { @@ -105,9 +106,9 @@ public ContentStoreWriter(ContentStore store, bool scoped) public override void Release(bool completed) { - if (_store== null) return; + if (Interlocked.CompareExchange(ref _released, 1, 0) != 0) + return; _store.Release(_lockinfo, completed); - _store = null; } } diff --git a/src/Umbraco.Web/PublishedCache/NuCache/SnapDictionary.cs b/src/Umbraco.Web/PublishedCache/NuCache/SnapDictionary.cs index f117a395b5e3..1d462f1b767b 100644 --- a/src/Umbraco.Web/PublishedCache/NuCache/SnapDictionary.cs +++ b/src/Umbraco.Web/PublishedCache/NuCache/SnapDictionary.cs @@ -90,7 +90,8 @@ private class WriteLockInfo private class SnapDictionaryWriter : ScopeContextualBase { private readonly WriteLockInfo _lockinfo = new WriteLockInfo(); - private SnapDictionary _dictionary; + private readonly SnapDictionary _dictionary; + private int _released; public SnapDictionaryWriter(SnapDictionary dictionary, bool scoped) { @@ -100,14 +101,18 @@ public SnapDictionaryWriter(SnapDictionary dictionary, bool scoped public override void Release(bool completed) { - if (_dictionary == null) return; + if (Interlocked.CompareExchange(ref _released, 1, 0) != 0) + return; _dictionary.Release(_lockinfo, completed); - _dictionary = null; } } // gets a scope contextual representing a locked writer to the dictionary - // GetScopedWriter? should the dict have a ref onto the scope provider? + // fixme GetScopedWriter? should the dict have a ref onto the scope provider? + // fixme this is not a "writer" but a "write lock" => rename GetWriteLock + // the dict is write-locked until the write-lock is released + // which happens when it is disposed (non-scoped) + // or when the scope context exits (scoped) public IDisposable GetWriter(IScopeProvider scopeProvider) { return ScopeContextualBase.Get(scopeProvider, _instanceId, scoped => new SnapDictionaryWriter(this, scoped)); @@ -130,13 +135,22 @@ private void Lock(WriteLockInfo lockInfo, bool forceGen = false) //RuntimeHelpers.PrepareConstrainedRegions(); try { } finally { + // increment the lock count, and register that this lock is counting _wlocked++; lockInfo.Count = true; + + // fixme - this comment: "ok to have holes in generation objects" is annoying + // 'cos if you have a hole, then doing _liveGen-1 in some places would be wrong + // besides, forceGen is used only when getting a scoped write lock, which is not reentrant? + // + // but... if _wlocked ==1 and we just incremented it, it means it was 0, so it wasnt locked, so wtf? + // this is the only place where _nextGen would turn true so ... this makes no sense at all! + if (_nextGen == false || (forceGen && _wlocked == 1)) // if true already... ok to have "holes" in generation objects { // because we are changing things, a new generation // is created, which will trigger a new snapshot - _nextGen = true; + _nextGen = true; // this is the ONLY place where _nextGen becomes true _liveGen += 1; } } @@ -154,6 +168,10 @@ private void Lock(ReadLockInfo lockInfo) private void Release(WriteLockInfo lockInfo, bool commit = true) { + // if the lock wasn't taken in the first place, do nothing + if (!lockInfo.Taken) + return; + if (commit == false) { var rtaken = false; @@ -162,6 +180,7 @@ private void Release(WriteLockInfo lockInfo, bool commit = true) Monitor.Enter(_rlocko, ref rtaken); try { } finally { + // forget about the temp. liveGen _nextGen = false; _liveGen -= 1; } @@ -184,8 +203,12 @@ private void Release(WriteLockInfo lockInfo, bool commit = true) } } + // fixme - pretend we need to do something that takes time + //System.Threading.Thread.Sleep(TimeSpan.FromSeconds(5)); + + // decrement the lock count, if counting, then exit the lock if (lockInfo.Count) _wlocked--; - if (lockInfo.Taken) Monitor.Exit(_wlocko); + Monitor.Exit(_wlocko); } private void Release(ReadLockInfo lockInfo) @@ -338,12 +361,12 @@ public Snapshot CreateSnapshot() { Lock(lockInfo); - // if no next generation is required, and we already have one, - // use it and create a new snapshot + // if no next generation is required, and we already have a gen object, + // use it to create a new snapshot if (_nextGen == false && _genObj != null) return new Snapshot(this, _genObj.GetGenRef()); - // else we need to try to create a new gen ref + // else we need to try to create a new gen object // whether we are wlocked or not, noone can rlock while we do, // so _liveGen and _nextGen are safe if (_wlocked > 0) // volatile, cannot ++ but could -- @@ -351,26 +374,32 @@ public Snapshot CreateSnapshot() // write-locked, cannot use latest gen (at least 1) so use previous var snapGen = _nextGen ? _liveGen - 1 : _liveGen; - // create a new gen ref unless we already have it + // create a new gen object if we don't already have one + // (happens the first time a snapshot is created) if (_genObj == null) _genObjs.Enqueue(_genObj = new GenObj(snapGen)); + + // fixme - getting a panic exception here + // means _genObj != null, means _nextGen == true + + // if we have one already, ensure it's consistent else if (_genObj.Gen != snapGen) throw new Exception("panic"); } else { - // not write-locked, can use latest gen, create a new gen ref + // not write-locked, can use latest gen (_liveGen), create a corresponding new gen object _genObjs.Enqueue(_genObj = new GenObj(_liveGen)); _nextGen = false; // this is the ONLY thing that triggers a _liveGen++ } // so... - // the genRefRef has a weak ref to the genRef, and is queued - // the snapshot has a ref to the genRef, which has a ref to the genRefRef - // when the snapshot is disposed, it decreases genRefRef counter + // the genObj has a weak ref to the genRef, and is queued + // the snapshot has a ref to the genRef, which has a ref to the genObj + // when the snapshot is disposed, it decreases genObj counter // so after a while, one of these conditions is going to be true: - // - the genRefRef counter is zero because all snapshots have properly been disposed - // - the genRefRef weak ref is dead because all snapshots have been collected + // - genObj.Count is zero because all snapshots have properly been disposed + // - genObj.WeakGenRef is dead because all snapshots have been collected // in both cases, we will dequeue and collect var snapshot = new Snapshot(this, _genObj.GetGenRef()); @@ -486,7 +515,7 @@ private void Collect(ConcurrentDictionary> dict) { task = _collectTask; } - return task ?? Task.FromResult(0); + return task ?? Task.CompletedTask; //if (task != null) // await task; } @@ -514,6 +543,7 @@ public TestHelper(SnapDictionary dict) public long LiveGen => _dict._liveGen; public long FloorGen => _dict._floorGen; public bool NextGen => _dict._nextGen; + public int WLocked => _dict._wlocked; public bool CollectAuto { @@ -563,15 +593,20 @@ public GenVal(long gen, TValue value) public class Snapshot : IDisposable { private readonly SnapDictionary _store; - private readonly GenerationReference _generationReference; - private long _gen; // copied for perfs + private readonly GenRef _genRef; + private readonly long _gen; // copied for perfs + private int _disposed; + + //private static int _count; + //private readonly int _thisCount; - internal Snapshot(SnapDictionary store, GenerationReference generationReference) + internal Snapshot(SnapDictionary store, GenRef genRef) { _store = store; - _generationReference = generationReference; - _gen = generationReference.GenObj.Gen; - _generationReference.GenObj.Reference(); + _genRef = genRef; + _gen = genRef.GenObj.Gen; + _genRef.GenObj.Reference(); + //_thisCount = _count++; } internal Snapshot(SnapDictionary store, long gen) @@ -580,17 +615,21 @@ internal Snapshot(SnapDictionary store, long gen) _gen = gen; } - public TValue Get(TKey key) + private void EnsureNotDisposed() { - if (_gen < 0) + if (_disposed > 0) throw new ObjectDisposedException("snapshot" /*+ " (" + _thisCount + ")"*/); + } + + public TValue Get(TKey key) + { + EnsureNotDisposed(); return _store.Get(key, _gen); } public IEnumerable GetAll() { - if (_gen < 0) - throw new ObjectDisposedException("snapshot" /*+ " (" + _thisCount + ")"*/); + EnsureNotDisposed(); return _store.GetAll(_gen); } @@ -598,8 +637,7 @@ public bool IsEmpty { get { - if (_gen < 0) - throw new ObjectDisposedException("snapshot" /*+ " (" + _thisCount + ")"*/); + EnsureNotDisposed(); return _store.IsEmpty(_gen); } } @@ -608,17 +646,16 @@ public long Gen { get { - if (_gen < 0) - throw new ObjectDisposedException("snapshot" /*+ " (" + _thisCount + ")"*/); + EnsureNotDisposed(); return _gen; } } public void Dispose() { - if (_gen < 0) return; - _gen = -1; - _generationReference?.GenObj.Release(); + if (Interlocked.CompareExchange(ref _disposed, 1, 0) != 0) + return; + _genRef?.GenObj.Release(); GC.SuppressFinalize(this); } } From 851c844c8b3268909b0939bb3ec8a5202ea554c5 Mon Sep 17 00:00:00 2001 From: Stephan Date: Thu, 14 Mar 2019 18:56:23 +0100 Subject: [PATCH 18/23] NuCache: fix panic exception --- .../Cache/SnapDictionaryTests.cs | 81 +++++++++++++++++++ .../PublishedCache/NuCache/ContentStore.cs | 4 +- .../PublishedCache/NuCache/SnapDictionary.cs | 15 +--- 3 files changed, 87 insertions(+), 13 deletions(-) diff --git a/src/Umbraco.Tests/Cache/SnapDictionaryTests.cs b/src/Umbraco.Tests/Cache/SnapDictionaryTests.cs index eb034eec2674..e4ca35fbd360 100644 --- a/src/Umbraco.Tests/Cache/SnapDictionaryTests.cs +++ b/src/Umbraco.Tests/Cache/SnapDictionaryTests.cs @@ -1098,6 +1098,87 @@ public void GetAll() Assert.AreEqual("four", all[3]); } + [Test] + public void DontPanic() + { + var d = new SnapDictionary(); + d.Test.CollectAuto = false; + + Assert.IsNull(d.Test.GenObj); // set with first snapshot or first lock, then never null + + // gen 1 + d.Set(1, "one"); + Assert.IsTrue(d.Test.NextGen); + Assert.AreEqual(1, d.Test.LiveGen); + Assert.IsNotNull(d.Test.GenObj); // set with lock + + var s1 = d.CreateSnapshot(); + Assert.IsFalse(d.Test.NextGen); + Assert.AreEqual(1, d.Test.LiveGen); + Assert.IsNotNull(d.Test.GenObj); + Assert.AreEqual(1, d.Test.GenObj.Gen); + + Assert.AreEqual(1, s1.Gen); + Assert.AreEqual("one", s1.Get(1)); + + d.Set(1, "uno"); + Assert.IsTrue(d.Test.NextGen); + Assert.AreEqual(2, d.Test.LiveGen); + Assert.IsNotNull(d.Test.GenObj); + Assert.AreEqual(1, d.Test.GenObj.Gen); + + var scopeContext = new ScopeContext(); + var scopeProvider = GetScopeProvider(scopeContext); + + // scopeProvider.Context == scopeContext -> writer is scoped + // writer is scope contextual and scoped + // when disposed, nothing happens + // when the context exists, the writer is released + using (d.GetWriter(scopeProvider)) + { + d.Set(1, "ein"); + Assert.IsTrue(d.Test.NextGen); + Assert.AreEqual(3, d.Test.LiveGen); + Assert.IsNotNull(d.Test.GenObj); + Assert.AreEqual(2, d.Test.GenObj.Gen); + } + + // writer has not released + Assert.AreEqual(1, d.Test.WLocked); + Assert.IsNotNull(d.Test.GenObj); + Assert.AreEqual(2, d.Test.GenObj.Gen); + + // nothing changed + Assert.IsTrue(d.Test.NextGen); + Assert.AreEqual(3, d.Test.LiveGen); + + // panic! + var s2 = d.CreateSnapshot(); + + Assert.AreEqual(1, d.Test.WLocked); + Assert.IsNotNull(d.Test.GenObj); + Assert.AreEqual(2, d.Test.GenObj.Gen); + Assert.AreEqual(3, d.Test.LiveGen); + Assert.IsTrue(d.Test.NextGen); + + // release writer + scopeContext.ScopeExit(true); + + Assert.AreEqual(0, d.Test.WLocked); + Assert.IsNotNull(d.Test.GenObj); + Assert.AreEqual(2, d.Test.GenObj.Gen); + Assert.AreEqual(3, d.Test.LiveGen); + Assert.IsTrue(d.Test.NextGen); + + var s3 = d.CreateSnapshot(); + + Assert.AreEqual(0, d.Test.WLocked); + Assert.IsNotNull(d.Test.GenObj); + Assert.AreEqual(3, d.Test.GenObj.Gen); + Assert.AreEqual(3, d.Test.LiveGen); + Assert.IsFalse(d.Test.NextGen); + } + private IScopeProvider GetScopeProvider(ScopeContext scopeContext = null) { var scopeProvider = Mock.Of(); diff --git a/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs b/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs index 2548046e2301..2b9c9bee4db5 100644 --- a/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs +++ b/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs @@ -133,11 +133,12 @@ private void Lock(WriteLockInfo lockInfo, bool forceGen = false) { _wlocked++; lockInfo.Count = true; - if (_nextGen == false || (forceGen && _wlocked == 1)) // if true already... ok to have "holes" in generation objects + if (_nextGen == false || (forceGen && _wlocked == 1)) { // because we are changing things, a new generation // is created, which will trigger a new snapshot _nextGen = true; + _genObjs.Enqueue(_genObj = new GenObj(_liveGen)); _liveGen += 1; } } @@ -214,7 +215,6 @@ private void Rollback(ConcurrentDictionary _dict._collectAuto = value; } + public GenObj GenObj => _dict._genObj; + public ConcurrentQueue GenObjs => _dict._genObjs; public Snapshot LiveSnapshot => new Snapshot(_dict, _dict._liveGen); From dfe6f2029ac5d65aa8bac1c91f27bc0da5ca5253 Mon Sep 17 00:00:00 2001 From: Stephan Date: Thu, 14 Mar 2019 19:48:44 +0100 Subject: [PATCH 19/23] NuCache: better fixing, cleanup --- .../Scoping/ScopeContextualBase.cs | 7 +- .../Cache/SnapDictionaryTests.cs | 67 +++++-------------- .../PublishedCache/NuCache/ContentStore.cs | 13 ++-- .../NuCache/PublishedSnapshotService.cs | 18 ++--- .../PublishedCache/NuCache/SnapDictionary.cs | 18 ++--- 5 files changed, 39 insertions(+), 84 deletions(-) diff --git a/src/Umbraco.Core/Scoping/ScopeContextualBase.cs b/src/Umbraco.Core/Scoping/ScopeContextualBase.cs index 1f2b6155e692..25f176d4719a 100644 --- a/src/Umbraco.Core/Scoping/ScopeContextualBase.cs +++ b/src/Umbraco.Core/Scoping/ScopeContextualBase.cs @@ -12,7 +12,7 @@ namespace Umbraco.Core.Scoping /// public abstract class ScopeContextualBase : IDisposable { - private bool _using, _scoped; + private bool _scoped; /// /// Gets a contextual object. @@ -38,9 +38,6 @@ public static T Get(IScopeProvider scopeProvider, string key, Func c () => ctor(true), (completed, item) => { item.Release(completed); }); - // the object can be 'used' only once at a time - if (w._using) throw new InvalidOperationException("panic: used."); - w._using = true; w._scoped = true; return w; @@ -52,8 +49,6 @@ public static T Get(IScopeProvider scopeProvider, string key, Func c /// public void Dispose() { - _using = false; - if (_scoped == false) Release(true); } diff --git a/src/Umbraco.Tests/Cache/SnapDictionaryTests.cs b/src/Umbraco.Tests/Cache/SnapDictionaryTests.cs index e4ca35fbd360..b435af9e7721 100644 --- a/src/Umbraco.Tests/Cache/SnapDictionaryTests.cs +++ b/src/Umbraco.Tests/Cache/SnapDictionaryTests.cs @@ -632,7 +632,7 @@ public void WriteLockingFirstSnapshot() Assert.AreEqual(1, d.Test.LiveGen); Assert.IsTrue(d.Test.NextGen); - using (d.GetWriter(GetScopeProvider())) + using (d.GetScopedWriteLock(GetScopeProvider())) { var s1 = d.CreateSnapshot(); @@ -685,7 +685,7 @@ public void WriteLocking() Assert.IsFalse(d.Test.NextGen); Assert.AreEqual("uno", s2.Get(1)); - using (d.GetWriter(GetScopeProvider())) + using (d.GetScopedWriteLock(GetScopeProvider())) { // gen 3 Assert.AreEqual(2, d.Test.GetValues(1).Length); @@ -724,13 +724,13 @@ public void NestedWriteLocking1() var scopeProvider = GetScopeProvider(); - using (var w1 = d.GetWriter(scopeProvider)) + using (var w1 = d.GetScopedWriteLock(scopeProvider)) { Assert.AreEqual(1, t.LiveGen); Assert.AreEqual(1, t.WLocked); Assert.IsTrue(t.NextGen); - using (var w2 = d.GetWriter(scopeProvider)) + using (var w2 = d.GetScopedWriteLock(scopeProvider)) { Assert.AreEqual(1, t.LiveGen); Assert.AreEqual(2, t.WLocked); @@ -770,9 +770,9 @@ public void NestedWriteLocking2() var scopeContext = new ScopeContext(); var scopeProvider = GetScopeProvider(scopeContext); - using (var w1 = d.GetWriter(scopeProvider)) + using (var w1 = d.GetScopedWriteLock(scopeProvider)) { - using (var w2 = d.GetWriter(scopeProvider)) + using (var w2 = d.GetScopedWriteLock(scopeProvider)) { Assert.AreSame(w1, w2); @@ -794,13 +794,13 @@ public void NestedWriteLocking3() var scopeProvider1 = GetScopeProvider(); var scopeProvider2 = GetScopeProvider(scopeContext); - using (var w1 = d.GetWriter(scopeProvider1)) + using (var w1 = d.GetScopedWriteLock(scopeProvider1)) { Assert.AreEqual(1, t.LiveGen); Assert.AreEqual(1, t.WLocked); Assert.IsTrue(t.NextGen); - using (var w2 = d.GetWriter(scopeProvider2)) + using (var w2 = d.GetScopedWriteLock(scopeProvider2)) { Assert.AreEqual(1, t.LiveGen); Assert.AreEqual(2, t.WLocked); @@ -850,7 +850,7 @@ public void WriteLocking2() var scopeProvider = GetScopeProvider(); - using (d.GetWriter(scopeProvider)) + using (d.GetScopedWriteLock(scopeProvider)) { // gen 3 Assert.AreEqual(2, d.Test.GetValues(1).Length); @@ -895,7 +895,7 @@ public void WriteLocking3() var scopeProvider = GetScopeProvider(); - using (d.GetWriter(scopeProvider)) + using (d.GetScopedWriteLock(scopeProvider)) { // creating a snapshot in a write-lock does NOT return the "current" content // it uses the previous snapshot, so new snapshot created only on release @@ -935,7 +935,7 @@ public void ScopeLocking1() var scopeContext = new ScopeContext(); var scopeProvider = GetScopeProvider(scopeContext); - using (d.GetWriter(scopeProvider)) + using (d.GetScopedWriteLock(scopeProvider)) { // creating a snapshot in a write-lock does NOT return the "current" content // it uses the previous snapshot, so new snapshot created only on release @@ -985,7 +985,7 @@ public void ScopeLocking2() var scopeContext = new ScopeContext(); var scopeProvider = GetScopeProvider(scopeContext); - using (d.GetWriter(scopeProvider)) + using (d.GetScopedWriteLock(scopeProvider)) { // creating a snapshot in a write-lock does NOT return the "current" content // it uses the previous snapshot, so new snapshot created only on release @@ -1015,45 +1015,8 @@ public void ScopeLocking2() Assert.AreEqual(2, s4.Gen); Assert.AreEqual("uno", s4.Get(1)); - // fixme - remove debugging code - /* - Exception caught = null; - var genFlip = 0; - var lckFlip = 0; - var thread = new System.Threading.Thread(() => - { - try - { - for (var i = 0; i < 20; i++) - { - if (t.LiveGen == 2 && genFlip == 0) genFlip = i; // flips at 1 - if (t.WLocked == 0 && lckFlip == 0) lckFlip = i; // flips at 10 ie 5s, as expected - d.CreateSnapshot(); - System.Threading.Thread.Sleep(500); - } - } - catch (Exception e) - { - caught = e; - } - }); - thread.Start(); - */ - scopeContext.ScopeExit(false); - // fixme - remove debugging code - /* - thread.Join(); - - Assert.IsNull(caught); // but then how can it be not null? - - Console.WriteLine(genFlip); - Console.WriteLine(lckFlip); - Assert.AreEqual(1, genFlip); - Assert.AreEqual(10, lckFlip); - */ - // now things have changed Assert.AreEqual(2, t.LiveGen); Assert.IsFalse(t.NextGen); @@ -1104,13 +1067,13 @@ public void DontPanic() var d = new SnapDictionary(); d.Test.CollectAuto = false; - Assert.IsNull(d.Test.GenObj); // set with first snapshot or first lock, then never null + Assert.IsNull(d.Test.GenObj); // gen 1 d.Set(1, "one"); Assert.IsTrue(d.Test.NextGen); Assert.AreEqual(1, d.Test.LiveGen); - Assert.IsNotNull(d.Test.GenObj); // set with lock + Assert.IsNull(d.Test.GenObj); var s1 = d.CreateSnapshot(); Assert.IsFalse(d.Test.NextGen); @@ -1134,7 +1097,7 @@ public void DontPanic() // writer is scope contextual and scoped // when disposed, nothing happens // when the context exists, the writer is released - using (d.GetWriter(scopeProvider)) + using (d.GetScopedWriteLock(scopeProvider)) { d.Set(1, "ein"); Assert.IsTrue(d.Test.NextGen); diff --git a/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs b/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs index 2b9c9bee4db5..7ab4a64f3108 100644 --- a/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs +++ b/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs @@ -92,13 +92,13 @@ private class WriteLockInfo } // a scope contextual that represents a locked writer to the dictionary - private class ContentStoreWriter : ScopeContextualBase + private class ScopedWriteLock : ScopeContextualBase { private readonly WriteLockInfo _lockinfo = new WriteLockInfo(); private readonly ContentStore _store; private int _released; - public ContentStoreWriter(ContentStore store, bool scoped) + public ScopedWriteLock(ContentStore store, bool scoped) { _store = store; store.Lock(_lockinfo, scoped); @@ -114,9 +114,9 @@ public override void Release(bool completed) // gets a scope contextual representing a locked writer to the dictionary // TODO: GetScopedWriter? should the dict have a ref onto the scope provider? - public IDisposable GetWriter(IScopeProvider scopeProvider) + public IDisposable GetScopedWriteLock(IScopeProvider scopeProvider) { - return ScopeContextualBase.Get(scopeProvider, _instanceId, scoped => new ContentStoreWriter(this, scoped)); + return ScopeContextualBase.Get(scopeProvider, _instanceId, scoped => new ScopedWriteLock(this, scoped)); } private void Lock(WriteLockInfo lockInfo, bool forceGen = false) @@ -137,9 +137,10 @@ private void Lock(WriteLockInfo lockInfo, bool forceGen = false) { // because we are changing things, a new generation // is created, which will trigger a new snapshot - _nextGen = true; - _genObjs.Enqueue(_genObj = new GenObj(_liveGen)); + if (_nextGen) + _genObjs.Enqueue(_genObj = new GenObj(_liveGen)); _liveGen += 1; + _nextGen = true; } } } diff --git a/src/Umbraco.Web/PublishedCache/NuCache/PublishedSnapshotService.cs b/src/Umbraco.Web/PublishedCache/NuCache/PublishedSnapshotService.cs index 541ff2ea2336..9c5587fbd50c 100755 --- a/src/Umbraco.Web/PublishedCache/NuCache/PublishedSnapshotService.cs +++ b/src/Umbraco.Web/PublishedCache/NuCache/PublishedSnapshotService.cs @@ -333,7 +333,7 @@ private void LockAndLoadContent(Action action) // first get a writer, then a scope // if there already is a scope, the writer will attach to it // otherwise, it will only exist here - cheap - using (_contentStore.GetWriter(_scopeProvider)) + using (_contentStore.GetScopedWriteLock(_scopeProvider)) using (var scope = _scopeProvider.CreateScope()) { scope.ReadLock(Constants.Locks.ContentTree); @@ -401,7 +401,7 @@ private void LoadContentFromLocalDbLocked(IScope scope) private void LockAndLoadMedia(Action action) { // see note in LockAndLoadContent - using (_mediaStore.GetWriter(_scopeProvider)) + using (_mediaStore.GetScopedWriteLock(_scopeProvider)) using (var scope = _scopeProvider.CreateScope()) { scope.ReadLock(Constants.Locks.MediaTree); @@ -529,7 +529,7 @@ private void LoadMediaFromLocalDbLocked(IScope scope) private void LockAndLoadDomains() { // see note in LockAndLoadContent - using (_domainStore.GetWriter(_scopeProvider)) + using (_domainStore.GetScopedWriteLock(_scopeProvider)) using (var scope = _scopeProvider.CreateScope()) { scope.ReadLock(Constants.Locks.Domains); @@ -586,7 +586,7 @@ public override void Notify(ContentCacheRefresher.JsonPayload[] payloads, out bo return; } - using (_contentStore.GetWriter(_scopeProvider)) + using (_contentStore.GetScopedWriteLock(_scopeProvider)) { NotifyLocked(payloads, out bool draftChanged2, out bool publishedChanged2); draftChanged = draftChanged2; @@ -682,7 +682,7 @@ public override void Notify(MediaCacheRefresher.JsonPayload[] payloads, out bool return; } - using (_mediaStore.GetWriter(_scopeProvider)) + using (_mediaStore.GetScopedWriteLock(_scopeProvider)) { NotifyLocked(payloads, out bool anythingChanged2); anythingChanged = anythingChanged2; @@ -803,7 +803,7 @@ private void Notify(ContentStore store, ContentTypeCacheRefresher.JsonPayload if (removedIds.Count == 0 && refreshedIds.Count == 0 && otherIds.Count == 0 && newIds.Count == 0) return; - using (store.GetWriter(_scopeProvider)) + using (store.GetScopedWriteLock(_scopeProvider)) { // ReSharper disable AccessToModifiedClosure action(removedIds, refreshedIds, otherIds, newIds); @@ -824,8 +824,8 @@ public override void Notify(DataTypeCacheRefresher.JsonPayload[] payloads) payload.Removed ? "Removed" : "Refreshed", payload.Id); - using (_contentStore.GetWriter(_scopeProvider)) - using (_mediaStore.GetWriter(_scopeProvider)) + using (_contentStore.GetScopedWriteLock(_scopeProvider)) + using (_mediaStore.GetScopedWriteLock(_scopeProvider)) { // TODO: need to add a datatype lock // this is triggering datatypes reload in the factory, and right after we create some @@ -858,7 +858,7 @@ public override void Notify(DomainCacheRefresher.JsonPayload[] payloads) return; // see note in LockAndLoadContent - using (_domainStore.GetWriter(_scopeProvider)) + using (_domainStore.GetScopedWriteLock(_scopeProvider)) { foreach (var payload in payloads) { diff --git a/src/Umbraco.Web/PublishedCache/NuCache/SnapDictionary.cs b/src/Umbraco.Web/PublishedCache/NuCache/SnapDictionary.cs index 73904d145201..c5b1df120648 100644 --- a/src/Umbraco.Web/PublishedCache/NuCache/SnapDictionary.cs +++ b/src/Umbraco.Web/PublishedCache/NuCache/SnapDictionary.cs @@ -87,13 +87,13 @@ private class WriteLockInfo } // a scope contextual that represents a locked writer to the dictionary - private class SnapDictionaryWriter : ScopeContextualBase + private class ScopedWriteLock : ScopeContextualBase { private readonly WriteLockInfo _lockinfo = new WriteLockInfo(); private readonly SnapDictionary _dictionary; private int _released; - public SnapDictionaryWriter(SnapDictionary dictionary, bool scoped) + public ScopedWriteLock(SnapDictionary dictionary, bool scoped) { _dictionary = dictionary; dictionary.Lock(_lockinfo, scoped); @@ -108,14 +108,12 @@ public override void Release(bool completed) } // gets a scope contextual representing a locked writer to the dictionary - // fixme GetScopedWriter? should the dict have a ref onto the scope provider? - // fixme this is not a "writer" but a "write lock" => rename GetWriteLock // the dict is write-locked until the write-lock is released // which happens when it is disposed (non-scoped) // or when the scope context exits (scoped) - public IDisposable GetWriter(IScopeProvider scopeProvider) + public IDisposable GetScopedWriteLock(IScopeProvider scopeProvider) { - return ScopeContextualBase.Get(scopeProvider, _instanceId, scoped => new SnapDictionaryWriter(this, scoped)); + return ScopeContextualBase.Get(scopeProvider, _instanceId, scoped => new ScopedWriteLock(this, scoped)); } private void Lock(WriteLockInfo lockInfo, bool forceGen = false) @@ -143,9 +141,10 @@ private void Lock(WriteLockInfo lockInfo, bool forceGen = false) { // because we are changing things, a new generation // is created, which will trigger a new snapshot - _nextGen = true; // this is the ONLY place where _nextGen becomes true - _genObjs.Enqueue(_genObj = new GenObj(_liveGen)); + if (_nextGen) + _genObjs.Enqueue(_genObj = new GenObj(_liveGen)); _liveGen += 1; + _nextGen = true; // this is the ONLY place where _nextGen becomes true } } } @@ -197,9 +196,6 @@ private void Release(WriteLockInfo lockInfo, bool commit = true) } } - // fixme - pretend we need to do something that takes time - //System.Threading.Thread.Sleep(TimeSpan.FromSeconds(5)); - // decrement the lock count, if counting, then exit the lock if (lockInfo.Count) _wlocked--; Monitor.Exit(_wlocko); From 043f4f3d02cfd000a799b93e412858b481ead139 Mon Sep 17 00:00:00 2001 From: Shannon Date: Mon, 1 Apr 2019 14:20:47 +1100 Subject: [PATCH 20/23] Revert "Merge remote-tracking branch 'origin/v8/8.0' into temp8-4011" This reverts commit e73cf590b7f7af8f6f31cb495fca11a7964965b3, reversing changes made to 89903a633b2762dd6da017a53d05963a76a6886c. --- build/NuSpecs/UmbracoCms.Core.nuspec | 2 +- build/NuSpecs/UmbracoCms.Web.nuspec | 2 +- build/NuSpecs/UmbracoCms.nuspec | 2 +- .../Scoping/ScopeContextualBase.cs | 40 +-- .../Services/Implement/ContentService.cs | 9 +- .../Cache/SnapDictionaryTests.cs | 240 ++---------------- .../validation/valpropertymsg.directive.js | 11 +- .../src/preview/preview.controller.js | 7 +- .../content.assigndomain.controller.js | 3 +- .../MediaPickerValueConverter.cs | 58 ++--- .../PublishedCache/NuCache/ContentStore.cs | 113 ++++++--- .../NuCache/PublishedSnapshotService.cs | 54 ++-- .../PublishedCache/NuCache/Snap/GenObj.cs | 37 --- .../PublishedCache/NuCache/Snap/GenRef.cs | 13 - .../PublishedCache/NuCache/Snap/LinkedNode.cs | 20 -- .../PublishedCache/NuCache/SnapDictionary.cs | 218 +++++++++------- src/Umbraco.Web/Umbraco.Web.csproj | 3 - 17 files changed, 299 insertions(+), 533 deletions(-) delete mode 100644 src/Umbraco.Web/PublishedCache/NuCache/Snap/GenObj.cs delete mode 100644 src/Umbraco.Web/PublishedCache/NuCache/Snap/GenRef.cs delete mode 100644 src/Umbraco.Web/PublishedCache/NuCache/Snap/LinkedNode.cs diff --git a/build/NuSpecs/UmbracoCms.Core.nuspec b/build/NuSpecs/UmbracoCms.Core.nuspec index 11b1870b90bc..b257f8dcddd3 100644 --- a/build/NuSpecs/UmbracoCms.Core.nuspec +++ b/build/NuSpecs/UmbracoCms.Core.nuspec @@ -1,6 +1,6 @@ - + UmbracoCms.Core 8.0.0 Umbraco Cms Core Binaries diff --git a/build/NuSpecs/UmbracoCms.Web.nuspec b/build/NuSpecs/UmbracoCms.Web.nuspec index e81c19928813..79113a7c9430 100644 --- a/build/NuSpecs/UmbracoCms.Web.nuspec +++ b/build/NuSpecs/UmbracoCms.Web.nuspec @@ -1,6 +1,6 @@ - + UmbracoCms.Web 8.0.0 Umbraco Cms Core Binaries diff --git a/build/NuSpecs/UmbracoCms.nuspec b/build/NuSpecs/UmbracoCms.nuspec index c93a82c6911e..8ec14844527d 100644 --- a/build/NuSpecs/UmbracoCms.nuspec +++ b/build/NuSpecs/UmbracoCms.nuspec @@ -1,6 +1,6 @@ - + UmbracoCms 8.0.0 Umbraco Cms diff --git a/src/Umbraco.Core/Scoping/ScopeContextualBase.cs b/src/Umbraco.Core/Scoping/ScopeContextualBase.cs index 25f176d4719a..746114223452 100644 --- a/src/Umbraco.Core/Scoping/ScopeContextualBase.cs +++ b/src/Umbraco.Core/Scoping/ScopeContextualBase.cs @@ -2,61 +2,39 @@ namespace Umbraco.Core.Scoping { - /// - /// Provides a base class for scope contextual objects. - /// - /// - /// A scope contextual object is enlisted in the current scope context, - /// if any, and released when the context exists. It must be used in a 'using' - /// block, and will be released when disposed, if not part of a scope. - /// + // base class for an object that will be enlisted in scope context, if any. it + // must be used in a 'using' block, and if not scoped, released when disposed, + // else when scope context runs enlisted actions public abstract class ScopeContextualBase : IDisposable { - private bool _scoped; - - /// - /// Gets a contextual object. - /// - /// The type of the object. - /// A scope provider. - /// A context key for the object. - /// A function producing the contextual object. - /// The contextual object. - /// - /// - /// + private bool _using, _scoped; + public static T Get(IScopeProvider scopeProvider, string key, Func ctor) where T : ScopeContextualBase { - // no scope context = create a non-scoped object var scopeContext = scopeProvider.Context; if (scopeContext == null) return ctor(false); - // create & enlist the scoped object var w = scopeContext.Enlist("ScopeContextualBase_" + key, () => ctor(true), (completed, item) => { item.Release(completed); }); + if (w._using) throw new InvalidOperationException("panic: used."); + w._using = true; w._scoped = true; return w; } - /// - /// - /// If not scoped, then this releases the contextual object. - /// public void Dispose() { + _using = false; + if (_scoped == false) Release(true); } - /// - /// Releases the contextual object. - /// - /// A value indicating whether the scoped operation completed. public abstract void Release(bool completed); } } diff --git a/src/Umbraco.Core/Services/Implement/ContentService.cs b/src/Umbraco.Core/Services/Implement/ContentService.cs index f2bef5292280..cc962fbe229d 100644 --- a/src/Umbraco.Core/Services/Implement/ContentService.cs +++ b/src/Umbraco.Core/Services/Implement/ContentService.cs @@ -2875,14 +2875,7 @@ public IContent CreateContentFromBlueprint(IContent blueprint, string name, int { foreach (var property in blueprint.Properties) { - if (property.PropertyType.VariesByCulture()) - { - content.SetValue(property.Alias, property.GetValue(culture), culture); - } - else - { - content.SetValue(property.Alias, property.GetValue()); - } + content.SetValue(property.Alias, property.GetValue(culture), culture); } content.Name = blueprint.Name; diff --git a/src/Umbraco.Tests/Cache/SnapDictionaryTests.cs b/src/Umbraco.Tests/Cache/SnapDictionaryTests.cs index b435af9e7721..013dbadbb855 100644 --- a/src/Umbraco.Tests/Cache/SnapDictionaryTests.cs +++ b/src/Umbraco.Tests/Cache/SnapDictionaryTests.cs @@ -5,7 +5,6 @@ using NUnit.Framework; using Umbraco.Core.Scoping; using Umbraco.Web.PublishedCache.NuCache; -using Umbraco.Web.PublishedCache.NuCache.Snap; namespace Umbraco.Tests.Cache { @@ -389,7 +388,8 @@ public async Task EventuallyCollectNulls() // collect liveGen GC.Collect(); - Assert.IsTrue(d.Test.GenObjs.TryPeek(out var genObj)); + SnapDictionary.GenerationObject genObj; + Assert.IsTrue(d.Test.GenerationObjects.TryPeek(out genObj)); genObj = null; // in Release mode, it works, but in Debug mode, the weak reference is still alive @@ -399,14 +399,14 @@ public async Task EventuallyCollectNulls() GC.Collect(); #endif - Assert.IsTrue(d.Test.GenObjs.TryPeek(out genObj)); - Assert.IsFalse(genObj.WeakGenRef.IsAlive); // snapshot is gone, along with its reference + Assert.IsTrue(d.Test.GenerationObjects.TryPeek(out genObj)); + Assert.IsFalse(genObj.WeakReference.IsAlive); // snapshot is gone, along with its reference await d.CollectAsync(); Assert.AreEqual(0, d.Test.GetValues(1).Length); // null value is gone Assert.AreEqual(0, d.Count); // item is gone - Assert.AreEqual(0, d.Test.GenObjs.Count); + Assert.AreEqual(0, d.Test.GenerationObjects.Count); Assert.AreEqual(0, d.SnapCount); // snapshot is gone Assert.AreEqual(0, d.GenCount); // and generation has been dequeued } @@ -632,7 +632,7 @@ public void WriteLockingFirstSnapshot() Assert.AreEqual(1, d.Test.LiveGen); Assert.IsTrue(d.Test.NextGen); - using (d.GetScopedWriteLock(GetScopeProvider())) + using (d.GetWriter(GetScopeProvider())) { var s1 = d.CreateSnapshot(); @@ -685,7 +685,7 @@ public void WriteLocking() Assert.IsFalse(d.Test.NextGen); Assert.AreEqual("uno", s2.Get(1)); - using (d.GetScopedWriteLock(GetScopeProvider())) + using (d.GetWriter(GetScopeProvider())) { // gen 3 Assert.AreEqual(2, d.Test.GetValues(1).Length); @@ -712,102 +712,16 @@ public void WriteLocking() } [Test] - public void NestedWriteLocking1() - { - var d = new SnapDictionary(); - var t = d.Test; - t.CollectAuto = false; - - Assert.AreEqual(0, d.CreateSnapshot().Gen); - - // no scope context: writers nest, last one to be disposed commits - - var scopeProvider = GetScopeProvider(); - - using (var w1 = d.GetScopedWriteLock(scopeProvider)) - { - Assert.AreEqual(1, t.LiveGen); - Assert.AreEqual(1, t.WLocked); - Assert.IsTrue(t.NextGen); - - using (var w2 = d.GetScopedWriteLock(scopeProvider)) - { - Assert.AreEqual(1, t.LiveGen); - Assert.AreEqual(2, t.WLocked); - Assert.IsTrue(t.NextGen); - - Assert.AreNotSame(w1, w2); // get a new writer each time - - d.Set(1, "one"); - - Assert.AreEqual(0, d.CreateSnapshot().Gen); - } - - Assert.AreEqual(1, t.LiveGen); - Assert.AreEqual(1, t.WLocked); - Assert.IsTrue(t.NextGen); - - Assert.AreEqual(0, d.CreateSnapshot().Gen); - } - - Assert.AreEqual(1, t.LiveGen); - Assert.AreEqual(0, t.WLocked); - Assert.IsTrue(t.NextGen); - - Assert.AreEqual(1, d.CreateSnapshot().Gen); - } - - [Test] - public void NestedWriteLocking2() + public void NestedWriteLocking() { var d = new SnapDictionary(); d.Test.CollectAuto = false; - Assert.AreEqual(0, d.CreateSnapshot().Gen); - - // scope context: writers enlist - - var scopeContext = new ScopeContext(); - var scopeProvider = GetScopeProvider(scopeContext); - - using (var w1 = d.GetScopedWriteLock(scopeProvider)) - { - using (var w2 = d.GetScopedWriteLock(scopeProvider)) - { - Assert.AreSame(w1, w2); - - d.Set(1, "one"); - } - } - } - - [Test] - public void NestedWriteLocking3() - { - var d = new SnapDictionary(); - var t = d.Test; - t.CollectAuto = false; - - Assert.AreEqual(0, d.CreateSnapshot().Gen); - - var scopeContext = new ScopeContext(); - var scopeProvider1 = GetScopeProvider(); - var scopeProvider2 = GetScopeProvider(scopeContext); - - using (var w1 = d.GetScopedWriteLock(scopeProvider1)) + var scopeProvider = GetScopeProvider(); + using (d.GetWriter(scopeProvider)) { - Assert.AreEqual(1, t.LiveGen); - Assert.AreEqual(1, t.WLocked); - Assert.IsTrue(t.NextGen); - - using (var w2 = d.GetScopedWriteLock(scopeProvider2)) + using (d.GetWriter(scopeProvider)) { - Assert.AreEqual(1, t.LiveGen); - Assert.AreEqual(2, t.WLocked); - Assert.IsTrue(t.NextGen); - - Assert.AreNotSame(w1, w2); - d.Set(1, "one"); } } @@ -850,7 +764,7 @@ public void WriteLocking2() var scopeProvider = GetScopeProvider(); - using (d.GetScopedWriteLock(scopeProvider)) + using (d.GetWriter(scopeProvider)) { // gen 3 Assert.AreEqual(2, d.Test.GetValues(1).Length); @@ -895,7 +809,7 @@ public void WriteLocking3() var scopeProvider = GetScopeProvider(); - using (d.GetScopedWriteLock(scopeProvider)) + using (d.GetWriter(scopeProvider)) { // creating a snapshot in a write-lock does NOT return the "current" content // it uses the previous snapshot, so new snapshot created only on release @@ -932,10 +846,9 @@ public void ScopeLocking1() Assert.AreEqual(2, s2.Gen); Assert.AreEqual("uno", s2.Get(1)); - var scopeContext = new ScopeContext(); - var scopeProvider = GetScopeProvider(scopeContext); + var scopeProvider = GetScopeProvider(true); - using (d.GetScopedWriteLock(scopeProvider)) + using (d.GetWriter(scopeProvider)) { // creating a snapshot in a write-lock does NOT return the "current" content // it uses the previous snapshot, so new snapshot created only on release @@ -954,7 +867,7 @@ public void ScopeLocking1() Assert.AreEqual(2, s4.Gen); Assert.AreEqual("uno", s4.Get(1)); - scopeContext.ScopeExit(true); + ((ScopeContext) scopeProvider.Context).ScopeExit(true); var s5 = d.CreateSnapshot(); Assert.AreEqual(3, s5.Gen); @@ -965,8 +878,7 @@ public void ScopeLocking1() public void ScopeLocking2() { var d = new SnapDictionary(); - var t = d.Test; - t.CollectAuto = false; + d.Test.CollectAuto = false; // gen 1 d.Set(1, "one"); @@ -979,13 +891,12 @@ public void ScopeLocking2() Assert.AreEqual(2, s2.Gen); Assert.AreEqual("uno", s2.Get(1)); - Assert.AreEqual(2, t.LiveGen); - Assert.IsFalse(t.NextGen); - + var scopeProviderMock = new Mock(); var scopeContext = new ScopeContext(); - var scopeProvider = GetScopeProvider(scopeContext); + scopeProviderMock.Setup(x => x.Context).Returns(scopeContext); + var scopeProvider = scopeProviderMock.Object; - using (d.GetScopedWriteLock(scopeProvider)) + using (d.GetWriter(scopeProvider)) { // creating a snapshot in a write-lock does NOT return the "current" content // it uses the previous snapshot, so new snapshot created only on release @@ -994,35 +905,18 @@ public void ScopeLocking2() Assert.AreEqual(2, s3.Gen); Assert.AreEqual("uno", s3.Get(1)); - // we made some changes, so a next gen is required - Assert.AreEqual(3, t.LiveGen); - Assert.IsTrue(t.NextGen); - Assert.AreEqual(1, t.WLocked); - // but live snapshot contains changes - var ls = t.LiveSnapshot; + var ls = d.Test.LiveSnapshot; Assert.AreEqual("ein", ls.Get(1)); Assert.AreEqual(3, ls.Gen); } - // nothing is committed until scope exits - Assert.AreEqual(3, t.LiveGen); - Assert.IsTrue(t.NextGen); - Assert.AreEqual(1, t.WLocked); - - // no changes until exit var s4 = d.CreateSnapshot(); Assert.AreEqual(2, s4.Gen); Assert.AreEqual("uno", s4.Get(1)); scopeContext.ScopeExit(false); - // now things have changed - Assert.AreEqual(2, t.LiveGen); - Assert.IsFalse(t.NextGen); - Assert.AreEqual(0, t.WLocked); - - // no changes since not completed var s5 = d.CreateSnapshot(); Assert.AreEqual(2, s5.Gen); Assert.AreEqual("uno", s5.Get(1)); @@ -1061,92 +955,12 @@ public void GetAll() Assert.AreEqual("four", all[3]); } - [Test] - public void DontPanic() - { - var d = new SnapDictionary(); - d.Test.CollectAuto = false; - - Assert.IsNull(d.Test.GenObj); - - // gen 1 - d.Set(1, "one"); - Assert.IsTrue(d.Test.NextGen); - Assert.AreEqual(1, d.Test.LiveGen); - Assert.IsNull(d.Test.GenObj); - - var s1 = d.CreateSnapshot(); - Assert.IsFalse(d.Test.NextGen); - Assert.AreEqual(1, d.Test.LiveGen); - Assert.IsNotNull(d.Test.GenObj); - Assert.AreEqual(1, d.Test.GenObj.Gen); - - Assert.AreEqual(1, s1.Gen); - Assert.AreEqual("one", s1.Get(1)); - - d.Set(1, "uno"); - Assert.IsTrue(d.Test.NextGen); - Assert.AreEqual(2, d.Test.LiveGen); - Assert.IsNotNull(d.Test.GenObj); - Assert.AreEqual(1, d.Test.GenObj.Gen); - - var scopeContext = new ScopeContext(); - var scopeProvider = GetScopeProvider(scopeContext); - - // scopeProvider.Context == scopeContext -> writer is scoped - // writer is scope contextual and scoped - // when disposed, nothing happens - // when the context exists, the writer is released - using (d.GetScopedWriteLock(scopeProvider)) - { - d.Set(1, "ein"); - Assert.IsTrue(d.Test.NextGen); - Assert.AreEqual(3, d.Test.LiveGen); - Assert.IsNotNull(d.Test.GenObj); - Assert.AreEqual(2, d.Test.GenObj.Gen); - } - - // writer has not released - Assert.AreEqual(1, d.Test.WLocked); - Assert.IsNotNull(d.Test.GenObj); - Assert.AreEqual(2, d.Test.GenObj.Gen); - - // nothing changed - Assert.IsTrue(d.Test.NextGen); - Assert.AreEqual(3, d.Test.LiveGen); - - // panic! - var s2 = d.CreateSnapshot(); - - Assert.AreEqual(1, d.Test.WLocked); - Assert.IsNotNull(d.Test.GenObj); - Assert.AreEqual(2, d.Test.GenObj.Gen); - Assert.AreEqual(3, d.Test.LiveGen); - Assert.IsTrue(d.Test.NextGen); - - // release writer - scopeContext.ScopeExit(true); - - Assert.AreEqual(0, d.Test.WLocked); - Assert.IsNotNull(d.Test.GenObj); - Assert.AreEqual(2, d.Test.GenObj.Gen); - Assert.AreEqual(3, d.Test.LiveGen); - Assert.IsTrue(d.Test.NextGen); - - var s3 = d.CreateSnapshot(); - - Assert.AreEqual(0, d.Test.WLocked); - Assert.IsNotNull(d.Test.GenObj); - Assert.AreEqual(3, d.Test.GenObj.Gen); - Assert.AreEqual(3, d.Test.LiveGen); - Assert.IsFalse(d.Test.NextGen); - } - - private IScopeProvider GetScopeProvider(ScopeContext scopeContext = null) + private IScopeProvider GetScopeProvider(bool withContext = false) { - var scopeProvider = Mock.Of(); - Mock.Get(scopeProvider) - .Setup(x => x.Context).Returns(scopeContext); + var scopeProviderMock = new Mock(); + var scopeContext = withContext ? new ScopeContext() : null; + scopeProviderMock.Setup(x => x.Context).Returns(scopeContext); + var scopeProvider = scopeProviderMock.Object; return scopeProvider; } } diff --git a/src/Umbraco.Web.UI.Client/src/common/directives/validation/valpropertymsg.directive.js b/src/Umbraco.Web.UI.Client/src/common/directives/validation/valpropertymsg.directive.js index c027e0778ee6..9ee83dc2ba3f 100644 --- a/src/Umbraco.Web.UI.Client/src/common/directives/validation/valpropertymsg.directive.js +++ b/src/Umbraco.Web.UI.Client/src/common/directives/validation/valpropertymsg.directive.js @@ -62,8 +62,8 @@ function valPropertyMsg(serverValidationManager) { if (!watcher) { watcher = scope.$watch("currentProperty.value", function (newValue, oldValue) { - - if (angular.equals(newValue, oldValue)) { + + if (!newValue || angular.equals(newValue, oldValue)) { return; } @@ -78,12 +78,10 @@ function valPropertyMsg(serverValidationManager) { // based on other errors. We'll also check if there's no other validation errors apart from valPropertyMsg, if valPropertyMsg // is the only one, then we'll clear. - if (errCount === 0 || (errCount === 1 && angular.isArray(formCtrl.$error.valPropertyMsg)) || (formCtrl.$invalid && angular.isArray(formCtrl.$error.valServer))) { + if ((errCount === 1 && angular.isArray(formCtrl.$error.valPropertyMsg)) || (formCtrl.$invalid && angular.isArray(formCtrl.$error.valServer))) { scope.errorMsg = ""; formCtrl.$setValidity('valPropertyMsg', true); - } else if (showValidation && scope.errorMsg === "") { - formCtrl.$setValidity('valPropertyMsg', false); - scope.errorMsg = getErrorMsg(); + stopWatch(); } }, true); } @@ -154,7 +152,6 @@ function valPropertyMsg(serverValidationManager) { showValidation = true; if (hasError && scope.errorMsg === "") { scope.errorMsg = getErrorMsg(); - startWatch(); } else if (!hasError) { scope.errorMsg = ""; diff --git a/src/Umbraco.Web.UI.Client/src/preview/preview.controller.js b/src/Umbraco.Web.UI.Client/src/preview/preview.controller.js index 4733c58556ed..7d6584d2f1bc 100644 --- a/src/Umbraco.Web.UI.Client/src/preview/preview.controller.js +++ b/src/Umbraco.Web.UI.Client/src/preview/preview.controller.js @@ -113,12 +113,7 @@ var app = angular.module("umbraco.preview", ['umbraco.resources', 'umbraco.servi $scope.exitPreview = function () { var culture = $location.search().culture || getParameterByName("culture"); - var relativeUrl = "/" + $scope.pageId; - - if(culture){ - relativeUrl +='?culture='+ culture; - } - + var relativeUrl = "/" + $scope.pageId +'?culture='+ culture; window.top.location.href = "../preview/end?redir=" + encodeURIComponent(relativeUrl); }; diff --git a/src/Umbraco.Web.UI.Client/src/views/content/content.assigndomain.controller.js b/src/Umbraco.Web.UI.Client/src/views/content/content.assigndomain.controller.js index 5fa1eebd0b2d..0f27f3046cc8 100644 --- a/src/Umbraco.Web.UI.Client/src/views/content/content.assigndomain.controller.js +++ b/src/Umbraco.Web.UI.Client/src/views/content/content.assigndomain.controller.js @@ -46,7 +46,8 @@ if (data.language !== "undefined") { var lang = vm.languages.filter(function (l) { - return matchLanguageById(l, data.language); + return matchLanguageById(l, data.language.Id); + }); if (lang.length > 0) { vm.language = lang[0]; diff --git a/src/Umbraco.Web/PropertyEditors/ValueConverters/MediaPickerValueConverter.cs b/src/Umbraco.Web/PropertyEditors/ValueConverters/MediaPickerValueConverter.cs index fb3134c9a3bb..802f42ed9e8a 100644 --- a/src/Umbraco.Web/PropertyEditors/ValueConverters/MediaPickerValueConverter.cs +++ b/src/Umbraco.Web/PropertyEditors/ValueConverters/MediaPickerValueConverter.cs @@ -1,5 +1,4 @@ using System; -using System.Collections; using System.Collections.Generic; using System.Linq; using Umbraco.Core; @@ -15,18 +14,11 @@ namespace Umbraco.Web.PropertyEditors.ValueConverters [DefaultPropertyValueConverter] public class MediaPickerValueConverter : PropertyValueConverterBase { - // hard-coding "image" here but that's how it works at UI level too - private const string ImageTypeAlias = "image"; - - private readonly IPublishedModelFactory _publishedModelFactory; private readonly IPublishedSnapshotAccessor _publishedSnapshotAccessor; - public MediaPickerValueConverter(IPublishedSnapshotAccessor publishedSnapshotAccessor, - IPublishedModelFactory publishedModelFactory) + public MediaPickerValueConverter(IPublishedSnapshotAccessor publishedSnapshotAccessor) { - _publishedSnapshotAccessor = publishedSnapshotAccessor ?? - throw new ArgumentNullException(nameof(publishedSnapshotAccessor)); - _publishedModelFactory = publishedModelFactory; + _publishedSnapshotAccessor = publishedSnapshotAccessor ?? throw new ArgumentNullException(nameof(publishedSnapshotAccessor)); } public override bool IsConverter(PublishedPropertyType propertyType) @@ -39,19 +31,15 @@ public override Type GetPropertyValueType(PublishedPropertyType propertyType) var isMultiple = IsMultipleDataType(propertyType.DataType); var isOnlyImages = IsOnlyImagesDataType(propertyType.DataType); + // hard-coding "image" here but that's how it works at UI level too + return isMultiple - ? isOnlyImages - ? typeof(IEnumerable<>).MakeGenericType(ModelType.For(ImageTypeAlias)) - : typeof(IEnumerable) - : isOnlyImages - ? ModelType.For(ImageTypeAlias) - : typeof(IPublishedContent); + ? (isOnlyImages ? typeof(IEnumerable<>).MakeGenericType(ModelType.For("image")) : typeof(IEnumerable)) + : (isOnlyImages ? ModelType.For("image") : typeof(IPublishedContent)); } public override PropertyCacheLevel GetPropertyCacheLevel(PublishedPropertyType propertyType) - { - return PropertyCacheLevel.Snapshot; - } + => PropertyCacheLevel.Snapshot; private bool IsMultipleDataType(PublishedDataType dataType) { @@ -65,31 +53,26 @@ private bool IsOnlyImagesDataType(PublishedDataType dataType) return config.OnlyImages; } - public override object ConvertSourceToIntermediate(IPublishedElement owner, PublishedPropertyType propertyType, - object source, bool preview) + public override object ConvertSourceToIntermediate(IPublishedElement owner, PublishedPropertyType propertyType, object source, bool preview) { if (source == null) return null; var nodeIds = source.ToString() - .Split(new[] {","}, StringSplitOptions.RemoveEmptyEntries) + .Split(new[] { "," }, StringSplitOptions.RemoveEmptyEntries) .Select(Udi.Parse) .ToArray(); return nodeIds; } - public override object ConvertIntermediateToObject(IPublishedElement owner, PublishedPropertyType propertyType, - PropertyCacheLevel cacheLevel, object source, bool preview) + public override object ConvertIntermediateToObject(IPublishedElement owner, PublishedPropertyType propertyType, PropertyCacheLevel cacheLevel, object source, bool preview) { - var isMultiple = IsMultipleDataType(propertyType.DataType); - var isOnlyImages = IsOnlyImagesDataType(propertyType.DataType); - - var udis = (Udi[]) source; - var mediaItems = isOnlyImages - ? _publishedModelFactory.CreateModelList(ImageTypeAlias) - : new List(); - - if (source == null) return isMultiple ? mediaItems : null; + if (source == null) + { + return null; + } + var udis = (Udi[])source; + var mediaItems = new List(); if (udis.Any()) { foreach (var udi in udis) @@ -101,15 +84,12 @@ public override object ConvertIntermediateToObject(IPublishedElement owner, Publ mediaItems.Add(item); } - return isMultiple ? mediaItems : FirstOrDefault(mediaItems); + if (IsMultipleDataType(propertyType.DataType)) + return mediaItems; + return mediaItems.FirstOrDefault(); } return source; } - - private object FirstOrDefault(IList mediaItems) - { - return mediaItems.Count == 0 ? null : mediaItems[0]; - } } } diff --git a/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs b/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs index 7ab4a64f3108..b3996050a6e9 100644 --- a/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs +++ b/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs @@ -8,7 +8,6 @@ using Umbraco.Core.Logging; using Umbraco.Core.Models.PublishedContent; using Umbraco.Core.Scoping; -using Umbraco.Web.PublishedCache.NuCache.Snap; namespace Umbraco.Web.PublishedCache.NuCache { @@ -30,8 +29,8 @@ internal class ContentStore private readonly ILogger _logger; private BPlusTree _localDb; - private readonly ConcurrentQueue _genObjs; - private GenObj _genObj; + private readonly ConcurrentQueue _genRefRefs; + private GenRefRef _genRefRef; private readonly object _wlocko = new object(); private readonly object _rlocko = new object(); private long _liveGen, _floorGen; @@ -65,8 +64,8 @@ public ContentStore( _contentTypesByAlias = new ConcurrentDictionary>(StringComparer.InvariantCultureIgnoreCase); _xmap = new ConcurrentDictionary(); - _genObjs = new ConcurrentQueue(); - _genObj = null; // no initial gen exists + _genRefRefs = new ConcurrentQueue(); + _genRefRef = null; // no initial gen exists _liveGen = _floorGen = 0; _nextGen = false; // first time, must create a snapshot _collectAuto = true; // collect automatically by default @@ -92,13 +91,12 @@ private class WriteLockInfo } // a scope contextual that represents a locked writer to the dictionary - private class ScopedWriteLock : ScopeContextualBase + private class ContentStoreWriter : ScopeContextualBase { private readonly WriteLockInfo _lockinfo = new WriteLockInfo(); - private readonly ContentStore _store; - private int _released; + private ContentStore _store; - public ScopedWriteLock(ContentStore store, bool scoped) + public ContentStoreWriter(ContentStore store, bool scoped) { _store = store; store.Lock(_lockinfo, scoped); @@ -106,17 +104,17 @@ public ScopedWriteLock(ContentStore store, bool scoped) public override void Release(bool completed) { - if (Interlocked.CompareExchange(ref _released, 1, 0) != 0) - return; + if (_store== null) return; _store.Release(_lockinfo, completed); + _store = null; } } // gets a scope contextual representing a locked writer to the dictionary // TODO: GetScopedWriter? should the dict have a ref onto the scope provider? - public IDisposable GetScopedWriteLock(IScopeProvider scopeProvider) + public IDisposable GetWriter(IScopeProvider scopeProvider) { - return ScopeContextualBase.Get(scopeProvider, _instanceId, scoped => new ScopedWriteLock(this, scoped)); + return ScopeContextualBase.Get(scopeProvider, _instanceId, scoped => new ContentStoreWriter(this, scoped)); } private void Lock(WriteLockInfo lockInfo, bool forceGen = false) @@ -133,14 +131,12 @@ private void Lock(WriteLockInfo lockInfo, bool forceGen = false) { _wlocked++; lockInfo.Count = true; - if (_nextGen == false || (forceGen && _wlocked == 1)) + if (_nextGen == false || (forceGen && _wlocked == 1)) // if true already... ok to have "holes" in generation objects { // because we are changing things, a new generation // is created, which will trigger a new snapshot - if (_nextGen) - _genObjs.Enqueue(_genObj = new GenObj(_liveGen)); - _liveGen += 1; _nextGen = true; + _liveGen += 1; } } } @@ -216,6 +212,7 @@ private void Rollback(ConcurrentDictionary("Collect."); #endif - while (_genObjs.TryPeek(out var genObj) && (genObj.Count == 0 || genObj.WeakGenRef.IsAlive == false)) + while (_genRefRefs.TryPeek(out GenRefRef genRefRef) && (genRefRef.Count == 0 || genRefRef.WGenRef.IsAlive == false)) { - _genObjs.TryDequeue(out genObj); // cannot fail since TryPeek has succeeded - _floorGen = genObj.Gen; + _genRefRefs.TryDequeue(out genRefRef); // cannot fail since TryPeek has succeeded + _floorGen = genRefRef.Gen; #if DEBUG //_logger.Debug("_floorGen=" + _floorGen + ", _liveGen=" + _liveGen); #endif @@ -1012,9 +1009,9 @@ public async Task WaitForPendingCollect() await task; } - public long GenCount => _genObjs.Count; + public long GenCount => _genRefRefs.Count; - public long SnapCount => _genObjs.Sum(x => x.Count); + public long SnapCount => _genRefRefs.Sum(x => x.Count); #endregion @@ -1064,6 +1061,24 @@ public Tuple[] GetValues(int id) #region Classes + private class LinkedNode + where TValue: class + { + public LinkedNode(TValue value, long gen, LinkedNode next = null) + { + Value = value; + Gen = gen; + Next = next; + } + + internal readonly long Gen; + + // reading & writing references is thread-safe on all .NET platforms + // mark as volatile to ensure we always read the correct value + internal volatile TValue Value; + internal volatile LinkedNode Next; + } + public class Snapshot : IDisposable { private readonly ContentStore _store; @@ -1085,7 +1100,7 @@ internal Snapshot(ContentStore store, GenRef genRef _store = store; _genRef = genRef; _gen = genRef.Gen; - Interlocked.Increment(ref genRef.GenObj.Count); + Interlocked.Increment(ref genRef.GenRefRef.Count); //_thisCount = _count++; #if DEBUG @@ -1186,15 +1201,49 @@ public void Dispose() { if (_gen < 0) return; #if DEBUG - _logger.Debug("Dispose snapshot ({Snapshot})", _genRef?.GenObj.Count.ToString() ?? "live"); + _logger.Debug("Dispose snapshot ({Snapshot})", _genRef?.GenRefRef.Count.ToString() ?? "live"); #endif _gen = -1; if (_genRef != null) - Interlocked.Decrement(ref _genRef.GenObj.Count); + Interlocked.Decrement(ref _genRef.GenRefRef.Count); GC.SuppressFinalize(this); } } + internal class GenRefRef + { + public GenRefRef(long gen) + { + Gen = gen; + WGenRef = new WeakReference(null); + } + + public GenRef GetGenRef() + { + // not thread-safe but always invoked from within a lock + var genRef = (GenRef) WGenRef.Target; + if (genRef == null) + WGenRef.Target = genRef = new GenRef(this, Gen); + return genRef; + } + + public readonly long Gen; + public readonly WeakReference WGenRef; + public int Count; + } + + internal class GenRef + { + public GenRef(GenRefRef genRefRef, long gen) + { + GenRefRef = genRefRef; + Gen = gen; + } + + public readonly GenRefRef GenRefRef; + public readonly long Gen; + } + #endregion } } diff --git a/src/Umbraco.Web/PublishedCache/NuCache/PublishedSnapshotService.cs b/src/Umbraco.Web/PublishedCache/NuCache/PublishedSnapshotService.cs index 9c5587fbd50c..e19531a25b19 100755 --- a/src/Umbraco.Web/PublishedCache/NuCache/PublishedSnapshotService.cs +++ b/src/Umbraco.Web/PublishedCache/NuCache/PublishedSnapshotService.cs @@ -330,15 +330,14 @@ public override bool EnsureEnvironment(out IEnumerable errors) private void LockAndLoadContent(Action action) { - // first get a writer, then a scope - // if there already is a scope, the writer will attach to it - // otherwise, it will only exist here - cheap - using (_contentStore.GetScopedWriteLock(_scopeProvider)) - using (var scope = _scopeProvider.CreateScope()) + using (_contentStore.GetWriter(_scopeProvider)) { - scope.ReadLock(Constants.Locks.ContentTree); - action(scope); - scope.Complete(); + using (var scope = _scopeProvider.CreateScope()) + { + scope.ReadLock(Constants.Locks.ContentTree); + action(scope); + scope.Complete(); + } } } @@ -400,13 +399,14 @@ private void LoadContentFromLocalDbLocked(IScope scope) private void LockAndLoadMedia(Action action) { - // see note in LockAndLoadContent - using (_mediaStore.GetScopedWriteLock(_scopeProvider)) - using (var scope = _scopeProvider.CreateScope()) + using (_mediaStore.GetWriter(_scopeProvider)) { - scope.ReadLock(Constants.Locks.MediaTree); - action(scope); - scope.Complete(); + using (var scope = _scopeProvider.CreateScope()) + { + scope.ReadLock(Constants.Locks.MediaTree); + action(scope); + scope.Complete(); + } } } @@ -528,13 +528,14 @@ private void LoadMediaFromLocalDbLocked(IScope scope) private void LockAndLoadDomains() { - // see note in LockAndLoadContent - using (_domainStore.GetScopedWriteLock(_scopeProvider)) - using (var scope = _scopeProvider.CreateScope()) + using (_domainStore.GetWriter(_scopeProvider)) { - scope.ReadLock(Constants.Locks.Domains); - LoadDomainsLocked(); - scope.Complete(); + using (var scope = _scopeProvider.CreateScope()) + { + scope.ReadLock(Constants.Locks.Domains); + LoadDomainsLocked(); + scope.Complete(); + } } } @@ -586,7 +587,7 @@ public override void Notify(ContentCacheRefresher.JsonPayload[] payloads, out bo return; } - using (_contentStore.GetScopedWriteLock(_scopeProvider)) + using (_contentStore.GetWriter(_scopeProvider)) { NotifyLocked(payloads, out bool draftChanged2, out bool publishedChanged2); draftChanged = draftChanged2; @@ -682,7 +683,7 @@ public override void Notify(MediaCacheRefresher.JsonPayload[] payloads, out bool return; } - using (_mediaStore.GetScopedWriteLock(_scopeProvider)) + using (_mediaStore.GetWriter(_scopeProvider)) { NotifyLocked(payloads, out bool anythingChanged2); anythingChanged = anythingChanged2; @@ -803,7 +804,7 @@ private void Notify(ContentStore store, ContentTypeCacheRefresher.JsonPayload if (removedIds.Count == 0 && refreshedIds.Count == 0 && otherIds.Count == 0 && newIds.Count == 0) return; - using (store.GetScopedWriteLock(_scopeProvider)) + using (store.GetWriter(_scopeProvider)) { // ReSharper disable AccessToModifiedClosure action(removedIds, refreshedIds, otherIds, newIds); @@ -824,8 +825,8 @@ public override void Notify(DataTypeCacheRefresher.JsonPayload[] payloads) payload.Removed ? "Removed" : "Refreshed", payload.Id); - using (_contentStore.GetScopedWriteLock(_scopeProvider)) - using (_mediaStore.GetScopedWriteLock(_scopeProvider)) + using (_contentStore.GetWriter(_scopeProvider)) + using (_mediaStore.GetWriter(_scopeProvider)) { // TODO: need to add a datatype lock // this is triggering datatypes reload in the factory, and right after we create some @@ -857,8 +858,7 @@ public override void Notify(DomainCacheRefresher.JsonPayload[] payloads) if (_isReady == false) return; - // see note in LockAndLoadContent - using (_domainStore.GetScopedWriteLock(_scopeProvider)) + using (_domainStore.GetWriter(_scopeProvider)) { foreach (var payload in payloads) { diff --git a/src/Umbraco.Web/PublishedCache/NuCache/Snap/GenObj.cs b/src/Umbraco.Web/PublishedCache/NuCache/Snap/GenObj.cs deleted file mode 100644 index b69dab7dac48..000000000000 --- a/src/Umbraco.Web/PublishedCache/NuCache/Snap/GenObj.cs +++ /dev/null @@ -1,37 +0,0 @@ -using System; -using System.Threading; - -namespace Umbraco.Web.PublishedCache.NuCache.Snap -{ - internal class GenObj - { - public GenObj(long gen) - { - Gen = gen; - WeakGenRef = new WeakReference(null); - } - - public GenRef GetGenRef() - { - // not thread-safe but always invoked from within a lock - var genRef = (GenRef)WeakGenRef.Target; - if (genRef == null) - WeakGenRef.Target = genRef = new GenRef(this); - return genRef; - } - - public readonly long Gen; - public readonly WeakReference WeakGenRef; - public int Count; - - public void Reference() - { - Interlocked.Increment(ref Count); - } - - public void Release() - { - Interlocked.Decrement(ref Count); - } - } -} diff --git a/src/Umbraco.Web/PublishedCache/NuCache/Snap/GenRef.cs b/src/Umbraco.Web/PublishedCache/NuCache/Snap/GenRef.cs deleted file mode 100644 index ade0251b8d6f..000000000000 --- a/src/Umbraco.Web/PublishedCache/NuCache/Snap/GenRef.cs +++ /dev/null @@ -1,13 +0,0 @@ -namespace Umbraco.Web.PublishedCache.NuCache.Snap -{ - internal class GenRef - { - public GenRef(GenObj genObj) - { - GenObj = genObj; - } - - public readonly GenObj GenObj; - public long Gen => GenObj.Gen; - } -} diff --git a/src/Umbraco.Web/PublishedCache/NuCache/Snap/LinkedNode.cs b/src/Umbraco.Web/PublishedCache/NuCache/Snap/LinkedNode.cs deleted file mode 100644 index 20d7e7ddcd2e..000000000000 --- a/src/Umbraco.Web/PublishedCache/NuCache/Snap/LinkedNode.cs +++ /dev/null @@ -1,20 +0,0 @@ -namespace Umbraco.Web.PublishedCache.NuCache.Snap -{ - internal class LinkedNode - where TValue : class - { - public LinkedNode(TValue value, long gen, LinkedNode next = null) - { - Value = value; - Gen = gen; - Next = next; - } - - public readonly long Gen; - - // reading & writing references is thread-safe on all .NET platforms - // mark as volatile to ensure we always read the correct value - public volatile TValue Value; - public volatile LinkedNode Next; - } -} \ No newline at end of file diff --git a/src/Umbraco.Web/PublishedCache/NuCache/SnapDictionary.cs b/src/Umbraco.Web/PublishedCache/NuCache/SnapDictionary.cs index c5b1df120648..30f6e7e63815 100644 --- a/src/Umbraco.Web/PublishedCache/NuCache/SnapDictionary.cs +++ b/src/Umbraco.Web/PublishedCache/NuCache/SnapDictionary.cs @@ -5,7 +5,6 @@ using System.Threading; using System.Threading.Tasks; using Umbraco.Core.Scoping; -using Umbraco.Web.PublishedCache.NuCache.Snap; namespace Umbraco.Web.PublishedCache.NuCache { @@ -21,9 +20,9 @@ internal class SnapDictionary // This class is optimized for many readers, few writers // Readers are lock-free - private readonly ConcurrentDictionary> _items; - private readonly ConcurrentQueue _genObjs; - private GenObj _genObj; + private readonly ConcurrentDictionary _items; + private readonly ConcurrentQueue _generationObjects; + private GenerationObject _generationObject; private readonly object _wlocko = new object(); private readonly object _rlocko = new object(); private long _liveGen, _floorGen; @@ -41,9 +40,9 @@ internal class SnapDictionary public SnapDictionary() { - _items = new ConcurrentDictionary>(); - _genObjs = new ConcurrentQueue(); - _genObj = null; // no initial gen exists + _items = new ConcurrentDictionary(); + _generationObjects = new ConcurrentQueue(); + _generationObject = null; // no initial gen exists _liveGen = _floorGen = 0; _nextGen = false; // first time, must create a snapshot _collectAuto = true; // collect automatically by default @@ -87,13 +86,12 @@ private class WriteLockInfo } // a scope contextual that represents a locked writer to the dictionary - private class ScopedWriteLock : ScopeContextualBase + private class SnapDictionaryWriter : ScopeContextualBase { private readonly WriteLockInfo _lockinfo = new WriteLockInfo(); - private readonly SnapDictionary _dictionary; - private int _released; + private SnapDictionary _dictionary; - public ScopedWriteLock(SnapDictionary dictionary, bool scoped) + public SnapDictionaryWriter(SnapDictionary dictionary, bool scoped) { _dictionary = dictionary; dictionary.Lock(_lockinfo, scoped); @@ -101,19 +99,17 @@ public ScopedWriteLock(SnapDictionary dictionary, bool scoped) public override void Release(bool completed) { - if (Interlocked.CompareExchange(ref _released, 1, 0) != 0) - return; + if (_dictionary == null) return; _dictionary.Release(_lockinfo, completed); + _dictionary = null; } } // gets a scope contextual representing a locked writer to the dictionary - // the dict is write-locked until the write-lock is released - // which happens when it is disposed (non-scoped) - // or when the scope context exits (scoped) - public IDisposable GetScopedWriteLock(IScopeProvider scopeProvider) + // GetScopedWriter? should the dict have a ref onto the scope provider? + public IDisposable GetWriter(IScopeProvider scopeProvider) { - return ScopeContextualBase.Get(scopeProvider, _instanceId, scoped => new ScopedWriteLock(this, scoped)); + return ScopeContextualBase.Get(scopeProvider, _instanceId, scoped => new SnapDictionaryWriter(this, scoped)); } private void Lock(WriteLockInfo lockInfo, bool forceGen = false) @@ -133,18 +129,14 @@ private void Lock(WriteLockInfo lockInfo, bool forceGen = false) //RuntimeHelpers.PrepareConstrainedRegions(); try { } finally { - // increment the lock count, and register that this lock is counting _wlocked++; lockInfo.Count = true; - - if (_nextGen == false || (forceGen && _wlocked == 1)) + if (_nextGen == false || (forceGen && _wlocked == 1)) // if true already... ok to have "holes" in generation objects { // because we are changing things, a new generation // is created, which will trigger a new snapshot - if (_nextGen) - _genObjs.Enqueue(_genObj = new GenObj(_liveGen)); + _nextGen = true; _liveGen += 1; - _nextGen = true; // this is the ONLY place where _nextGen becomes true } } } @@ -161,10 +153,6 @@ private void Lock(ReadLockInfo lockInfo) private void Release(WriteLockInfo lockInfo, bool commit = true) { - // if the lock wasn't taken in the first place, do nothing - if (!lockInfo.Taken) - return; - if (commit == false) { var rtaken = false; @@ -173,7 +161,6 @@ private void Release(WriteLockInfo lockInfo, bool commit = true) Monitor.Enter(_rlocko, ref rtaken); try { } finally { - // forget about the temp. liveGen _nextGen = false; _liveGen -= 1; } @@ -196,9 +183,8 @@ private void Release(WriteLockInfo lockInfo, bool commit = true) } } - // decrement the lock count, if counting, then exit the lock if (lockInfo.Count) _wlocked--; - Monitor.Exit(_wlocko); + if (lockInfo.Taken) Monitor.Exit(_wlocko); } private void Release(ReadLockInfo lockInfo) @@ -212,9 +198,9 @@ private void Release(ReadLockInfo lockInfo) public int Count => _items.Count; - private LinkedNode GetHead(TKey key) + private LinkedNode GetHead(TKey key) { - _items.TryGetValue(key, out var link); // else null + _items.TryGetValue(key, out LinkedNode link); // else null return link; } @@ -235,7 +221,7 @@ public void Set(TKey key, TValue value) // for an older gen - if value is different then insert a new // link for the new gen, with the new value if (link.Value != value) - _items.TryUpdate(key, new LinkedNode(value, _liveGen, link), link); + _items.TryUpdate(key, new LinkedNode(value, _liveGen, link), link); } else { @@ -249,7 +235,7 @@ public void Set(TKey key, TValue value) } else { - _items.TryAdd(key, new LinkedNode(value, _liveGen)); + _items.TryAdd(key, new LinkedNode(value, _liveGen)); } } finally @@ -275,7 +261,7 @@ public void Clear() { if (kvp.Value.Gen < _liveGen) { - var link = new LinkedNode(null, _liveGen, kvp.Value); + var link = new LinkedNode(null, _liveGen, kvp.Value); _items.TryUpdate(kvp.Key, link, kvp.Value); } else @@ -351,12 +337,12 @@ public Snapshot CreateSnapshot() { Lock(lockInfo); - // if no next generation is required, and we already have a gen object, - // use it to create a new snapshot - if (_nextGen == false && _genObj != null) - return new Snapshot(this, _genObj.GetGenRef()); + // if no next generation is required, and we already have one, + // use it and create a new snapshot + if (_nextGen == false && _generationObject != null) + return new Snapshot(this, _generationObject.GetReference()); - // else we need to try to create a new gen object + // else we need to try to create a new gen ref // whether we are wlocked or not, noone can rlock while we do, // so _liveGen and _nextGen are safe if (_wlocked > 0) // volatile, cannot ++ but could -- @@ -364,32 +350,29 @@ public Snapshot CreateSnapshot() // write-locked, cannot use latest gen (at least 1) so use previous var snapGen = _nextGen ? _liveGen - 1 : _liveGen; - // create a new gen object if we don't already have one - // (happens the first time a snapshot is created) - if (_genObj == null) - _genObjs.Enqueue(_genObj = new GenObj(snapGen)); - - // if we have one already, ensure it's consistent - else if (_genObj.Gen != snapGen) + // create a new gen ref unless we already have it + if (_generationObject == null) + _generationObjects.Enqueue(_generationObject = new GenerationObject(snapGen)); + else if (_generationObject.Gen != snapGen) throw new Exception("panic"); } else { - // not write-locked, can use latest gen (_liveGen), create a corresponding new gen object - _genObjs.Enqueue(_genObj = new GenObj(_liveGen)); + // not write-locked, can use latest gen, create a new gen ref + _generationObjects.Enqueue(_generationObject = new GenerationObject(_liveGen)); _nextGen = false; // this is the ONLY thing that triggers a _liveGen++ } // so... - // the genObj has a weak ref to the genRef, and is queued - // the snapshot has a ref to the genRef, which has a ref to the genObj - // when the snapshot is disposed, it decreases genObj counter + // the genRefRef has a weak ref to the genRef, and is queued + // the snapshot has a ref to the genRef, which has a ref to the genRefRef + // when the snapshot is disposed, it decreases genRefRef counter // so after a while, one of these conditions is going to be true: - // - genObj.Count is zero because all snapshots have properly been disposed - // - genObj.WeakGenRef is dead because all snapshots have been collected + // - the genRefRef counter is zero because all snapshots have properly been disposed + // - the genRefRef weak ref is dead because all snapshots have been collected // in both cases, we will dequeue and collect - var snapshot = new Snapshot(this, _genObj.GetGenRef()); + var snapshot = new Snapshot(this, _generationObject.GetReference()); // reading _floorGen is safe if _collectTask is null if (_collectTask == null && _collectAuto && _liveGen - _floorGen > CollectMinGenDelta) @@ -433,16 +416,16 @@ private Task CollectAsyncLocked() private void Collect() { // see notes in CreateSnapshot - while (_genObjs.TryPeek(out var genObj) && (genObj.Count == 0 || genObj.WeakGenRef.IsAlive == false)) + while (_generationObjects.TryPeek(out GenerationObject generationObject) && (generationObject.Count == 0 || generationObject.WeakReference.IsAlive == false)) { - _genObjs.TryDequeue(out genObj); // cannot fail since TryPeek has succeeded - _floorGen = genObj.Gen; + _generationObjects.TryDequeue(out generationObject); // cannot fail since TryPeek has succeeded + _floorGen = generationObject.Gen; } Collect(_items); } - private void Collect(ConcurrentDictionary> dict) + private void Collect(ConcurrentDictionary dict) { // it is OK to enumerate a concurrent dictionary and it does not lock // it - and here it's not an issue if we skip some items, they will be @@ -477,7 +460,7 @@ private void Collect(ConcurrentDictionary> dict) // not live, null value, no next link = remove that one -- but only if // the dict has not been updated, have to do it via ICollection<> (thanks // Mr Toub) -- and if the dict has been updated there is nothing to collect - var idict = dict as ICollection>>; + var idict = dict as ICollection>; /*var removed =*/ idict.Remove(kvp); //Console.WriteLine("remove (" + (removed ? "true" : "false") + ")"); continue; @@ -502,14 +485,14 @@ private void Collect(ConcurrentDictionary> dict) { task = _collectTask; } - return task ?? Task.CompletedTask; + return task ?? Task.FromResult(0); //if (task != null) // await task; } - public long GenCount => _genObjs.Count; + public long GenCount => _generationObjects.Count; - public long SnapCount => _genObjs.Sum(x => x.Count); + public long SnapCount => _generationObjects.Sum(x => x.Count); #endregion @@ -530,7 +513,6 @@ public TestHelper(SnapDictionary dict) public long LiveGen => _dict._liveGen; public long FloorGen => _dict._floorGen; public bool NextGen => _dict._nextGen; - public int WLocked => _dict._wlocked; public bool CollectAuto { @@ -538,15 +520,13 @@ public bool CollectAuto set => _dict._collectAuto = value; } - public GenObj GenObj => _dict._genObj; - - public ConcurrentQueue GenObjs => _dict._genObjs; + public ConcurrentQueue GenerationObjects => _dict._generationObjects; public Snapshot LiveSnapshot => new Snapshot(_dict, _dict._liveGen); public GenVal[] GetValues(TKey key) { - _dict._items.TryGetValue(key, out var link); // else null + _dict._items.TryGetValue(key, out LinkedNode link); // else null if (link == null) return new GenVal[0]; @@ -579,23 +559,35 @@ public GenVal(long gen, TValue value) #region Classes + private class LinkedNode + { + public LinkedNode(TValue value, long gen, LinkedNode next = null) + { + Value = value; + Gen = gen; + Next = next; + } + + internal readonly long Gen; + + // reading & writing references is thread-safe on all .NET platforms + // mark as volatile to ensure we always read the correct value + internal volatile TValue Value; + internal volatile LinkedNode Next; + } + public class Snapshot : IDisposable { private readonly SnapDictionary _store; - private readonly GenRef _genRef; - private readonly long _gen; // copied for perfs - private int _disposed; - - //private static int _count; - //private readonly int _thisCount; + private readonly GenerationReference _generationReference; + private long _gen; // copied for perfs - internal Snapshot(SnapDictionary store, GenRef genRef) + internal Snapshot(SnapDictionary store, GenerationReference generationReference) { _store = store; - _genRef = genRef; - _gen = genRef.GenObj.Gen; - _genRef.GenObj.Reference(); - //_thisCount = _count++; + _generationReference = generationReference; + _gen = generationReference.GenerationObject.Gen; + _generationReference.GenerationObject.Reference(); } internal Snapshot(SnapDictionary store, long gen) @@ -604,21 +596,17 @@ internal Snapshot(SnapDictionary store, long gen) _gen = gen; } - private void EnsureNotDisposed() - { - if (_disposed > 0) - throw new ObjectDisposedException("snapshot" /*+ " (" + _thisCount + ")"*/); - } - public TValue Get(TKey key) { - EnsureNotDisposed(); + if (_gen < 0) + throw new ObjectDisposedException("snapshot" /*+ " (" + _thisCount + ")"*/); return _store.Get(key, _gen); } public IEnumerable GetAll() { - EnsureNotDisposed(); + if (_gen < 0) + throw new ObjectDisposedException("snapshot" /*+ " (" + _thisCount + ")"*/); return _store.GetAll(_gen); } @@ -626,7 +614,8 @@ public bool IsEmpty { get { - EnsureNotDisposed(); + if (_gen < 0) + throw new ObjectDisposedException("snapshot" /*+ " (" + _thisCount + ")"*/); return _store.IsEmpty(_gen); } } @@ -635,20 +624,63 @@ public long Gen { get { - EnsureNotDisposed(); + if (_gen < 0) + throw new ObjectDisposedException("snapshot" /*+ " (" + _thisCount + ")"*/); return _gen; } } public void Dispose() { - if (Interlocked.CompareExchange(ref _disposed, 1, 0) != 0) - return; - _genRef?.GenObj.Release(); + if (_gen < 0) return; + _gen = -1; + _generationReference?.GenerationObject.Release(); GC.SuppressFinalize(this); } } + internal class GenerationObject + { + public GenerationObject(long gen) + { + Gen = gen; + WeakReference = new WeakReference(null); + } + + public GenerationReference GetReference() + { + // not thread-safe but always invoked from within a lock + var generationReference = (GenerationReference) WeakReference.Target; + if (generationReference == null) + WeakReference.Target = generationReference = new GenerationReference(this); + return generationReference; + } + + public readonly long Gen; + public readonly WeakReference WeakReference; + public int Count; + + public void Reference() + { + Interlocked.Increment(ref Count); + } + + public void Release() + { + Interlocked.Decrement(ref Count); + } + } + + internal class GenerationReference + { + public GenerationReference(GenerationObject generationObject) + { + GenerationObject = generationObject; + } + + public readonly GenerationObject GenerationObject; + } + #endregion } } diff --git a/src/Umbraco.Web/Umbraco.Web.csproj b/src/Umbraco.Web/Umbraco.Web.csproj index c6cbc7cbaae9..09cc7d856a0a 100755 --- a/src/Umbraco.Web/Umbraco.Web.csproj +++ b/src/Umbraco.Web/Umbraco.Web.csproj @@ -205,9 +205,6 @@ - - - From b4885f48d77aa160fc19166386678ea075ede043 Mon Sep 17 00:00:00 2001 From: Shannon Date: Mon, 1 Apr 2019 14:31:34 +1100 Subject: [PATCH 21/23] fixing bad merge problem --- build/NuSpecs/UmbracoCms.Core.nuspec | 2 +- build/NuSpecs/UmbracoCms.Web.nuspec | 2 +- build/NuSpecs/UmbracoCms.nuspec | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/build/NuSpecs/UmbracoCms.Core.nuspec b/build/NuSpecs/UmbracoCms.Core.nuspec index b257f8dcddd3..11b1870b90bc 100644 --- a/build/NuSpecs/UmbracoCms.Core.nuspec +++ b/build/NuSpecs/UmbracoCms.Core.nuspec @@ -1,6 +1,6 @@ - + UmbracoCms.Core 8.0.0 Umbraco Cms Core Binaries diff --git a/build/NuSpecs/UmbracoCms.Web.nuspec b/build/NuSpecs/UmbracoCms.Web.nuspec index 79113a7c9430..e81c19928813 100644 --- a/build/NuSpecs/UmbracoCms.Web.nuspec +++ b/build/NuSpecs/UmbracoCms.Web.nuspec @@ -1,6 +1,6 @@ - + UmbracoCms.Web 8.0.0 Umbraco Cms Core Binaries diff --git a/build/NuSpecs/UmbracoCms.nuspec b/build/NuSpecs/UmbracoCms.nuspec index 8ec14844527d..c93a82c6911e 100644 --- a/build/NuSpecs/UmbracoCms.nuspec +++ b/build/NuSpecs/UmbracoCms.nuspec @@ -1,6 +1,6 @@ - + UmbracoCms 8.0.0 Umbraco Cms From 76e2f2f38013cd3a5f2fe65315ab925afee9c494 Mon Sep 17 00:00:00 2001 From: Bjarke Berg Date: Tue, 2 Apr 2019 09:15:30 +0200 Subject: [PATCH 22/23] #4011 - Not using a unnecessary function call in views --- .../src/views/propertyeditors/grid/editors/embed.controller.js | 3 --- .../src/views/propertyeditors/grid/editors/embed.html | 2 +- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/src/Umbraco.Web.UI.Client/src/views/propertyeditors/grid/editors/embed.controller.js b/src/Umbraco.Web.UI.Client/src/views/propertyeditors/grid/editors/embed.controller.js index 8e9ea06a84fe..a45e569897a2 100644 --- a/src/Umbraco.Web.UI.Client/src/views/propertyeditors/grid/editors/embed.controller.js +++ b/src/Umbraco.Web.UI.Client/src/views/propertyeditors/grid/editors/embed.controller.js @@ -17,9 +17,6 @@ angular.module("umbraco") $scope.embedHtml = getEmbed(); }, false); - $scope.hasEmbed = function() { - return $scope.control.value !== null; - } $scope.setEmbed = function() { var embed = { submit: function(model) { diff --git a/src/Umbraco.Web.UI.Client/src/views/propertyeditors/grid/editors/embed.html b/src/Umbraco.Web.UI.Client/src/views/propertyeditors/grid/editors/embed.html index 0178c3b3ec30..8cfa71f08288 100644 --- a/src/Umbraco.Web.UI.Client/src/views/propertyeditors/grid/editors/embed.html +++ b/src/Umbraco.Web.UI.Client/src/views/propertyeditors/grid/editors/embed.html @@ -1,6 +1,6 @@
-
+
Click to embed
From a9f5d3c1e8e9e0c9ec460181d6f0153762526ae7 Mon Sep 17 00:00:00 2001 From: Bjarke Berg Date: Tue, 2 Apr 2019 09:41:29 +0200 Subject: [PATCH 23/23] #4011 - Use discreet word not disgrete --- .../src/less/components/umb-grid.less | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/src/Umbraco.Web.UI.Client/src/less/components/umb-grid.less b/src/Umbraco.Web.UI.Client/src/less/components/umb-grid.less index 5a14294ebf7f..b5abbe06bcd2 100644 --- a/src/Umbraco.Web.UI.Client/src/less/components/umb-grid.less +++ b/src/Umbraco.Web.UI.Client/src/less/components/umb-grid.less @@ -165,13 +165,13 @@ min-height: 88px; border-width: 1px; border-style: dashed; - border-color: @ui-action-disgrete-border; - color: @ui-action-disgrete-type; + border-color: @ui-action-discreet-border; + color: @ui-action-discreet-type; transition: border-color 100ms linear; &:hover { - border-color: @ui-action-disgrete-border-hover; - color: @ui-action-disgrete-type-hover; + border-color: @ui-action-discreet-border-hover; + color: @ui-action-discreet-type-hover; cursor: pointer; } } @@ -209,9 +209,9 @@ } .umb-grid .cell-tools-add { - color: @ui-action-disgrete-type; + color: @ui-action-discreet-type; &:focus, &:hover { - color: @ui-action-disgrete-type-hover; + color: @ui-action-discreet-type-hover; text-decoration: none; } } @@ -221,17 +221,17 @@ top: 50%; left: 50%; transform: translate(-50%, -50%); - color: @ui-action-disgrete-type; + color: @ui-action-discreet-type; } .umb-grid .cell-tools-add.-bar { display: block; text-align: center; padding: 5px; - border: 1px dashed @ui-action-disgrete-border; + border: 1px dashed @ui-action-discreet-border; margin: 10px; &:focus, &:hover { - border-color: @ui-action-disgrete-border-hover; + border-color: @ui-action-discreet-border-hover; } } @@ -542,13 +542,13 @@ display: inline-block; cursor: pointer; border-radius: 200px; - border: 1px solid @ui-action-disgrete-border; + border: 1px solid @ui-action-discreet-border; margin: 2px; &:hover, &:hover * { - background: @ui-action-disgrete-type-hover !important; + background: @ui-action-discreet-type-hover !important; color: @white !important; - border-color: @ui-action-disgrete-border-hover !important; + border-color: @ui-action-discreet-border-hover !important; text-decoration: none; } }