Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Issue 688 AsyncHelper.WaitForCompletion leaks unobserved exceptions #692

Merged
merged 13 commits into from Oct 22, 2020
Merged

Issue 688 AsyncHelper.WaitForCompletion leaks unobserved exceptions #692

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
ad763b8
Create an example of how TaskHelper.WaitForCompletion leaks unobserve…
Aug 13, 2020
c4d6ed2
Use internalsvisibleto rather than making things public
Aug 13, 2020
2618c65
Tidy up pre PR
Aug 14, 2020
60cea01
Add more detailed info on the unhandled exception to diagnose build f…
Aug 14, 2020
4a8b3e9
Patch fix to netfx version of SqlUtil too
Aug 14, 2020
3b09392
Stop other unit tests from leaving unobserved exceptions
Aug 15, 2020
a2ea136
Leak even fewer unobserved exceptions
Aug 15, 2020
fc09c96
Merge branch 'master' into UnobservedExceptionPR
Sep 1, 2020
08b54d4
View the exception from the continuation
Sep 1, 2020
f3aed3b
Observe the exception from _the other_ SqlUtil
Sep 1, 2020
07ad6ec
Swap lambda to local function
jm771 Sep 1, 2020
c350827
Merge https://github.com/dotnet/SqlClient into UnobservedExceptionPR
jm771 Oct 21, 2020
04ac97f
Merge branch 'UnobservedExceptionPR' of https://github.com/jm771/SqlC…
jm771 Oct 21, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlUtil.cs
View file
Open in desktop
Expand Up @@ -17,6 +17,8 @@
using System.Transactions;
using Microsoft.Data.Common;

[assembly: InternalsVisibleTo("FunctionalTests")]
cheenamalhotra marked this conversation as resolved.
Show resolved Hide resolved

