[Windows] Unwrap AggregateException when catching exceptions from Task.Wait() method #602

snrnats · 2018-02-19T18:47:47Z

We need to unwrap AggregateException when using Task.Wait() method. So that the caller will get original exception.

msftclas · 2018-02-19T18:47:58Z

All CLA requirements met.

guperrot · 2018-02-20T19:41:29Z

Hi, we are not sure what you are trying to improve here.
Can you tell us more about the intent of this change?

snrnats · 2018-02-20T21:17:46Z

In all cases Wait method is called on tasks that can throw StorageException. But Wait call will wrap up this exception with AggregateException according to docs .

Here is the issue: the caller of PutLog method expects StorageException type of exception and tries to catch it (see Changel.PersistLogAsync()), but it won’t happen because AggregateException will be thrown and no code will handle it.

I changed all other Wait calls for consistency and to prevent further issues.

guperrot · 2018-02-20T21:27:38Z

Hi, I mean, what does this change achieve for real? How is user experience impacted and what are the motivations for this?

snrnats · 2018-02-20T21:53:38Z

My app is affected by the issues I described. I've got some crash reports when using AppCenter.Push 1.3.

Application Specific Information:
TaskExceptionHolder_UnhandledException (AggregateException_ctor_DefaultMessage (The storage operation failed))

Exception Stack:
unknown location
SharedLibrary!<BaseAddress>+0x4eb21d
System.Threading.Tasks.Task.Wait(Int32 millisecondsTimeout, Threading.CancellationToken cancellationToken)
System.Threading.Tasks.Task.Wait()
Microsoft.AppCenter.Storage.Storage.<>c__DisplayClass12_0.<PutLog>b__0()
System.Action.Invoke()
SharedLibrary!<BaseAddress>+0x4f52e5
SharedLibrary!<BaseAddress>+0x4f520c
System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
...

SQLite.SQLiteException: Busy
Microsoft.AppCenter.Storage.StorageException: The storage operation failed
System.AggregateException: AggregateException_ctor_DefaultMessage (The storage operation failed)
System.AggregateException: TaskExceptionHolder_UnhandledException (AggregateException_ctor_DefaultMessage (The storage operation failed))

guperrot · 2018-02-20T21:57:35Z

Do you have a reproduction scenario for this crash?

snrnats · 2018-02-20T22:12:08Z

I don't have steps. It reproduces not very often according to crash count. For now I can tell that the crash occurs only while executing code from in process background task.

guperrot · 2018-02-20T22:32:52Z

But how do we know this patch solves a crash that we don't know how to reproduce?

snrnats · 2018-02-21T06:39:58Z

My crash logs say that app crashes because AggregateException isn't handled in the code. In our case the AggregateException contains StorageException. Try-catch for StorageException is already presented in
Changel.PersistLogAsync(). We just need to unwrap the exception.

It will prevent the crash. Although I am not sure about the reason of underlying SQLite.SQLiteException: Busy

guperrot

Please try adding a unit test that (with mocks) that would crash if we don't have this patch.

guperrot · 2018-02-21T20:05:33Z

SDK/AppCenter/Microsoft.AppCenter.Windows.Shared/Storage/Storage.cs

