From b930dfba435137f03b5fcf510b1be0099d93ad89 Mon Sep 17 00:00:00 2001 From: mirsking Date: Fri, 9 Sep 2016 13:35:18 +0800 Subject: [PATCH] fix deadlock bug when one of requests in changeset failed. --- .../Batch/RestierBatchChangeSetRequestItem.cs | 39 ++++++++++++++-- .../Batch/RestierChangeSetProperty.cs | 46 +++++++++++-------- .../RestierExceptionFilterAttribute.cs | 18 ++++++-- .../Model/EdmHelpers.cs | 1 + .../Operation/OperationExecutor.cs | 1 + .../Properties/Resources.Designer.cs | 11 ++++- .../Properties/Resources.resx | 3 ++ .../Query/RestierQueryBuilder.cs | 1 + .../RestierController.cs | 45 +++++++++++------- .../Routing/HttpConfigurationExtensions.cs | 1 + 10 files changed, 123 insertions(+), 43 deletions(-) diff --git a/src/Microsoft.Restier.Publishers.OData/Batch/RestierBatchChangeSetRequestItem.cs b/src/Microsoft.Restier.Publishers.OData/Batch/RestierBatchChangeSetRequestItem.cs index 8095c1c7..02904b0b 100644 --- a/src/Microsoft.Restier.Publishers.OData/Batch/RestierBatchChangeSetRequestItem.cs +++ b/src/Microsoft.Restier.Publishers.OData/Batch/RestierBatchChangeSetRequestItem.cs @@ -3,6 +3,8 @@ using System; using System.Collections.Generic; +using System.Linq; +using System.Net; using System.Net.Http; using System.Threading; using System.Threading.Tasks; @@ -45,10 +47,39 @@ public override async Task SendRequestAsync( this.SetChangeSetProperty(changeSetProperty); Dictionary contentIdToLocationMapping = new Dictionary(); - List> responseTasks = new List>(); + var responseTasks = new List>>(); + foreach (HttpRequestMessage request in Requests) { - responseTasks.Add(SendMessageAsync(invoker, request, cancellationToken, contentIdToLocationMapping)); + // Since exceptions may occure before the request is sent to RestierController, + // we must catch the exceptions here and call OnChangeSetCompleted, + // so as to avoid deadlock mentioned in Github Issue #82. + TaskCompletionSource tcs = new TaskCompletionSource(); + var task = + SendMessageAsync(invoker, request, cancellationToken, contentIdToLocationMapping) + .ContinueWith( + t => + { + if (t.Exception != null) + { + var taskEx = (t.Exception.InnerExceptions != null && + t.Exception.InnerExceptions.Count == 1) + ? t.Exception.InnerExceptions.First() + : t.Exception; + changeSetProperty.Exceptions.Add(taskEx); + changeSetProperty.OnChangeSetCompleted(request); + tcs.SetException(taskEx); + } + else + { + tcs.SetResult(t.Result); + } + + return tcs.Task; + }, + cancellationToken); + + responseTasks.Add(task); } // the responseTasks will be complete after: @@ -60,9 +91,9 @@ public override async Task SendRequestAsync( List responses = new List(); try { - foreach (Task responseTask in responseTasks) + foreach (var responseTask in responseTasks) { - HttpResponseMessage response = responseTask.Result; + HttpResponseMessage response = responseTask.Result.Result; if (response.IsSuccessStatusCode) { responses.Add(response); diff --git a/src/Microsoft.Restier.Publishers.OData/Batch/RestierChangeSetProperty.cs b/src/Microsoft.Restier.Publishers.OData/Batch/RestierChangeSetProperty.cs index 59a9cbc6..f0f7c2d8 100644 --- a/src/Microsoft.Restier.Publishers.OData/Batch/RestierChangeSetProperty.cs +++ b/src/Microsoft.Restier.Publishers.OData/Batch/RestierChangeSetProperty.cs @@ -1,6 +1,8 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. See License.txt in the project root for license information. +using System; +using System.Collections.Generic; using System.Linq; using System.Net.Http; using System.Threading; @@ -28,6 +30,7 @@ public RestierChangeSetProperty(RestierBatchChangeSetRequestItem changeSetReques this.changeSetRequestItem = changeSetRequestItem; this.changeSetCompletedTaskSource = new TaskCompletionSource(); this.subRequestCount = this.changeSetRequestItem.Requests.Count(); + this.Exceptions = new List(); } /// @@ -35,6 +38,8 @@ public RestierChangeSetProperty(RestierBatchChangeSetRequestItem changeSetReques /// public ChangeSet ChangeSet { get; set; } + public IList Exceptions { get; set; } + /// /// The callback to execute when the changeset is completed. /// @@ -44,25 +49,30 @@ public Task OnChangeSetCompleted(HttpRequestMessage request) { if (Interlocked.Decrement(ref this.subRequestCount) == 0) { - this.changeSetRequestItem.SubmitChangeSet(request, this.ChangeSet) - .ContinueWith(t => - { - if (t.Exception != null) - { - var taskEx = - (t.Exception.InnerExceptions != null - && t.Exception.InnerExceptions.Count == 1) - ? - t.Exception.InnerExceptions.First() - : - t.Exception; - this.changeSetCompletedTaskSource.SetException(taskEx); - } - else + if (Exceptions.Count == 0) + { + this.changeSetRequestItem.SubmitChangeSet(request, this.ChangeSet) + .ContinueWith(t => { - this.changeSetCompletedTaskSource.SetResult(true); - } - }); + if (t.Exception != null) + { + var taskEx = + (t.Exception.InnerExceptions != null + && t.Exception.InnerExceptions.Count == 1) + ? t.Exception.InnerExceptions.First() + : t.Exception; + this.changeSetCompletedTaskSource.SetException(taskEx); + } + else + { + this.changeSetCompletedTaskSource.SetResult(true); + } + }); + } + else + { + this.changeSetCompletedTaskSource.SetException(Exceptions); + } } return this.changeSetCompletedTaskSource.Task; diff --git a/src/Microsoft.Restier.Publishers.OData/Filters/RestierExceptionFilterAttribute.cs b/src/Microsoft.Restier.Publishers.OData/Filters/RestierExceptionFilterAttribute.cs index 28daf2d3..bd3ad0a0 100644 --- a/src/Microsoft.Restier.Publishers.OData/Filters/RestierExceptionFilterAttribute.cs +++ b/src/Microsoft.Restier.Publishers.OData/Filters/RestierExceptionFilterAttribute.cs @@ -28,10 +28,10 @@ namespace Microsoft.Restier.Publishers.OData internal sealed class RestierExceptionFilterAttribute : ExceptionFilterAttribute { private static readonly List Handlers = new List - { - HandleChangeSetValidationException, - HandleCommonException - }; + { + HandleChangeSetValidationException, + HandleCommonException + }; private delegate Task ExceptionHandlerDelegate( HttpActionExecutedContext context, @@ -126,6 +126,16 @@ private static Task HandleCommonException( code = HttpStatusCode.NotImplemented; } + // When exception occured in a ChangeSet request, + // exception must be handled in OnChangeSetCompleted + // to avoid deadlock in Github Issue #82. + var changeSetProperty = context.Request.GetChangeSet(); + if (changeSetProperty != null) + { + changeSetProperty.Exceptions.Add(exception); + changeSetProperty.OnChangeSetCompleted(context.Request); + } + if (code != HttpStatusCode.Unused) { if (useVerboseErros) diff --git a/src/Microsoft.Restier.Publishers.OData/Model/EdmHelpers.cs b/src/Microsoft.Restier.Publishers.OData/Model/EdmHelpers.cs index f5244df7..1c74e871 100644 --- a/src/Microsoft.Restier.Publishers.OData/Model/EdmHelpers.cs +++ b/src/Microsoft.Restier.Publishers.OData/Model/EdmHelpers.cs @@ -7,6 +7,7 @@ using System.Web.OData; using Microsoft.Extensions.DependencyInjection; using Microsoft.OData.Edm; +using Microsoft.Restier.Publishers.OData.Properties; namespace Microsoft.Restier.Publishers.OData.Model { diff --git a/src/Microsoft.Restier.Publishers.OData/Operation/OperationExecutor.cs b/src/Microsoft.Restier.Publishers.OData/Operation/OperationExecutor.cs index d9c427e4..e941c35d 100644 --- a/src/Microsoft.Restier.Publishers.OData/Operation/OperationExecutor.cs +++ b/src/Microsoft.Restier.Publishers.OData/Operation/OperationExecutor.cs @@ -17,6 +17,7 @@ using Microsoft.Restier.Core.Operation; using Microsoft.Restier.Publishers.OData.Formatter; using Microsoft.Restier.Publishers.OData.Model; +using Microsoft.Restier.Publishers.OData.Properties; namespace Microsoft.Restier.Publishers.OData.Operation { diff --git a/src/Microsoft.Restier.Publishers.OData/Properties/Resources.Designer.cs b/src/Microsoft.Restier.Publishers.OData/Properties/Resources.Designer.cs index 3c38b074..42f3f464 100644 --- a/src/Microsoft.Restier.Publishers.OData/Properties/Resources.Designer.cs +++ b/src/Microsoft.Restier.Publishers.OData/Properties/Resources.Designer.cs @@ -8,7 +8,7 @@ // //------------------------------------------------------------------------------ -namespace Microsoft.Restier.Publishers.OData { +namespace Microsoft.Restier.Publishers.OData.Properties { using System; @@ -177,6 +177,15 @@ internal static string KeyNotValidForEntityType { } } + /// + /// Looks up a localized string similar to Model state is not valid with message {0}, please check your request.. + /// + internal static string ModelStateIsNotValid { + get { + return ResourceManager.GetString("ModelStateIsNotValid", resourceCulture); + } + } + /// /// Looks up a localized string similar to Only one key was specified, when multiple were expected.. /// diff --git a/src/Microsoft.Restier.Publishers.OData/Properties/Resources.resx b/src/Microsoft.Restier.Publishers.OData/Properties/Resources.resx index a44bf8ee..8c10cbde 100644 --- a/src/Microsoft.Restier.Publishers.OData/Properties/Resources.resx +++ b/src/Microsoft.Restier.Publishers.OData/Properties/Resources.resx @@ -156,6 +156,9 @@ Specified key '{0}' is not a valid property of entity '{1}'. + + Model state is not valid with message {0}, please check your request. + Only one key was specified, when multiple were expected. diff --git a/src/Microsoft.Restier.Publishers.OData/Query/RestierQueryBuilder.cs b/src/Microsoft.Restier.Publishers.OData/Query/RestierQueryBuilder.cs index 6a327f11..397aa1b8 100644 --- a/src/Microsoft.Restier.Publishers.OData/Query/RestierQueryBuilder.cs +++ b/src/Microsoft.Restier.Publishers.OData/Query/RestierQueryBuilder.cs @@ -10,6 +10,7 @@ using Microsoft.OData.UriParser; using Microsoft.Restier.Core; using Microsoft.Restier.Publishers.OData.Model; +using Microsoft.Restier.Publishers.OData.Properties; using ODataPath = System.Web.OData.Routing.ODataPath; namespace Microsoft.Restier.Publishers.OData.Query diff --git a/src/Microsoft.Restier.Publishers.OData/RestierController.cs b/src/Microsoft.Restier.Publishers.OData/RestierController.cs index 8eb1a128..2d1c0683 100644 --- a/src/Microsoft.Restier.Publishers.OData/RestierController.cs +++ b/src/Microsoft.Restier.Publishers.OData/RestierController.cs @@ -1,10 +1,11 @@ -// Copyright (c) Microsoft Corporation. All rights reserved. +// Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. See License.txt in the project root for license information. extern alias Net; using System; using System.Collections; using System.Collections.Generic; +using System.Globalization; using System.Linq; using System.Net; using System.Net.Http; @@ -12,6 +13,7 @@ using System.Threading; using System.Threading.Tasks; using System.Web.Http; +using System.Web.Http.ModelBinding; using System.Web.OData; using System.Web.OData.Extensions; using System.Web.OData.Formatter; @@ -19,6 +21,7 @@ using System.Web.OData.Results; using System.Web.OData.Routing; using Microsoft.Extensions.DependencyInjection; +using Microsoft.OData; using Microsoft.OData.Edm; using Microsoft.OData.UriParser; using Microsoft.Restier.Core; @@ -27,6 +30,7 @@ using Microsoft.Restier.Core.Submit; using Microsoft.Restier.Publishers.OData.Batch; using Microsoft.Restier.Publishers.OData.Model; +using Microsoft.Restier.Publishers.OData.Properties; using Microsoft.Restier.Publishers.OData.Query; // This is a must for creating response with correct extension method @@ -137,11 +141,7 @@ public async Task Get( /// The task object that contains the creation result. public async Task Post(EdmEntityObject edmEntityObject, CancellationToken cancellationToken) { - if (!this.ModelState.IsValid) - { - return BadRequest(this.ModelState); - } - + CheckModelState(); ODataPath path = this.GetPath(); IEdmEntitySet entitySet = path.NavigationSource as IEdmEntitySet; if (entitySet == null) @@ -193,11 +193,6 @@ public async Task Post(EdmEntityObject edmEntityObject, Cance /// The task object that contains the updated result. public async Task Put(EdmEntityObject edmEntityObject, CancellationToken cancellationToken) { - if (!this.ModelState.IsValid) - { - return BadRequest(this.ModelState); - } - return await this.Update(edmEntityObject, true, cancellationToken); } @@ -209,11 +204,6 @@ public async Task Put(EdmEntityObject edmEntityObject, Cancel /// The task object that contains the updated result. public async Task Patch(EdmEntityObject edmEntityObject, CancellationToken cancellationToken) { - if (!this.ModelState.IsValid) - { - return BadRequest(this.ModelState); - } - return await this.Update(edmEntityObject, false, cancellationToken); } @@ -360,6 +350,7 @@ private async Task Update( bool isFullReplaceUpdate, CancellationToken cancellationToken) { + CheckModelState(); ODataPath path = this.GetPath(); IEdmEntitySet entitySet = path.NavigationSource as IEdmEntitySet; if (entitySet == null) @@ -700,5 +691,27 @@ private IHttpActionResult CreateResult(Type resultType, object result) return (IHttpActionResult)Activator.CreateInstance(genericResultType, result, this); } + + private void CheckModelState() + { + if (!this.ModelState.IsValid) + { + var errorList = ( + from item in this.ModelState + where item.Value.Errors.Any() + select + string.Format( + CultureInfo.InvariantCulture, + "{{ Error: {0}, Exception {1} }}", + item.Value.Errors[0].ErrorMessage, + item.Value.Errors[0].Exception.Message)).ToList(); + + throw new ODataException( + string.Format( + CultureInfo.InvariantCulture, + Resources.ModelStateIsNotValid, + string.Join(";", errorList))); + } + } } } diff --git a/src/Microsoft.Restier.Publishers.OData/Routing/HttpConfigurationExtensions.cs b/src/Microsoft.Restier.Publishers.OData/Routing/HttpConfigurationExtensions.cs index 98507828..c56edff1 100644 --- a/src/Microsoft.Restier.Publishers.OData/Routing/HttpConfigurationExtensions.cs +++ b/src/Microsoft.Restier.Publishers.OData/Routing/HttpConfigurationExtensions.cs @@ -17,6 +17,7 @@ using Microsoft.OData.Edm; using Microsoft.Restier.Core; using Microsoft.Restier.Publishers.OData.Batch; +using Microsoft.Restier.Publishers.OData.Properties; using ServiceLifetime = Microsoft.OData.ServiceLifetime; namespace Microsoft.Restier.Publishers.OData