ZooKeeperNetEx.Recipes WriteLock allows multiple clients to obtain lock to the same dir simultaneously

[nj4x]

Pls find issue reproducing gist here https://gist.github.com/nj4x/a13c7b8c4362ded9cedc

[shayhatsor]

this one is basically "by design", not my design... but the java one. please refer to the test here for usage example. Currently, AFAIK WriteLock works properly, but the interface is far from intuitive. mostly because when I converted the code, I tried to keep the same flow. In order for it to make sense as async, i'll probably need to re-write it from scratch. Till then, I'll look at the java implementation again and try to revise the c# implementation in order to make the interface more intuitive.

[shayhatsor]

the behavior of the Lock method is documented:

/// <summary>
/// Attempts to acquire the exclusive write lock returning whether or not it was
/// acquired. Note that the exclusive lock may be acquired some time later after
/// this method has been invoked due to the current lock owner going away.
/// </summary>
public async Task<bool> Lock()
{
    using(await lockable.LockAsync().ConfigureAwait(false))
    {
        if (Closed)
        {
            return false;
        }
        await ensurePathExists(dir).ConfigureAwait(false);

        return await retryOperation(zop).ConfigureAwait(false);
    }
}

I am aware it's far from intuitive, i'll see how to improve.

[nj4x]

Does that mean that two different apps can obtain the lock to the same resource simultaneously ?

On Wed, 27 Jan 2016 at 17:26 Shay Hazor notifications@github.com wrote:

the behavior of the Lock method is documented:

///

/// Attempts to acquire the exclusive write lock returning whether or not it was/// acquired. Note that the exclusive lock may be acquired some time later after/// this method has been invoked due to the current lock owner going away.///

public async Task Lock() { using(await lockable.LockAsync().ConfigureAwait(false)) { if (Closed) { return false; } await ensurePathExists(dir).ConfigureAwait(false);

    return await retryOperation(zop).ConfigureAwait(false);
}

}

I am aware it's far from intuitive, i'll see how to improve.

— Reply to this email directly or view it on GitHub https://github.com/shayhatsor/zookeeper/issues/4#issuecomment-175688153.

[shayhatsor]

no, it means that in order to get the expected behavior, the code that needs the lock should implement LockListener :

/// <summary>
/// This class has two methods which are call
/// back methods when a lock is acquired and 
/// when the lock is released.
/// 
/// </summary>
public interface LockListener
{
    /// <summary>
    /// call back called when the lock 
    /// is acquired
    /// </summary>
    void lockAcquired();

    /// <summary>
    /// call back called when the lock is 
    /// released.
    /// </summary>
    void lockReleased();
}

it must be set either in the WriteLock constructor or via the setLockListener(callback) method before calling Lock()

[nj4x]

Implemented LockListener, result is the same -

WatchedEvent state:SyncConnected type:None path: WatchedEvent state:SyncConnected type:None path: Lock acquired by APP-2 Lock acquired by APP-1 App1 lock acquired: True App2 lock acquired: True ls /test x-95283211971985411-0000000049 x-95283211971985412-0000000048

The problem is you are using session id in a prefix and order of sequence is broken, but according to https://zookeeper.apache.org/doc/r3.1.2/recipes.html there should be static prefix with a sequence:

Locks

Fully distributed locks that are globally synchronous, meaning at any snapshot in time no two clients think they hold the same lock. These can be implemented using ZooKeeeper. As with priority queues, first define a lock node.

Clients wishing to obtain a lock do the following:

1.

Call create( ) with a pathname of "locknode/lock-" and the sequence and ephemeral flags set. 2.

Call getChildren( ) on the lock node without setting the watch flag (this is important to avoid the herd effect). 3.

If the pathname created in step 1 has the lowest sequence number suffix, the client has the lock and the client exits the protocol. 4.

The client calls exists( ) with the watch flag set on the path in the lock directory with the next lowest sequence number. 5.

if exists( ) returns false, go to step 2. Otherwise, wait for a notification for the pathname from the previous step before going to step 2.

On Wed, 27 Jan 2016 at 18:18 Shay Hazor notifications@github.com wrote:

no, it means that in order to get the expected behavior, the code that needs the lock should implement LockListener :

///

/// This class has two methods which are call/// back methods when a lock is acquired and /// when the lock is released./// ///

public interface LockListener { ///

/// call back called when the lock
/// is acquired
/// </summary>

void lockAcquired();

/// <summary>
/// call back called when the lock is
/// released.
/// </summary>
void lockReleased();

}

it must be set either in the WriteLock constructor or via the setLockListener(callback) method before calling Lock()

— Reply to this email directly or view it on GitHub https://github.com/shayhatsor/zookeeper/issues/4#issuecomment-175719623.

[nj4x]

