Skip to content

Annotate ObjectPool with nullable #22823

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

Merged
2 commits merged into from
Jun 16, 2020
Merged

Annotate ObjectPool with nullable #22823

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
42963fb
Annotate ObjectPool with nullable
pranavkm Jun 11, 2020
3c7caad
fixup
pranavkm Jun 12, 2020
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 src/ObjectPool/ref/Microsoft.Extensions.ObjectPool.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
<PropertyGroup>
<TargetFrameworks>netstandard2.0;$(DefaultNetCoreTargetFramework)</TargetFrameworks>
<TargetFrameworks Condition="'$(DotNetBuildFromSource)' == 'true'">$(DefaultNetCoreTargetFramework)</TargetFrameworks>
<Nullable>annotations</Nullable>
</PropertyGroup>
<ItemGroup Condition="'$(TargetFramework)' == 'netstandard2.0'">
<Compile Include="Microsoft.Extensions.ObjectPool.netstandard2.0.cs" />
Expand Down
3 changes: 2 additions & 1 deletion src/ObjectPool/ref/Microsoft.Extensions.ObjectPool.netcoreapp.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ public partial class DefaultObjectPool<T> : Microsoft.Extensions.ObjectPool.Obje
{
public DefaultObjectPool(Microsoft.Extensions.ObjectPool.IPooledObjectPolicy<T> policy) { }
public DefaultObjectPool(Microsoft.Extensions.ObjectPool.IPooledObjectPolicy<T> policy, int maximumRetained) { }
[return: System.Diagnostics.CodeAnalysis.NotNullAttribute]
Copy link
Member

@JamesNK JamesNK Jun 12, 2020

Choose a reason for hiding this comment

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

Should NotNull also be on the base class? Would it ever make sense to return null from ObjectPool?

Copy link
Contributor Author

@pranavkm pranavkm Jun 12, 2020

Choose a reason for hiding this comment

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

It's already on the base class too.

Copy link
Member

@JamesNK JamesNK Jun 12, 2020

Choose a reason for hiding this comment

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

I don't see it -

aspnetcore/src/ObjectPool/ref/Microsoft.Extensions.ObjectPool.netcoreapp.cs

Lines 57 to 62 in 42963fb

public abstract partial class ObjectPool<T> where T : class
{
protected ObjectPool() { }
public abstract T Get();
public abstract void Return(T obj);
}

Copy link
Contributor Author

@pranavkm pranavkm Jun 12, 2020

Choose a reason for hiding this comment

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

Oops, I think I meant to put this on the base type.

public override T Get() { throw null; }
public override void Return(T obj) { }
Copy link
Member

@JamesNK JamesNK Jun 12, 2020

Choose a reason for hiding this comment

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

Can null be passed to Return? If T is not null everywhere then the notnull generic constraint could be used.

Copy link
Contributor Author

@pranavkm pranavkm Jun 12, 2020

Choose a reason for hiding this comment

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

T already has a class constraint. The compiler requires that both notnull and class to be the first generic constraint so you're unable to apply both simultaneously. That said, there aren't any null checks here. You could return a null value, but that would likely corrupt the pool

Copy link
Contributor Author

@pranavkm pranavkm Jun 12, 2020

Choose a reason for hiding this comment

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

As I discovered, T : class indicates that T is non-null. This removes the need to specify a NotNullAttribute, and we get the right behavior

}
Expand Down Expand Up @@ -40,7 +41,7 @@ public override void Return(T obj) { }
}
public static partial class ObjectPool
{
public static Microsoft.Extensions.ObjectPool.ObjectPool<T> Create<T>(Microsoft.Extensions.ObjectPool.IPooledObjectPolicy<T> policy = null) where T : class, new() { throw null; }
public static Microsoft.Extensions.ObjectPool.ObjectPool<T> Create<T>(Microsoft.Extensions.ObjectPool.IPooledObjectPolicy<T>? policy = null) where T : class, new() { throw null; }
}
public abstract partial class ObjectPoolProvider
{
Expand Down
2 changes: 1 addition & 1 deletion src/ObjectPool/ref/Microsoft.Extensions.ObjectPool.netstandard2.0.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ public override void Return(T obj) { }
}
public static partial class ObjectPool
{
public static Microsoft.Extensions.ObjectPool.ObjectPool<T> Create<T>(Microsoft.Extensions.ObjectPool.IPooledObjectPolicy<T> policy = null) where T : class, new() { throw null; }
public static Microsoft.Extensions.ObjectPool.ObjectPool<T> Create<T>(Microsoft.Extensions.ObjectPool.IPooledObjectPolicy<T>? policy = null) where T : class, new() { throw null; }
}
public abstract partial class ObjectPoolProvider
{
Expand Down
10 changes: 6 additions & 4 deletions src/ObjectPool/src/DefaultObjectPool.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Runtime.CompilerServices;
using System.Threading;

Expand All @@ -18,10 +19,10 @@ public class DefaultObjectPool<T> : ObjectPool<T> where T : class
private protected readonly ObjectWrapper[] _items;
private protected readonly IPooledObjectPolicy<T> _policy;
private protected readonly bool _isDefaultPolicy;
private protected T _firstItem;
private protected T? _firstItem;

// This class was introduced in 2.1 to avoid the interface call where possible
private protected readonly PooledObjectPolicy<T> _fastPolicy;
private protected readonly PooledObjectPolicy<T>? _fastPolicy;

/// <summary>
/// Creates an instance of <see cref="DefaultObjectPool{T}"/>.
Expand Down Expand Up @@ -54,6 +55,7 @@ bool IsDefaultPolicy()
}
}

