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.

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

Fix disposable extensions bugs and add relevant tests. #157

Merged
merged 2 commits into from
Mar 6, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CodeJam.Main.Tests/CodeJam.Main.Tests.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
<TargetFrameworks Condition=" '$(NetFrameworkTests)' == 'true' ">net48;net472;net471;net47;net461;net45;net40;net35</TargetFrameworks>
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
<DisableImplicitNuGetFallbackFolder>true</DisableImplicitNuGetFallbackFolder>
<LangVersion>12.0</LangVersion>
</PropertyGroup>

<!-- #region Targeting -->
Expand Down
93 changes: 93 additions & 0 deletions CodeJam.Main.Tests/DisposableExtensionsTests.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
using System;
using System.Diagnostics.CodeAnalysis;
using System.Linq;
using System.Threading;
using System.Threading.Tasks;

using NUnit.Framework;

namespace CodeJam;

[TestFixture(Category = "Disposable")]
[SuppressMessage("ReSharper", "HeapView.CanAvoidClosure")]
public static class DisposableExtensionsTests
{
[Test]
public static void DisposeAllMustReleaseAllObjects()
{
const int expectedDisposeCount = 10;

int actualDisposeCount = 0;

var objectsForDispose = Enumerable
.Range(0, expectedDisposeCount)
.Select(x => Disposable.Create(() => ++actualDisposeCount));

objectsForDispose.DisposeAll();

Assert.AreEqual(expectedDisposeCount, actualDisposeCount);
}

[Test]
public static void DisposeAllMustCollectAllExceptions()
{
const int expectedExceptionCount = 7;

var objectsWithException = Enumerable
.Range(0, expectedExceptionCount)
.Select(x => Disposable.Create(() => throw new Exception()));

const int expectedSuccessCount = 3;

var objectsWithoutException = Enumerable
.Range(0, expectedSuccessCount)
.Select(x => Disposable.Create(() => { }));

var objectsForDispose = objectsWithException.Concat(objectsWithoutException).ToArray();

int actualExceptionCount = -1;

try
{
objectsForDispose.DisposeAll();
}
catch (AggregateException ex)
{
actualExceptionCount = ex.InnerExceptions.Count;
}

Assert.AreEqual(expectedExceptionCount, actualExceptionCount);
}

#if NETSTANDARD21_OR_GREATER || NETCOREAPP30_OR_GREATER
[Test]
public static async Task DisposeAsyncMustCallDiposeOnce()
{
const int expectedDisposeCount = 1;

int actualDisposeCount = 0;

var objectForDispose = Disposable.Create(() => ++actualDisposeCount);

await objectForDispose.DisposeAsync();

Assert.AreEqual(expectedDisposeCount, actualDisposeCount);
}

[Test]
public static void DisposeAsyncMustNotBlockThread()
{
var disposeDuration = new TimeSpan(0, 0, 1);

var longDisposableObject = Disposable.Create(() => Thread.Sleep(disposeDuration));

var startTime = DateTime.Now;

var task = longDisposableObject.DisposeAsync();

var callDuration = DateTime.Now - startTime;

Assert.Less(callDuration, disposeDuration);
}
#endif
}
14 changes: 6 additions & 8 deletions CodeJam.Main/DisposableExtensions.cs
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
using System;
using System.Collections.Generic;
#if NETSTANDARD21_OR_GREATER || NETCOREAPP30_OR_GREATER
using System.Threading.Tasks;
#endif

using CodeJam.Internal;

using JetBrains.Annotations;
Expand Down Expand Up @@ -30,7 +29,8 @@ public static void DisposeAll([InstantHandle] this IEnumerable<IDisposable> disp
}
catch (Exception ex)
{
exceptions = new List<Exception> { ex };
if (exceptions != null) exceptions.Add(ex);
andrewvk marked this conversation as resolved.
Show resolved Hide resolved
else exceptions = new List<Exception> { ex };
}
}

Expand Down Expand Up @@ -66,14 +66,12 @@ public static void DisposeAll(
/// Calls DisposeAsync if <paramref name="disposable"/> implements <see cref="IAsyncDisposable"/>, otherwise
/// calls <see cref="IDisposable.Dispose"/>
/// </summary>
public static ValueTask DisposeAsync(this IDisposable disposable)
public static async ValueTask DisposeAsync(this IDisposable disposable)
{
Code.NotNull(disposable, nameof(disposable));
if (disposable is IAsyncDisposable asyncDisposable)
return asyncDisposable.DisposeAsync();

disposable.Dispose();
return new ValueTask();
await asyncDisposable.DisposeAsync();
await new ValueTask(Task.Run(() => disposable.Dispose()));
Copy link
Member

Choose a reason for hiding this comment

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

What is the reason for making ValueTask here?
Isn't await is enough ?

P.S.
Don't we need ConfigureAwaitable(false) as a library here ?

Copy link
Contributor

Choose a reason for hiding this comment

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

As for the ValueTask - these two implementations have almost tame behavior so both look good for me.

ConfigureAwaitable is not used in the lib anyways. We may add the calls but I'm not sure if it should be done,

Copy link
Member

Choose a reason for hiding this comment

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

  1. +1
  2. At the moment, a lot of CJ code does not contain CfgAwait(false). If we move away from this, we should fix the whole library code.

}
#endif
}
Expand Down