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

Deprecate Lock and ModuleScope's public type cache API #391

Merged
merged 11 commits into from
Jun 24, 2018
17 changes: 17 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,22 @@
# Castle Core Changelog

## Unreleased

Deprecations:
- The API surrounding `Lock` has been deprecated. This consists of the members listed below. Consider using the Base Class Library's `System.Threading.ReaderWriterLockSlim` instead. (@stakx, #391)
- `Castle.Core.Internal.Lock` (class)
- `Castle.Core.Internal.ILockHolder` (interface)
- `Castle.Core.Internal.IUpgradeableLockHolder` (interface)
- The proxy type cache in `ModuleScope` should no longer be accessed directly. For this reason, the members listed below have been deprecated. (@stakx, #391)
- `Castle.DynamicProxy.ModuleScope.Lock` (property)
- `Castle.DynamicProxy.ModuleScope.GetFromCache` (method)
- `Castle.DynamicProxy.ModuleScope.RegisterInCache` (method)
- `Castle.DynamicProxy.Generators.BaseProxyGenerator.AddToCache` (method)
- `Castle.DynamicProxy.Generators.BaseProxyGenerator.GetFromCache` (method)
- `Castle.DynamicProxy.Generators.CacheKey` (class)
- `Castle.DynamicProxy.Serialization.CacheMappingsAttribute.ApplyTo` (method)
- `Castle.DynamicProxy.Serialization.CacheMappingsAttribute.GetDeserializedMappings` (method)

## 4.3.1 (2018-06-21)

Enhancements:
Expand Down
1 change: 1 addition & 0 deletions buildscripts/common.props
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

<PropertyGroup>
<NoWarn>$(NoWarn);CS1591;CS3014;CS3003;CS3001;CS3021</NoWarn>
<NoWarn>$(NoWarn);CS0612;CS0618</NoWarn> <!-- TODO: Remove this line once `[Obsolete]` members have been dealt with. -->
Copy link
Member Author

Choose a reason for hiding this comment

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

I am not a fan of disabling [Obsolete]-related warnings CS0612 and CS0618 solution-wide, but this still seems better than sprinkling #pragma warning disables and #pragma warning restores across the whole code base. [assembly: SuppressMessage] would have been nice, but I couldn't get it to work with these warnings.

We should remove this again right before releasing Castle Core 5.0.0.

<RepositoryType>git</RepositoryType>
<RepositoryUrl>https://github.com/castleproject/Core</RepositoryUrl>
<BuildVersion>0.0.0</BuildVersion>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,13 @@

namespace Castle.Core.Internal.Tests
{
using System;
using System.Threading;

using NUnit.Framework;

[TestFixture]
[Obsolete]
public class SlimReadWriteLockTestCase
{
private SlimReadWriteLock @lock;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,8 @@ namespace Castle.Components.DictionaryAdapter
/// </summary>
public class DictionaryAdapterFactory : IDictionaryAdapterFactory
{
private readonly Dictionary<Type, DictionaryAdapterMeta> interfaceToMeta = new Dictionary<Type, DictionaryAdapterMeta>();

private readonly Lock interfaceToMetaLock = Lock.Create();
private readonly SynchronizedDictionary<Type, DictionaryAdapterMeta> interfaceToMeta =
new SynchronizedDictionary<Type, DictionaryAdapterMeta>();

#region IDictionaryAdapterFactory

Expand Down Expand Up @@ -130,35 +129,21 @@ private DictionaryAdapterMeta InternalGetAdapterMeta(Type type,
if (type.GetTypeInfo().IsInterface == false)
throw new ArgumentException("Only interfaces can be adapted to a dictionary", "type");

DictionaryAdapterMeta meta;

using (interfaceToMetaLock.ForReading())
{
if (interfaceToMeta.TryGetValue(type, out meta))
return meta;
}

using (var heldLock = interfaceToMetaLock.ForReadingUpgradeable())
return interfaceToMeta.GetOrAdd(type, t =>
{
if (interfaceToMeta.TryGetValue(type, out meta))
return meta;

using (heldLock.Upgrade())
if (descriptor == null && other != null)
{
if (descriptor == null && other != null)
descriptor = other.CreateDescriptor();
descriptor = other.CreateDescriptor();
}

#if FEATURE_LEGACY_REFLECTION_API
var appDomain = Thread.GetDomain();
var typeBuilder = CreateTypeBuilder(type, appDomain);
var appDomain = Thread.GetDomain();
var typeBuilder = CreateTypeBuilder(type, appDomain);
#else
var typeBuilder = CreateTypeBuilder(type);
var typeBuilder = CreateTypeBuilder(type);
#endif
meta = CreateAdapterMeta(type, typeBuilder, descriptor);
interfaceToMeta.Add(type, meta);
return meta;
}
}
return CreateAdapterMeta(type, typeBuilder, descriptor);
});
}

private object InternalGetAdapter(Type type, IDictionary dictionary, PropertyDescriptor descriptor)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,11 @@ namespace Castle.Components.DictionaryAdapter.Xml
using System;
using System.Collections.Generic;
using System.Threading;
using Castle.Core.Internal;

public class SingletonDispenser<TKey, TItem>
Copy link
Contributor

@TimLovellSmith TimLovellSmith Jun 21, 2018

Choose a reason for hiding this comment

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

If SingletonDispenser is an internal utility why is it public? Should it become [Obsolete] too?

Copy link

Choose a reason for hiding this comment

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

Or it should be internal, like as in the csharp modifier we all know and love :)

Copy link
Member

Choose a reason for hiding this comment

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

I'd keep this pull request focused on deprecating Lock.

where TItem : class
{
private readonly Lock locker;
private readonly ReaderWriterLockSlim locker;
private readonly Dictionary<TKey, object> items;
private readonly Func<TKey, TItem> factory;

Expand All @@ -32,7 +31,7 @@ public SingletonDispenser(Func<TKey, TItem> factory)
if (factory == null)
throw Error.ArgumentNull("factory");

this.locker = new SlimReadWriteLock();
this.locker = new ReaderWriterLockSlim(LockRecursionPolicy.NoRecursion);
this.items = new Dictionary<TKey, object>();
this.factory = factory;
}
Expand All @@ -53,22 +52,44 @@ private TItem GetOrCreate(TKey key)

private bool TryGetExistingItem(TKey key, out object item)
{
using (locker.ForReading())
locker.EnterReadLock();
try
{
if (items.TryGetValue(key, out item))
{
return true;
}
}
finally
{
locker.ExitReadLock();
}

using (var hold = locker.ForReadingUpgradeable())
locker.EnterUpgradeableReadLock();
try
{
if (items.TryGetValue(key, out item))
{
return true;

using (hold.Upgrade())
items[key] = item = new ManualResetEvent(false);
}
else
{
locker.EnterWriteLock();
try
{
items[key] = item = new ManualResetEvent(false);
return false;
}
finally
{
locker.ExitWriteLock();
}
}
}
finally
{
locker.ExitUpgradeableReadLock();
}

return false;
}

private TItem WaitForCreate(TKey key, object item)
Expand All @@ -77,8 +98,15 @@ private TItem WaitForCreate(TKey key, object item)

handle.WaitOne();

using (locker.ForReading())
return (TItem) items[key];
locker.EnterReadLock();
try
{
return (TItem)items[key];
}
finally
{
locker.ExitReadLock();
}
}

