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

4.x: Improve the logic of AsyncLock #504

Merged
merged 2 commits into from
May 26, 2018
Merged
Changes from 1 commit
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
88 changes: 55 additions & 33 deletions Rx.NET/Source/src/System.Reactive/Concurrency/AsyncLock.cs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ namespace System.Reactive.Concurrency
/// </summary>
public sealed class AsyncLock : IDisposable
{
private readonly Queue<Action> queue = new Queue<Action>();
private Queue<Action> queue;
private bool isAcquired = false;
private bool hasFaulted = false;

Expand All @@ -27,50 +27,72 @@ public void Wait(Action action)
if (action == null)
throw new ArgumentNullException(nameof(action));

var isOwner = false;
lock (queue)
// allow one thread to update the state
lock (this)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Never ever lock on "this"! Code using the AsyncLock in a private field might use it as a lock as well and we're gonna run into some serious trouble.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's why I noted it in the PR text. Adding a guard = new object() resolves this with the obvious added cost of allocation. If there was an internal version of this lock and the discipline to not lock on it by Rx code, that would be better. I'll update the PR to use a guard.

{
if (!hasFaulted)
// if a previous action crashed, ignore any future actions
if (hasFaulted)
{
queue.Enqueue(action);
isOwner = !isAcquired;
isAcquired = true;
return;
}

// if the "lock" is busy, queue up the extra work
// otherwise there is no need to queue up "action"
if (isAcquired)
{
// create the queue if necessary
var q = queue;
if (q == null)
{
q = new Queue<Action>();
queue = q;
}
// enqueue the work
q.Enqueue(action);
return;
}

// indicate there is processing going on
isAcquired = true;
}

if (isOwner)
// if we get here, execute the "action" first

for (; ; )
{
while (true)
try
{
var work = default(Action);
lock (queue)
action();
}
catch
{
// the execution failed, terminate this AsyncLock
lock (this)
{
if (queue.Count > 0)
{
work = queue.Dequeue();
}
else
{
isAcquired = false;
break;
}
// throw away the queue
queue = null;
// report fault
hasFaulted = true;
}
throw;
}

try
// execution succeeded, let's see if more work has to be done
lock (this)
{
var q = queue;
Copy link
Collaborator

Choose a reason for hiding this comment

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

We're in a lock, why the local variable?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A habit from lock-free programming: touch fields as less as possible around atomic accesses.

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 think there's much to gain, it just makes the code less readable.

// either there is no queue yet or we run out of work
if (q == null || q.Count == 0)
{
work();
// release the lock
isAcquired = false;
return;
}
catch
{
lock (queue)
{
queue.Clear();
hasFaulted = true;
}

throw;
}
// get the next work action
action = q.Dequeue();
}
// loop back and execute the action
}
}

Expand All @@ -79,9 +101,9 @@ public void Wait(Action action)
/// </summary>
public void Dispose()
{
lock (queue)
lock (this)
{
queue.Clear();
queue = null;
hasFaulted = true;
}
}
Expand Down