@@ -52,7 +53,7 @@ public Storage() : this(DefaultAdapter())
        internal Storage(IStorageAdapter adapter)
        {
            _storageAdapter = adapter;
-            _queue.Add(new Task(() => InitializeDatabaseAsync().Wait()));


Like @MatkovIvan mentioned in a conversation we had, just using .GetAwaiter().GetResult() would remove the need for an extension utils, please switch to that and remove the additional extension.

snrnats · 2018-02-21T21:06:44Z

Hi @guperrot
I will try to add unit test a little bit later.

…ion if any are thrown

snrnats · 2018-02-22T10:31:37Z

Tests/Microsoft.AppCenter.Test.Windows/Storage/FakeStorageTest.cs

@@ -85,7 +85,7 @@ public void GetLogsQueryError()
            var mockAdapter = new Mock<IStorageAdapter>();
            mockAdapter.Setup(
                    a => a.GetAsync(It.IsAny<PredType>(), It.IsAny<int>()))
-                .Throws(new StorageException());
+                .ThrowsAsync(new StorageException());


ThrowsAsync() allows to setup mock that returns faulted task. Throws() throws right away without returning a task.

guperrot · 2018-02-22T22:38:30Z

Tests/Microsoft.AppCenter.Test.Windows/TaskExtension.cs

@@ -8,26 +8,11 @@ public static class TaskExtension
    {


Hi, thanks for the changes. However since we use GetAwaiter().GetResult() directly in code, we should remove this utility.

snrnats · 2018-02-23T08:00:57Z

Hi. I noticed strange behavior of unit tests. Running all tests complete successfully, but if I filter tests with "Storage" and run them then ThrowStorageExceptionInDeleteLogsTime will fail.

Debug Trace:
[AppCenter] WARN: Channel is temporarily disabled; log was saved to disk
[AppCenter] DEBUG: CheckPendingLogs(channelName) pending log count: 1
[AppCenter] DEBUG: triggerIngestion(channelName) pendingLogCount=1
[AppCenter] DEBUG: CheckPendingLogs(channelName) pending log count: 0
[AppCenter] WARN: Could not delete logs for batch test-batch-id
Microsoft.AppCenter.Storage.StorageException: The storage operation failed
   at Moq.MethodCall.Execute(ICallContext call)
   at Moq.MethodCallReturn`2.Execute(ICallContext call)
   at Moq.ExecuteCall.HandleIntercept(ICallContext invocation, InterceptorContext ctx, CurrentInterceptContext localctx)
   at Moq.Interceptor.Intercept(ICallContext invocation)
   at Moq.Proxy.CastleProxyFactory.Interceptor.Intercept(IInvocation invocation)
   at Castle.DynamicProxy.AbstractInvocation.Proceed()
   at Castle.Proxies.ObjectProxy_1.DeleteLogs(String channelName, String batchId)
   at Microsoft.AppCenter.Channel.Channel.HandleSendingSuccess(State state, String batchId) in D:\Work\AppCenter-SDK-DotNet\SDK\AppCenter\Microsoft.AppCenter.Windows.Shared\Channel\Channel.cs:line 432

Seems like 'HandleSendingSuccess' needs to await for _storage.DeleteLogs(Name, batchId), but even then the test will fail. Do you observe the same behavior?

reset feedback

guperrot

On develop if I run those 4 tests at once like you did, they are all green.
However on your branch it fails so this is a regression that needs to be fixed.

snrnats · 2018-03-01T13:09:11Z

Waiting for a next release of moq4 where an issue affecting this PR will be resolved

guperrot · 2018-04-03T01:13:54Z

Hi @snrnats, are you still blocked on moq release?

snrnats · 2018-04-03T10:10:29Z

Hi @guperrot . Yes, still waiting for the next moq4 release.

I'm trying to reach out to authors of the moq4, hope they will give us more information about the next release

Update: probable there will be a release of moq4 in the next few days

guperrot · 2018-04-13T21:01:28Z

You will also need to fix the conflicts, a big refactoring was introduced to address #517.

MatkovIvan · 2018-04-13T21:11:58Z

Tests/Microsoft.AppCenter.Test.Windows/Storage/FakeStorageTest.cs

@@ -85,11 +85,11 @@ public void GetLogsQueryError()
            var mockAdapter = new Mock<IStorageAdapter>();
            mockAdapter.Setup(
                    a => a.GetAsync(It.IsAny<PredType>(), It.IsAny<int>()))
-                .Throws(new StorageException());
+                .ThrowsAsync(new StorageException());


You can use

.Throws(TaskExtension.GetFaultedTask(new StorageException()))

to avoid moq bug.

This line of code produce an error Error CS1503 Argument 1: cannot convert from 'System.Threading.Tasks.Task<Microsoft.AppCenter.Storage.StorageException>' to 'System.Exception'.

Anyway Throws() is implemented differently then ThrowsAsync(). I think that in most cases we should use ThrowsAsync(). Compare

/// Throws exception private static Task Throw() { throw new Exception(); return Task.Delay(1000); } /// Return faulted task private static async Task ThrowAsync() { throw new Exception(); await Task.Delay(1000); }

I have another way to rewrite it. We could use Returns() with a faulted task. It will look bulky but will do what was intended.
.Returns(TaskExtension.GetFaultedTask<List<Microsoft.AppCenter.Storage.Storage.LogEntry>>(new StorageException()))

I can try to change it in a such way if we don't want to wait for a moq4 release.

Oops, I mean Returns with a faulted task of course. You understood my idea correctly 👍

MatkovIvan · 2018-04-13T21:17:34Z

Tests/Microsoft.AppCenter.Test.Windows/Storage/StorageTest.cs

@@ -24,7 +24,7 @@ public void InitializeStorageTest()
        [TestMethod]
        public void CountEmptyStorage()
        {
-            var count = _storage.CountLogsAsync(StorageTestChannelName).RunNotAsync();
+            var count = _storage.CountLogsAsync(StorageTestChannelName).GetAwaiter().GetResult();


Tests can be async and I've already replaced it partially.
async/await looks like right replacement instead of GetAwaiter().GetResult()
You don't necessarily change all the tests in the project, only those that relate to your fix.

…d9c724)

snrnats · 2018-04-14T08:13:29Z

I reverted changes that are not related to fix

Unwrap AggregateException on calling Task.Wait() method

afb56ed

guperrot suggested changes Feb 21, 2018

View reviewed changes

Use GetAwaiter().GetResult instead of util method

923e5b0

snrnats added 2 commits February 22, 2018 13:00

Use GetAwaiter().GetResult() instead of Result to get original except…

a4ea73c

…ion if any are thrown

Updated unit tests to properly detect exceptions thrown by Storage

080976a

snrnats commented Feb 22, 2018

View reviewed changes

guperrot previously requested changes Feb 22, 2018

View reviewed changes

Inlined GetAwaiter().GetResult() in unit tests

1d9c724

guperrot suggested changes Feb 23, 2018

View reviewed changes

guperrot added the bug label Feb 27, 2018

guperrot changed the title ~~Unwrap AggregateException when catching exceptions from Task.Wait() method~~ [Windows] Unwrap AggregateException when catching exceptions from Task.Wait() method Apr 10, 2018

MatkovIvan reviewed Apr 13, 2018

View reviewed changes

snrnats added 2 commits April 14, 2018 10:46

Inlined GetAwaiter().GetResult() in unit tests (reverted from commit 1…

ade9829

…d9c724)

Merge branch 'develop' into develop-snrnats

9b0a608

Removed unused 'using'

ada7d55

Use Mock.Returns() with faulted task until Mock.ThrowsAsync is fixed.

7478fca

guperrot approved these changes Apr 16, 2018

View reviewed changes

guperrot merged commit 478d442 into microsoft:develop Apr 16, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Windows] Unwrap AggregateException when catching exceptions from Task.Wait() method #602

[Windows] Unwrap AggregateException when catching exceptions from Task.Wait() method #602

snrnats commented Feb 19, 2018

msftclas commented Feb 19, 2018 •

edited

Loading

guperrot commented Feb 20, 2018

snrnats commented Feb 20, 2018

guperrot commented Feb 20, 2018

snrnats commented Feb 20, 2018 •

edited

Loading

guperrot commented Feb 20, 2018

snrnats commented Feb 20, 2018 •

edited

Loading

guperrot commented Feb 20, 2018 •

edited

Loading

snrnats commented Feb 21, 2018

guperrot left a comment

guperrot Feb 21, 2018

snrnats commented Feb 21, 2018

snrnats Feb 22, 2018

guperrot Feb 22, 2018

snrnats commented Feb 23, 2018

guperrot left a comment

snrnats commented Mar 1, 2018

guperrot commented Apr 3, 2018

snrnats commented Apr 3, 2018 •

edited

Loading

guperrot commented Apr 13, 2018

MatkovIvan Apr 13, 2018

snrnats Apr 14, 2018 •

edited

Loading

MatkovIvan Apr 14, 2018 •

edited

Loading

MatkovIvan Apr 13, 2018

snrnats commented Apr 14, 2018

[Windows] Unwrap AggregateException when catching exceptions from Task.Wait() method #602

[Windows] Unwrap AggregateException when catching exceptions from Task.Wait() method #602

Conversation

snrnats commented Feb 19, 2018

msftclas commented Feb 19, 2018 • edited Loading

guperrot commented Feb 20, 2018

snrnats commented Feb 20, 2018

guperrot commented Feb 20, 2018

snrnats commented Feb 20, 2018 • edited Loading

guperrot commented Feb 20, 2018

snrnats commented Feb 20, 2018 • edited Loading

guperrot commented Feb 20, 2018 • edited Loading

snrnats commented Feb 21, 2018

guperrot left a comment

Choose a reason for hiding this comment

guperrot Feb 21, 2018

Choose a reason for hiding this comment

snrnats commented Feb 21, 2018

snrnats Feb 22, 2018

Choose a reason for hiding this comment

guperrot Feb 22, 2018

Choose a reason for hiding this comment

snrnats commented Feb 23, 2018

guperrot left a comment

Choose a reason for hiding this comment

snrnats commented Mar 1, 2018

guperrot commented Apr 3, 2018

snrnats commented Apr 3, 2018 • edited Loading

guperrot commented Apr 13, 2018

MatkovIvan Apr 13, 2018

Choose a reason for hiding this comment

snrnats Apr 14, 2018 • edited Loading

Choose a reason for hiding this comment

MatkovIvan Apr 14, 2018 • edited Loading

Choose a reason for hiding this comment

MatkovIvan Apr 13, 2018

Choose a reason for hiding this comment

snrnats commented Apr 14, 2018

msftclas commented Feb 19, 2018 •

edited

Loading

snrnats commented Feb 20, 2018 •

edited

Loading

snrnats commented Feb 20, 2018 •

edited

Loading

guperrot commented Feb 20, 2018 •

edited

Loading

snrnats commented Apr 3, 2018 •

edited

Loading

snrnats Apr 14, 2018 •

edited

Loading

MatkovIvan Apr 14, 2018 •

edited

Loading