CP Subsystem - Fenced Lock

[mdumandag]

Implementation, tests, documentation and code samples for the Fenced Lock
is added.

Also, the session manager for session aware CP proxies is implemented
fully along with its test suit. The unused parts of the session manager
will be used with the Semaphore proxy.

[mdumandag]

Unrelated test failure, reported on #238

[mdumandag]

verify

[puzpuzpuz]

Leaving some comments. Will continue the review tomorrow.

[puzpuzpuz]

Do we really need this method in Python client? It seems to me that it could be merged with lock_and_get_fence. Or, if we aim to mimic Python library, then it probably makes sense to mimic RLock's acquire/release. The same consideration applies to other methods of this class.

In general, I'd also prefer to expose only blocking API in this class, like it's done in Java client. The concept of reentrant lock is much easier to understand and deal with correctly if it only has a blocking API.

WDYT?

[mdumandag]

Good suggestion. I think it makes sense for all non-java clients to remove these methods. So, I removed lock_and_get_fence, try_lock_and_get_fence and get_fence methods and made lock and try_lock methods return a fencing token.

[puzpuzpuz]

Another stupid question: what a user is supposed to do if they use non-blocking mode of the lock? Namely, when they obtain the lock via lock = client.cp_subsystem.get_lock("lock") and then deal with futures. AFAIU they will have to handle acquire result with continue_with and release the lock later. I may be completely wrong, but doesn't this pattern assume a switch to the reactor thread? If so, the subsequent release will be happening on another thread, which is a problem.

[mdumandag]

That is indeed a very good question. We can make the API blocking as you suggested in the above comment and that would simplify the solution, but it would be really weird if we have just the blocking version for FencedLock, and two versions for other proxies.

Therefore, I am proposing implementing a new type of future. It would be similar to the deferred version of the boost::future from the C++. We will lazily evaluate the callbacks on the thread that calls the result() method on the future objects and make the methods on this proxy to return this new future type.

So, when someone does this

lock = client.cp_subsystem.get_lock("lock")
future = lock.lock()
future.add_done_callback(lambda _: print("callback"))

The print statement will not be executed even if the future resolves.

However, when someone does this

lock = client.cp_subsystem.get_lock("lock")
future = lock.lock()
future.add_done_callback(lambda _: print("callback"))

...

future.result()

The callback would be executed in the thread that calls the result (main thread in most cases) after future resolves.

What do you think about this approach?

[puzpuzpuz]

What are the benefit in this approach when compared with the blocking API? The user will have to make sure to call future.result() on the same thread, which is more or less the same as calling a blocking lock method. Also, I wonder if this approach with lazy future execution may lead to user confusion and possible issues.

[mdumandag]

Nothing comes to my mind apart from the consistent API offered by the client.

It can in fact lead to confusion, and maybe we can implement the deferred future, provide a helper function that will convert normal futures to deferred ones, and return normal futures in the proxy?

So, if someone wants to unlock in a callback, he may do something like this

from hazelcast.future import to_deferred

...

lock = client.cp_subsystem.get_lock("lock")
future = to_deferred(lock.lock())
future.add_done_callback(lambda _: print("callback"))

...

future.result()

[puzpuzpuz]

I'm not totally against this to_deferred approach and I understand the point of having a consistent API. My main concern is confusion that users familiar with RLock may have. If we decide to leave future-based API in FencedLock, we should clearly describe the underlying thread id logic, including some code snippets, in both ref manual and API docs.

[mdumandag]

@puzpuzpuz, after thinking about this problem over the weekend, I have decided to revert the changes done for the deferred idea, as it offers almost no value, besides having a somewhat consistent API.

But, I think it still makes sense to provide a non-blocking API. Maybe it can be useful for use-cases like below

lock = ...
queue = ...

lock.lock().result()

def cb(_):
    queue.offer(3)

lock.unlock().add_done_callback(cb)

Anyways, I have added a note to the docstring mentioning that we advise using this proxy in blocking mode, non-blocking usage may cause a failure because the requests and callbacks may be executed on different threads.

[puzpuzpuz]

If there will be a warning in the ref manual and API doc, it should probably be fine to keep futures around.

Anyways, I have added a note to the docstring mentioning that we advise using this proxy in blocking mode, non-blocking usage may cause a failure because the requests and callbacks may be executed on different threads.

Does it make sense to add some tests for non-blocking mode? Currently we only test the blocking one.

[mdumandag]

Well, it is hard and unreliable to test those. For example, for lock.unlock().add_done_callback(cb), the cb might be executed on the reactor thread if the unlock future is not completed, or on the thread that calls add_done_callback, if the future is already completed. We might make use of some internals such as reactor.add_timer() to execute everything on the reactor thread, but I would rather not.

We do have mock tests for all kinds of failure scenarios, I believe these should be enough

[puzpuzpuz]

Well, it is hard and unreliable to test those.

If it's hard for us to test this code, how the users will feel?

[mdumandag]

I meant testing it that way is hard and unreliable due to the problem I mentioned(It is specific to APIs that rely on thread ids, for others, it is straightforward to test).

Using it is pretty simple

[mdumandag]

Thanks for the review Andrey!