I've checked your test, it also fails if you try to change the order of ZK clients acquiring the lock:

        for (int i = count - 1; i >= 0; i--)
        {
            ZooKeeper keeper = createClient();
            WriteLock leader = new WriteLock(keeper, "/test", null);
            leader.setLockListener(new LockCallback(this));
            nodes[i] = leader;
        }

        for (int i = 0; i < count; i++)
        {
            nodes[i].Lock().GetAwaiter().GetResult();
        }

[shayhatsor]

an example of correct usage:

    using System;
    using System.Threading;
    using System.Threading.Tasks;
    using org.apache.zookeeper;
    using org.apache.zookeeper.recipes.@lock;

namespace ZooKeeperNetExTest
{
    internal class Program
    {
        private static void Main(string[] args)
        {
            Test().Wait();
        }

        private static async Task Test()
        {
            var zk1 = new ZooKeeper("localhost:2181", 5000, new ZkWatcher()); // app1
            var zk2 = new ZooKeeper("localhost:2181", 5000, new ZkWatcher()); // app2
            Action zk1LockedAction = () => LockedAction("App1 lock acquired");
            Action zk2LockedAction = () => LockedAction("App2 lock acquired");
            Action zk1ReleasedAction = () => LockedAction("App1 lock released");
            Action zk2ReleasedAction = () => LockedAction("App2 lock released");
            var zk1Lock = new WriteLock(zk1, "/test", null, new LockListenerExample(zk1LockedAction, zk1ReleasedAction));
            var zk2Lock = new WriteLock(zk2, "/test", null, new LockListenerExample(zk2LockedAction, zk2ReleasedAction));
            Console.WriteLine("starting zk1Lock lock attempt");
            await zk1Lock.Lock();
            await WriteChildren(zk1);
            Console.WriteLine("starting zk2Lock lock attempt");
            await zk2Lock.Lock();
            await WriteChildren(zk1);
            Console.WriteLine("unlocking zk1Lock");
            await zk1Lock.unlock();
            await WriteChildren(zk1);
            Console.WriteLine("unlocking zk2Lock");
            await zk2Lock.unlock();
            await WriteChildren(zk1);
            Console.ReadLine();
        }

        private static async Task WriteChildren(ZooKeeper zk)
        {
            await Task.Delay(100);
            var childrenResult = await zk.getChildrenAsync("/test");
            Console.WriteLine("ls /test");
            foreach (var @lock in childrenResult.Children)
            {
                Console.WriteLine("" + @lock);
            }
        }

        private class LockListenerExample : LockListener
        {
            private readonly Action lockAquiredAction;
            private readonly Action lockreleasedAction;

            public LockListenerExample(Action lockAquiredAction,Action lockreleasedAction)
            {
                this.lockAquiredAction = lockAquiredAction;
                this.lockreleasedAction = lockreleasedAction;
            }

            public void lockAcquired()
            {
                Thread.Sleep(1000);
                lockAquiredAction();
            }

            public void lockReleased()
            {
                Thread.Sleep(1000);
                lockreleasedAction();
            }
        }

        private static void LockedAction(string message)
        {
            Console.WriteLine(message);
        }

        private class ZkWatcher : Watcher
        {
            public override Task process(WatchedEvent @event)
            {
                Console.WriteLine(@event.ToString());
                return Task.Delay(0);
            }
        }
    }
}

[nj4x]

I'm sorry, but this usage is not correct. I mean you can not predict in which order applications will aquire the Lock, so if you change the order, both clients will get the lock acquired:

        Console.WriteLine("starting zk2Lock lock attempt");
        await zk2Lock.Lock();
        await WriteChildren(zk1);
    Console.WriteLine("starting zk1Lock lock attempt");
        await zk1Lock.Lock();
        await WriteChildren(zk1);

[nj4x]

starting zk2Lock lock attempt WatchedEvent state:SyncConnected type:None path: WatchedEvent state:SyncConnected type:None path: App2 lock acquired ls /test x-95283823188180993-0000000050 starting zk1Lock lock attempt App1 lock acquired ls /test x-95283823188180992-0000000051 x-95283823188180993-0000000050

[shayhatsor]

I see your point. that's a bug. Note that this is a very rough guideline to implementing the recipes. I've tried to follow the official implementation which is here. By the way, there's absolutely a bug in the implementation of LockListener - its methods don't return Task. Thanks for pointing this out, I'll see what I've missed from the java implementation. There could be a possibility this bug also exists there, but probably not...

[nj4x]

Thanks, Shay!

[shayhatsor]

I believe I've fixed the issue. Please have a look at the latest nuget package 3.4.7.4 Thanks again for your input.

[nj4x]

Issue is fixed.

P.S. I've found missing ConfigureAwait(false) at WriteLock in lockReleased()/lockAcquired() usage though.

[shayhatsor]

will fix, thanks!

[shayhatsor]

fixed in 3.4.7.5