[return: NotNull]
public override T Get()
{
var item = _firstItem;
Expand Down Expand Up @@ -97,7 +99,7 @@ public override void Return(T obj)
[DebuggerDisplay("{Element}")]
private protected struct ObjectWrapper
{
public T Element;
public T? Element;
}
}
}
5 changes: 2 additions & 3 deletions src/ObjectPool/src/DisposableObjectPool.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Runtime.CompilerServices;
using System.Threading;

namespace Microsoft.Extensions.ObjectPool
Expand Down Expand Up @@ -82,7 +81,7 @@ public void Dispose()
}
}

private void DisposeItem(T item)
private void DisposeItem(T? item)
{
if (item is IDisposable disposable)
{
Expand Down
5 changes: 2 additions & 3 deletions src/ObjectPool/src/LeakTrackingObjectPool.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
Expand Down Expand Up @@ -31,8 +31,7 @@ public override T Get()

public override void Return(T obj)
{
Tracker tracker;
if (_trackers.TryGetValue(obj, out tracker))
if (_trackers.TryGetValue(obj, out var tracker))
{
_trackers.Remove(obj);
tracker.Dispose();
Expand Down
1 change: 1 addition & 0 deletions src/ObjectPool/src/Microsoft.Extensions.ObjectPool.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
<PackageTags>pooling</PackageTags>
<IsPackable>true</IsPackable>
<IsAspNetCoreApp>true</IsAspNetCoreApp>
<Nullable>enable</Nullable>
</PropertyGroup>

<ItemGroup>
Expand Down
6 changes: 4 additions & 2 deletions src/ObjectPool/src/ObjectPool.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System.Diagnostics.CodeAnalysis;

namespace Microsoft.Extensions.ObjectPool
{
/// <summary>
Expand Down Expand Up @@ -28,7 +30,7 @@ public abstract class ObjectPool<T> where T : class
public static class ObjectPool
{
/// <inheritdoc />
public static ObjectPool<T> Create<T>(IPooledObjectPolicy<T> policy = null) where T : class, new()
public static ObjectPool<T> Create<T>(IPooledObjectPolicy<T>? policy = null) where T : class, new()
{
var provider = new DefaultObjectPoolProvider();
return provider.Create(policy ?? new DefaultPooledObjectPolicy<T>());
Expand Down
1 change: 1 addition & 0 deletions src/ObjectPool/test/Microsoft.Extensions.ObjectPool.Tests.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

<PropertyGroup>
<TargetFrameworks>$(DefaultNetCoreTargetFramework);net472</TargetFrameworks>
<Nullable>enable</Nullable>
</PropertyGroup>

<ItemGroup>
Expand Down
6 changes: 3 additions & 3 deletions src/ObjectPool/test/ThreadingTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System.Threading;
Expand All @@ -8,8 +8,8 @@ namespace Microsoft.Extensions.ObjectPool
{
public class ThreadingTest
{
private CancellationTokenSource _cts;
private DefaultObjectPool<Item> _pool;
private CancellationTokenSource _cts = default!;
private DefaultObjectPool<Item> _pool = default!;
private bool _foundError;

[Fact]
Expand Down