namespace Microsoft.Data.SqlClient
{
internal static class AsyncHelper
Expand Down Expand Up @@ -204,6 +206,7 @@ internal static void WaitForCompletion(Task task, int timeout, Action onTimeout
}
if (!task.IsCompleted)
{
task.ContinueWith(_ => { }); //Ensure the task does not leave an unobserved exception
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you verify that this doesn't allocate a delegate and closure/state object on each invocation? I've done a lot of work to try and reduce allocations for async calls and I'd like to make sure things aren't regressed without good reason.

Copy link
Contributor Author

@jm771 jm771 Aug 15, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't do - as there is nothing to capture in the closure - did this as an experiment to confirm:

        private static Action<Task> GetLambdaNoClosure(int context)
	{
		return _ => { };
	}
	
	private static Action<Task> GetLambdaWithClosure(int context)
	{
		return _ => { context++; };
	}
	
	public static void Main()
	{
		var a = GetLambdaNoClosure(1);
		var b = GetLambdaNoClosure(2);
		Console.WriteLine(object.ReferenceEquals(a, b));
		
		var c = GetLambdaWithClosure(1);
		var d = GetLambdaWithClosure(2);
		Console.WriteLine(object.ReferenceEquals(c, d));
	}

Output:
True False

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. Correctness has priority before performance anyway so probably not is good enough for me.

if (onTimeout != null)
{
onTimeout();
Expand Down
3 changes: 3 additions & 0 deletions src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlUtil.cs
View file
Open in desktop
Expand Up @@ -17,6 +17,8 @@
using System.Threading.Tasks;
using SysTx = System.Transactions;

[assembly: InternalsVisibleTo("FunctionalTests")]

namespace Microsoft.Data.SqlClient
{
using Microsoft.Data.Common;
Expand Down Expand Up @@ -189,6 +191,7 @@ internal static void WaitForCompletion(Task task, int timeout, Action onTimeout
}
if (!task.IsCompleted)
{
task.ContinueWith(_ => { }); //Ensure the task does not leave an unobserved exception
if (onTimeout != null)
{
onTimeout();
Expand Down
1 change: 1 addition & 0 deletions src/Microsoft.Data.SqlClient/tests/FunctionalTests/Microsoft.Data.SqlClient.Tests.csproj
View file
Open in desktop
Expand Up @@ -40,6 +40,7 @@
<Compile Include="SqlCredentialTest.cs" />
<Compile Include="SqlDataRecordTest.cs" />
<Compile Include="SqlExceptionTest.cs" />
<Compile Include="SqlHelperTest.cs" />
<Compile Include="SqlParameterTest.cs" />
<Compile Include="SqlClientFactoryTest.cs" />
<Compile Include="SqlErrorCollectionTest.cs" />
Expand Down
55 changes: 55 additions & 0 deletions src/Microsoft.Data.SqlClient/tests/FunctionalTests/SqlHelperTest.cs
View file
Open in desktop
@@ -0,0 +1,55 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System;
using System.Threading;
using System.Threading.Tasks;
using Xunit;

namespace Microsoft.Data.SqlClient.Tests
{
public class SqlHelperTest
{
private void TimeOutATask()
{
TaskCompletionSource<bool> tcs = new TaskCompletionSource<bool>();
AsyncHelper.WaitForCompletion(tcs.Task, 1); //Will time out as task uncompleted
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't love that this line causes the unit test to take a second to run - but that's the minimum timeout on WaitForCompletion

tcs.SetException(new TimeoutException("Dummy timeout exception")); //Our task now completes with an error
}

private Exception UnwrapException(Exception e)
{
return e?.InnerException != null ? UnwrapException(e.InnerException) : e;
}

[Fact]
public void WaitForCompletion_DoesNotCreateUnobservedException()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test does need a bit of rework to handle randomness. It fails randomly in pipelines.

You'll notice if you run this test in loop, the second round does not pass ever. Any thoughts why can't we run this test in second round? It maybe a hint towards random errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was just looking into this - it seems the fix I had written to WaitForCompletion wasn't actually a fix. I needed to make the continuation actually observe the exception, the behaviour I had lead to a race condition, where if the continuation hadn't been executed at the point we called GC.Collect, then there was still a reference to the original task and it wasn't Collected. Creating a false positive for me having fixed it.

I proved this by modifying WaitForCompletion to return the continuation, and waited for it to complete, at which point the error happened deterministically. Making the continuation observe the exception fixed the error. My bad, I misinterpreted the text here: https://docs.microsoft.com/en-us/dotnet/api/system.threading.tasks.taskcontinuationoptions?view=netcore-3.1 saying "If you do not access the Exception property, the exception is unhandled." To only apply to OnlyOnFaulted, when it applies to all TaskContinuationOptions.

I can't see a way to make the failure deterministic without passing some sort of signal back from the continuation, which doesn't fit the signature and isn't desirable.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part is just a test so make it as messy as it needs to be to get a deterministic answer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does pass deterministically now with the fix in place - what I couldn't get deterministic was the failure without the fix in place.

{
var unobservedExceptionHappenedEvent = new AutoResetEvent(false);
Exception unhandledException = null;
EventHandler<UnobservedTaskExceptionEventArgs> handleUnobservedException =
(o, a) => { unhandledException = a.Exception; unobservedExceptionHappenedEvent.Set(); };
jm771 marked this conversation as resolved.
Show resolved Hide resolved

TaskScheduler.UnobservedTaskException += handleUnobservedException;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't much like messing with global state from a unit test, but I'm not sure of what other way to test this. I had a look at TaskExceptionHolder (which is what is responsible for raising the event when finalized) but it looks anything along those lines would be messing with the internals of the Task system too much.


try
{
TimeOutATask(); //Create the task in another function so the task has no reference remaining
GC.Collect(); //Force collection of unobserved task
GC.WaitForPendingFinalizers();

bool unobservedExceptionHappend = unobservedExceptionHappenedEvent.WaitOne(1);
if (unobservedExceptionHappend) //Save doing string interpolation in the happy case
{
var e = UnwrapException(unhandledException);
Assert.False(true, $"Did not expect an unobserved exception, but found a {e?.GetType()} with message \"{e?.Message}\"");
}
}
finally
{
TaskScheduler.UnobservedTaskException -= handleUnobservedException;
}
}
}
}