fix deadlock bug when one of requests in changeset failed.

chinadragon0515 · Sep 9, 2016 · b930dfb · b930dfb
1 parent 4587b3f
commit b930dfb
Show file tree

Hide file tree

Showing 10 changed files with 123 additions and 43 deletions.
diff --git 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<ODataBatchResponseItem> SendRequestAsync(
             this.SetChangeSetProperty(changeSetProperty);
 
             Dictionary<string, string> contentIdToLocationMapping = new Dictionary<string, string>();
-            List<Task<HttpResponseMessage>> responseTasks = new List<Task<HttpResponseMessage>>();
+            var responseTasks = new List<Task<Task<HttpResponseMessage>>>();
+
             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<HttpResponseMessage> tcs = new TaskCompletionSource<HttpResponseMessage>();
+                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<ODataBatchResponseItem> SendRequestAsync(
             List<HttpResponseMessage> responses = new List<HttpResponseMessage>();
             try
             {
-                foreach (Task<HttpResponseMessage> 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
@@ -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,13 +30,16 @@ public RestierChangeSetProperty(RestierBatchChangeSetRequestItem changeSetReques
             this.changeSetRequestItem = changeSetRequestItem;
             this.changeSetCompletedTaskSource = new TaskCompletionSource<bool>();
             this.subRequestCount = this.changeSetRequestItem.Requests.Count();
+            this.Exceptions = new List<Exception>();
         }
 
         /// <summary>
         /// Gets or sets the changeset.
         /// </summary>
         public ChangeSet ChangeSet { get; set; }
 
+        public IList<Exception> Exceptions { get; set; }
+
         /// <summary>
         /// The callback to execute when the changeset is completed.
         /// </summary>
@@ -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
@@ -28,10 +28,10 @@ namespace Microsoft.Restier.Publishers.OData
     internal sealed class RestierExceptionFilterAttribute : ExceptionFilterAttribute
     {
         private static readonly List<ExceptionHandlerDelegate> Handlers = new List<ExceptionHandlerDelegate>
-            {
-                HandleChangeSetValidationException,
-                HandleCommonException
-            };
+        {
+            HandleChangeSetValidationException,
+            HandleCommonException
+        };
 
         private delegate Task<HttpResponseMessage> ExceptionHandlerDelegate(
             HttpActionExecutedContext context,
@@ -126,6 +126,16 @@ private static Task<HttpResponseMessage> 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
@@ -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
@@ -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
diff --git a/src/Microsoft.Restier.Publishers.OData/Properties/Resources.resx b/src/Microsoft.Restier.Publishers.OData/Properties/Resources.resx
@@ -156,6 +156,9 @@
   <data name="KeyNotValidForEntityType" xml:space="preserve">
     <value>Specified key '{0}' is not a valid property of entity '{1}'.</value>
   </data>
+  <data name="ModelStateIsNotValid" xml:space="preserve">
+    <value>Model state is not valid with message {0}, please check your request.</value>
+  </data>
   <data name="MultiKeyValuesExpected" xml:space="preserve">
     <value>Only one key was specified, when multiple were expected.</value>
   </data>

diff --git 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
@@ -1,24 +1,27 @@
-// 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;
 using System.Net.Http.Headers;
 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;
 using System.Web.OData.Query;
 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<HttpResponseMessage> Get(
         /// <returns>The task object that contains the creation result.</returns>
         public async Task<IHttpActionResult> 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<IHttpActionResult> Post(EdmEntityObject edmEntityObject, Cance
         /// <returns>The task object that contains the updated result.</returns>
         public async Task<IHttpActionResult> 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<IHttpActionResult> Put(EdmEntityObject edmEntityObject, Cancel
         /// <returns>The task object that contains the updated result.</returns>
         public async Task<IHttpActionResult> 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<IHttpActionResult> 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
@@ -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