Add an internal ObjectNotDisposedException (#18605)

that we can throw/immediately catch from finalizers when an object wasn't properly disposed and there's pending work being lost or resources being leaked
Azure · Feb 9, 2021 · 5510048 · 5510048
1 parent b493f7a
commit 5510048
Show file tree

Hide file tree

Showing 6 changed files with 84 additions and 20 deletions.
diff --git a/sdk/core/Azure.Core/src/Shared/ObjectNotDisposedException.cs b/sdk/core/Azure.Core/src/Shared/ObjectNotDisposedException.cs
@@ -0,0 +1,24 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT License.
+
+using System;
+
+namespace Azure.Core
+{
+    /// <summary>
+    /// An exception thrown and immediately caught by finalizers with pending
+    /// work or resources that were not properly disposed.  This exception only
+    /// exists to notify users of incorrect usage while debugging with first
+    /// chance exceptions.
+    /// </summary>
+    internal class ObjectNotDisposedException : InvalidOperationException
+    {
+        /// <summary>
+        /// Creates a new instance of an ObjectNotDisposedException.
+        /// </summary>
+        /// <param name="message">The exception message.</param>
+        public ObjectNotDisposedException(string message) : base(message)
+        {
+        }
+    }
+}
diff --git a/sdk/core/Azure.Core/tests/Azure.Core.Tests.csproj b/sdk/core/Azure.Core/tests/Azure.Core.Tests.csproj
@@ -28,6 +28,7 @@
     <Compile Include="..\src\Shared\ConnectionString.cs" LinkBase="Shared" />
     <Compile Include="..\src\Shared\ForwardsClientCallsAttribute.cs" LinkBase="Shared" />
     <Compile Include="..\src\Shared\NoBodyResponseOfT.cs" LinkBase="Shared" />
+    <Compile Include="..\src\Shared\ObjectNotDisposedException.cs" LinkBase="Shared" />
     <Compile Include="..\src\Shared\OperationHelpers.cs" LinkBase="Shared" />
     <Compile Include="..\src\Shared\PageResponseEnumerator.cs" LinkBase="Shared" />
     <Compile Include="..\src\Shared\RetriableStream.cs" LinkBase="Shared" />

diff --git a/sdk/core/Azure.Core/tests/ObjectNotDisposedExceptionTests.cs b/sdk/core/Azure.Core/tests/ObjectNotDisposedExceptionTests.cs
@@ -0,0 +1,26 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT License.
+
+using System;
+using NUnit.Framework;
+
+namespace Azure.Core.Tests
+{
+    public class ObjectNotDisposedExceptionTests
+    {
+        [Test]
+        public void Construct()
+        {
+            string message = "Work was not cleaned up.";
+            var ex = new ObjectNotDisposedException(message);
+            Assert.AreEqual(message, ex.Message);
+        }
+
+        [Test]
+        public void IsInvalidOperationException()
+        {
+            var ex = new ObjectNotDisposedException("Work was not cleaned up.");
+            Assert.True(ex is InvalidOperationException);
+        }
+    }
+}
diff --git a/sdk/search/Azure.Search.Documents/src/Azure.Search.Documents.csproj b/sdk/search/Azure.Search.Documents/src/Azure.Search.Documents.csproj
@@ -18,27 +18,24 @@
 
   <!-- Pull in Shared Source from Azure.Core -->
   <ItemGroup>
-    <Compile Include="$(AzureCoreSharedSources)Argument.cs" Link="Shared\%(RecursiveDir)\%(Filename)%(Extension)" />
-    <Compile Include="$(AzureCoreSharedSources)ArrayBufferWriter.cs" Link="Shared\%(RecursiveDir)\%(Filename)%(Extension)" />
-    <Compile Include="$(AzureCoreSharedSources)AzureKeyCredentialPolicy.cs" Link="Shared\%(RecursiveDir)\%(Filename)%(Extension)" />
-    <Compile Include="$(AzureCoreSharedSources)AzureResourceProviderNamespaceAttribute.cs" Link="Shared\Core\%(RecursiveDir)\%(Filename)%(Extension)" />
-    <Compile Include="$(AzureCoreSharedSources)ClientDiagnostics.cs" Link="Shared\%(RecursiveDir)\%(Filename)%(Extension)" />
-    <Compile Include="$(AzureCoreSharedSources)ContentTypeUtilities.cs" Link="Shared\%(RecursiveDir)\%(Filename)%(Extension)" />
-    <Compile Include="$(AzureCoreSharedSources)DiagnosticScope.cs" Link="Shared\%(RecursiveDir)\%(Filename)%(Extension)" />
-    <Compile Include="$(AzureCoreSharedSources)DiagnosticScopeFactory.cs" Link="Shared\%(RecursiveDir)\%(Filename)%(Extension)" />
-    <Compile Include="$(AzureCoreSharedSources)ForwardsClientCallsAttribute.cs" Link="Shared\Core\%(RecursiveDir)\%(Filename)%(Extension)" />
-    <Compile Include="$(AzureCoreSharedSources)HttpMessageSanitizer.cs" Link="Shared\%(RecursiveDir)\%(Filename)%(Extension)" />
-    <Compile Include="$(AzureCoreSharedSources)TaskExtensions.cs" Link="Shared\%(RecursiveDir)\%(Filename)%(Extension)" />
-    <Compile Include="$(AzureCoreSharedSources)OperationHelpers.cs" Link="Shared\%(RecursiveDir)\%(Filename)%(Extension)" />
-    <Compile Include="$(AzureCoreSharedSources)PageResponseEnumerator.cs" Link="Shared\%(RecursiveDir)\%(Filename)%(Extension)" />
-    <Compile Include="$(AzureCoreSharedSources)SyncAsyncEventHandlerExtensions.cs" Link="Shared\%(RecursiveDir)\%(Filename)%(Extension)" />
+    <Compile Include="$(AzureCoreSharedSources)Argument.cs" LinkBase="Shared" />
+    <Compile Include="$(AzureCoreSharedSources)ArrayBufferWriter.cs" LinkBase="Shared" />
+    <Compile Include="$(AzureCoreSharedSources)AzureKeyCredentialPolicy.cs" LinkBase="Shared" />
+    <Compile Include="$(AzureCoreSharedSources)AzureResourceProviderNamespaceAttribute.cs" LinkBase="Shared\Core" />
+    <Compile Include="$(AzureCoreSharedSources)ClientDiagnostics.cs" LinkBase="Shared" />
+    <Compile Include="$(AzureCoreSharedSources)ContentTypeUtilities.cs" LinkBase="Shared" />
+    <Compile Include="$(AzureCoreSharedSources)DiagnosticScope.cs" LinkBase="Shared" />
+    <Compile Include="$(AzureCoreSharedSources)DiagnosticScopeFactory.cs" LinkBase="Shared" />
+    <Compile Include="$(AzureCoreSharedSources)ForwardsClientCallsAttribute.cs" LinkBase="Shared\Core" />
+    <Compile Include="$(AzureCoreSharedSources)HttpMessageSanitizer.cs" LinkBase="Shared" />
+    <Compile Include="$(AzureCoreSharedSources)TaskExtensions.cs" LinkBase="Shared" />
+    <Compile Include="$(AzureCoreSharedSources)OperationHelpers.cs" LinkBase="Shared" />
+    <Compile Include="$(AzureCoreSharedSources)ObjectNotDisposedException.cs" LinkBase="Shared" />
+    <Compile Include="$(AzureCoreSharedSources)PageResponseEnumerator.cs" LinkBase="Shared" />
+    <Compile Include="$(AzureCoreSharedSources)SyncAsyncEventHandlerExtensions.cs" LinkBase="Shared" />
   </ItemGroup>
 
   <!-- Project and Package references -->
-  <PropertyGroup>
-    <!-- Force a project reference until SyncAsyncEventHandler has shipped -->
-    <UseProjectReferenceToAzureClients>true</UseProjectReferenceToAzureClients>
-  </PropertyGroup>
   <Import Project="$(MSBuildThisFileDirectory)..\..\..\core\Azure.Core\src\Azure.Core.props" />
   <ItemGroup>
     <PackageReference Include="System.Text.Json" />

diff --git a/sdk/search/Azure.Search.Documents/src/Batching/SearchIndexingBufferedSender.cs b/sdk/search/Azure.Search.Documents/src/Batching/SearchIndexingBufferedSender.cs
@@ -183,11 +183,11 @@ internal async Task DisposeAsync(bool async)
                 try
                 {
                     #pragma warning disable CA1065 // Do not raise exceptions in unexpected locations
-                    throw new InvalidOperationException(
+                    throw new ObjectNotDisposedException(
                         $"{nameof(SearchIndexingBufferedSender<T>)} has {_publisher.IndexingActionsCount} unsent indexing actions.");
                     #pragma warning restore CA1065 // Do not raise exceptions in unexpected locations
                 }
-                catch (InvalidOperationException)
+                catch (ObjectNotDisposedException)
                 {
                 }
             }

diff --git a/sdk/search/Azure.Search.Documents/tests/Batching/BatchingTests.cs b/sdk/search/Azure.Search.Documents/tests/Batching/BatchingTests.cs
@@ -803,6 +803,22 @@ public async Task Dispose_UndisposedNoCrash()
                     });
             AssertNoFailures(indexer);
             await indexer.UploadDocumentsAsync(data);
+
+            // To verify the developer experience, debug this test with first
+            // chance exceptions enabled and you'll see an exception raised
+            // from the SearchIndexingBufferedSender finalizer like:
+            // "Azure.Core.ObjectNotDisposedException: 'SearchIndexingBufferedSender has 768 unsent indexing actions.'"
+            if (Debugger.IsAttached)
+            {
+                indexer = null;
+                int maxAttempts = 10;
+                for (int i = 0; i < maxAttempts; i++)
+                {
+                    GC.Collect(GC.MaxGeneration, GCCollectionMode.Forced, blocking: true);
+                    GC.WaitForPendingFinalizers();
+                    await DelayAsync(TimeSpan.FromSeconds(1), TimeSpan.FromSeconds(1));
+                }
+            }
         }
         #endregion