private TItem Create(TKey key, object item)
Expand All @@ -87,8 +115,15 @@ private TItem Create(TKey key, object item)

var result = factory(key);

using (locker.ForWriting())
locker.EnterWriteLock();
try
{
items[key] = result;
}
finally
{
locker.ExitWriteLock();
}

handle.Set();
return result;
Expand Down
3 changes: 3 additions & 0 deletions src/Castle.Core/Core/Internal/ILockHolder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,10 @@
namespace Castle.Core.Internal
{
using System;
using System.ComponentModel;

[Obsolete("Consider using `System.Threading.ReaderWriterLockSlim` instead of `Lock` and related types.")] // TODO: Remove this type.
[EditorBrowsable(EditorBrowsableState.Never)]
Copy link
Member Author

Choose a reason for hiding this comment

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

I've put // TODO comments next to the most [Obsolete] attributes. They contain instructions how to deal with each obsolete member, so that the clean-up will be as simple as possible.

public interface ILockHolder:IDisposable
{
bool LockAcquired { get; }
Expand Down
5 changes: 5 additions & 0 deletions src/Castle.Core/Core/Internal/IUpgradeableLockHolder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,11 @@

namespace Castle.Core.Internal
{
using System;
using System.ComponentModel;

[Obsolete("Consider using `System.Threading.ReaderWriterLockSlim` instead of `Lock` and related types.")] // TODO: Remove this type.
[EditorBrowsable(EditorBrowsableState.Never)]
public interface IUpgradeableLockHolder : ILockHolder
{
ILockHolder Upgrade();
Expand Down
11 changes: 11 additions & 0 deletions src/Castle.Core/Core/Internal/Lock.cs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,12 @@

namespace Castle.Core.Internal
{
using System;
using System.ComponentModel;
using System.Threading;

[Obsolete("Consider using `System.Threading.ReaderWriterLockSlim` instead of `Lock` and related types.")] // TODO: Remove this type.
[EditorBrowsable(EditorBrowsableState.Never)]
public abstract class Lock
{
public abstract IUpgradeableLockHolder ForReadingUpgradeable();
Expand All @@ -32,5 +38,10 @@ public static Lock Create()
{
return new SlimReadWriteLock();
}

internal static Lock CreateFor(ReaderWriterLockSlim underlyingLock)
{
return new SlimReadWriteLock(underlyingLock);
}
}
}
5 changes: 5 additions & 0 deletions src/Castle.Core/Core/Internal/NoOpLock.cs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,11 @@

namespace Castle.Core.Internal
{
using System;
using System.ComponentModel;

[Obsolete("Consider using `System.Threading.ReaderWriterLockSlim` instead of `Lock` and related types.")] // TODO: Remove this type.
[EditorBrowsable(EditorBrowsableState.Never)]
internal class NoOpLock : ILockHolder
{
public static readonly ILockHolder Lock = new NoOpLock();
Expand Down
5 changes: 5 additions & 0 deletions src/Castle.Core/Core/Internal/NoOpUpgradeableLock.cs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,11 @@

namespace Castle.Core.Internal
{
using System;
using System.ComponentModel;

[Obsolete("Consider using `System.Threading.ReaderWriterLockSlim` instead of `Lock` and related types.")] // TODO: Remove this type.
[EditorBrowsable(EditorBrowsableState.Never)]
internal class NoOpUpgradeableLock : IUpgradeableLockHolder
{
public static readonly IUpgradeableLockHolder Lock = new NoOpUpgradeableLock();
Expand Down
4 changes: 4 additions & 0 deletions src/Castle.Core/Core/Internal/SlimReadLockHolder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,12 @@

namespace Castle.Core.Internal
{
using System;
using System.ComponentModel;
using System.Threading;

[Obsolete("Consider using `System.Threading.ReaderWriterLockSlim` instead of `Lock` and related types.")] // TODO: Remove this type.
[EditorBrowsable(EditorBrowsableState.Never)]
internal class SlimReadLockHolder : ILockHolder
{
private readonly ReaderWriterLockSlim locker;
Expand Down
16 changes: 15 additions & 1 deletion src/Castle.Core/Core/Internal/SlimReadWriteLock.cs
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,25 @@

namespace Castle.Core.Internal
{
using System;
using System.ComponentModel;
using System.Threading;

[Obsolete("Consider using `System.Threading.ReaderWriterLockSlim` instead of `Lock` and related types.")] // TODO: Remove this type.
[EditorBrowsable(EditorBrowsableState.Never)]
internal class SlimReadWriteLock : Lock
{
private readonly ReaderWriterLockSlim locker = new ReaderWriterLockSlim(LockRecursionPolicy.NoRecursion);
private readonly ReaderWriterLockSlim locker;

public SlimReadWriteLock()
{
this.locker = new ReaderWriterLockSlim(LockRecursionPolicy.NoRecursion);
}

internal SlimReadWriteLock(ReaderWriterLockSlim underlyingLock)
{
this.locker = underlyingLock;
}

public override IUpgradeableLockHolder ForReadingUpgradeable()
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,12 @@

namespace Castle.Core.Internal
{
using System;
using System.ComponentModel;
using System.Threading;

[Obsolete("Consider using `System.Threading.ReaderWriterLockSlim` instead of `Lock` and related types.")] // TODO: Remove this type.
[EditorBrowsable(EditorBrowsableState.Never)]
internal class SlimUpgradeableReadLockHolder : IUpgradeableLockHolder
{
private readonly ReaderWriterLockSlim locker;
Expand Down
4 changes: 4 additions & 0 deletions src/Castle.Core/Core/Internal/SlimWriteLockHolder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,12 @@

namespace Castle.Core.Internal
{
using System;
using System.ComponentModel;
using System.Threading;

[Obsolete("Consider using `System.Threading.ReaderWriterLockSlim` instead of `Lock` and related types.")] // TODO: Remove this type.
[EditorBrowsable(EditorBrowsableState.Never)]
internal class SlimWriteLockHolder : ILockHolder
{
private readonly ReaderWriterLockSlim locker;
Expand Down
Loading