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

4.x: Make RefCountDisposable lock-free #574

Merged
merged 4 commits into from
Jun 6, 2018
Merged

4.x: Make RefCountDisposable lock-free #574

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
93da8e4
4.x: Lock-free RefCountDisposable
akarnokd Jun 5, 2018
6b5b72b
Use the _count field for disposable indication
akarnokd Jun 5, 2018
5679549
Restore the diagnostic check
akarnokd Jun 5, 2018
d508a7f
Throw explicitly on overflow
akarnokd Jun 6, 2018
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
109 changes: 72 additions & 37 deletions Rx.NET/Source/src/System.Reactive/Disposables/RefCountDisposable.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,12 @@ namespace System.Reactive.Disposables
public sealed class RefCountDisposable : ICancelable
{
private readonly bool _throwWhenDisposed;
private readonly object _gate = new object();
private IDisposable _disposable;
private bool _isPrimaryDisposed;
/// <summary>
/// Holds the number of active child disposables and the
/// indicator bit (31) if the main _disposable has been marked
/// for disposition.
/// </summary>
private int _count;

/// <summary>
Expand All @@ -38,15 +41,14 @@ public RefCountDisposable(IDisposable disposable, bool throwWhenDisposed)
throw new ArgumentNullException(nameof(disposable));

_disposable = disposable;
_isPrimaryDisposed = false;
_count = 0;
_throwWhenDisposed = throwWhenDisposed;
}

/// <summary>
/// Gets a value that indicates whether the object is disposed.
/// </summary>
public bool IsDisposed => _disposable == null;
public bool IsDisposed => Volatile.Read(ref _count) == int.MinValue;

/// <summary>
/// Returns a dependent disposable that when disposed decreases the refcount on the underlying disposable.
Expand All @@ -56,20 +58,31 @@ public RefCountDisposable(IDisposable disposable, bool throwWhenDisposed)
[System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Design", "CA1024:UsePropertiesWhereAppropriate", Justification = "Backward compat + non-trivial work for a property getter.")]
public IDisposable GetDisposable()
{
lock (_gate)
// the current state
var cnt = Volatile.Read(ref _count);

for (; ; )
{
if (_disposable == null)
// If bit 31 is set and the active count is zero, don't create an inner
if (cnt == int.MinValue)
{
if (_throwWhenDisposed)
throw new ObjectDisposedException("RefCountDisposable");

return Disposable.Empty;
}
else

// Should not overflow the bits 0..30
System.Diagnostics.Debug.Assert((cnt & 0x7FFFFFFF) < int.MaxValue);

// Increment the active count by one, works because the increment
// won't affect bit 31
var u = Interlocked.CompareExchange(ref _count, cnt + 1, cnt);
Copy link
Collaborator

@danielcweber danielcweber Jun 6, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

The behaviour is excatly as in the locked version before, after int.MaxValue it will wrap and lead to strange behaviour (actually, the new version will act as if disposed). We should take the opportunity and not just Debug.Assert but throw something.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Shouldn't the debug assert throw always on failure? Besides, I'm not sure we can support 2 billion child subscribers anyway.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Debug.Assert will not be compiled into Release builds. 2 billion child subscribers are possible, tested this morning.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We could expand to long, allowing far more child disposables.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't really think anybody will hit that number, let's for now just make sure it throws instead of wrapping.

if (u == cnt)
{
_count++;
return new InnerDisposable(this);
}
cnt = u;
}
}

Expand All @@ -78,50 +91,72 @@ public IDisposable GetDisposable()
/// </summary>
public void Dispose()
{
var disposable = default(IDisposable);
lock (_gate)
var cnt = Volatile.Read(ref _count);

for (; ; )
{
if (_disposable != null)

// already marked as disposed via bit 31?
if ((cnt & 0x80000000) != 0)
{
if (!_isPrimaryDisposed)
{
_isPrimaryDisposed = true;
// yes, nothing to do
break;
}

// how many active disposables are there?
int active = cnt & 0x7FFFFFFF;

if (_count == 0)
{
disposable = _disposable;
_disposable = null;
}
// keep the active count but set the dispose marker of bit 31
var u = int.MinValue | active;

var b = Interlocked.CompareExchange(ref _count, u, cnt);

if (b == cnt) {
// if there were 0 active disposables, there can't be any more after
// the CAS so we can dispose the underlying disposable
if (active == 0)
{
_disposable?.Dispose();
_disposable = null;
}
break;
}
cnt = b;
}

disposable?.Dispose();
}

private void Release()
{
var disposable = default(IDisposable);
lock (_gate)
{
if (_disposable != null)
{
_count--;

System.Diagnostics.Debug.Assert(_count >= 0);
var cnt = Volatile.Read(ref _count);

if (_isPrimaryDisposed)
for (; ; )
{
// extract the main disposed state (bit 31)
var main = (int)(cnt & 0x80000000);
// get the active count
var active = cnt & 0x7FFFFFFF;

// keep the main disposed state but decrement the counter
// in theory, active should be always > 0 at this point,
// guaranteed by the InnerDisposable.Dispose's Exchange operation.
System.Diagnostics.Debug.Assert(active > 0);
var u = main | (active - 1);

var b = Interlocked.CompareExchange(ref _count, u, cnt);

if (b == cnt) {
// if after the CAS there was zero active disposables and
// the main has been also marked for disposing,
// it is safe to dispose the underlying disposable
if (u == int.MinValue)
{
if (_count == 0)
{
disposable = _disposable;
_disposable = null;
}
_disposable?.Dispose();
_disposable = null;
}
break;
}
cnt = b;
}

disposable?.Dispose();
}

private sealed class InnerDisposable : IDisposable
Expand Down