Re: Modify ZooKeeper Java client to hold weak references to Watcher objects

[Patrick Hunt <ph...@apache.org>]

Would "WeakWatcher", a proxy watcher that holds a weak reference to the 
proxied watcher, not also solve this with minimal overhead? You could do 
this today.

> 1/ Add a useWeakReferences parameter to new constructor (sets default
> behaviour)
> 2/ Add alternative methods, which take useWeakWatcherRef boolean.

I would prefer option 2 over 1, since I could see some users wishing to 
use both weak/strong in the same session. Couldn't we just mark the 
watcher with a "implements WeakWatcher" obviating the need for an 
explicit parameter?

An additoinal option - we could do the same thing by instead having a 
"ZooKeeperWeakWatches" subclass of zookeeper that provides this 
additional functionality (if we could overcome other objections) which 
would not impact existing users. You might be able to do this today, or 
with minimal changes to the ZooKeeper class...

> I would prefer deregisterWatcher, as a result.

Seems like this would be a useful feature. Today you can get away with 
not doing this by using something like a StrongWatcher proxy similar to 
above (or some other way of ignoring the eventual trigger). Each 
deregister requires a roundtrip to the server however (also handle the 
case when disconnected of course). We might want to allow multiple paths 
to be dereg as once... (the server will kill the connection if the 
request packet size exceeds 1mb though so keep that in mind too...). Not 
sure if there are other technical issues to consider.

Patrick

Dominic Williams wrote:
> Quite a few platforms make the specification of weak listeners explicit e.g.
> in ActionScript you can specify a boolean in addEventListener that specifies
> whether the event source should hold weak references to listeners. Therefore
> I think whether you want an event source to hold a weak or strong reference
> to your listener is really an application-level choice i.e. it is the
> application logic's choice whether to supply an anonymous inner class
> instance that will disappear unless the event source holds a strong
> reference, or a named object that you want to be freed from memory when
> *you* remove your references to it.
> 
> I think for many application programming problems, there is no nice way to
> manage memory unless an event source offers to hold weak references to
> listeners.
> 
> deregisterWatcher certainly works for things like locks where you do try {
> lock.lock(); } finally { lock.unlock(); } but how about where you are
> continually creating objects that maintain a copy of some items beneath
> nodes (where, say, you are constantly changing your focus from one node to
> another).
> 
> In this case, most probably you will want to create a primitive that
> calls deregisterWatcher in finalize(). But the problem of course is that
> finalize() will never get called.
> 
> For that reason, you end up with references to primitives on which you
> *must* call close() before you lose a reference to them.
> 
> In practice, this is just not possible to do reliably which is why I don't
> think there can be a substitute to weak references.
> 
> Best, Dominic
> 
> On 19 March 2010 18:47, Henry Robinson <he...@cloudera.com> wrote:
> 
>> (moved to zookeeper-dev)
>>
>> This API exposes internal implementation details which ties future versions
>> of the client to supporting a particular set of semantics.
>>
>> I would prefer deregisterWatcher, as a result.
>>
>> Henry
>>
>> On 19 March 2010 03:11, Dominic Williams <thedwilliams@googlemail.com
>>> wrote:
>>> Hi I can see some people might be assigning for example anonymous class
>>> instances as watchers/handlers, and not keeping any references to them.
>>>
>>> To avoid breaking existing use cases, two options:
>>>
>>> 1/ Add a useWeakReferences parameter to new constructor (sets default
>>> behaviour)
>>>
>>> 2/ Add alternative methods, which take useWeakWatcherRef boolean.
>>>
>>> Internally will be trivial to have both
>>>
>>> private final Map<String, Set<Watcher>> dataWatches =
>>>            new HashMap<String, Set<Watcher>>();
>>>
>>> private final Map<String, WeakSet<Watcher>> dataWatchesWeak =
>>>             new HashMap<String, WeakSet<Watcher>>();
>>>
>>> On 18 March 2010 22:47, Ted Dunning <te...@gmail.com> wrote:
>>>
>>>> This kind of sounds strange to me.
>>>>
>>>> My typical idiom is to create a watcher but not retain any references
>> to
>>> it
>>>> outside the client.  It sounds to me like your change will cause my
>>>> watchers
>>>> to be collected and deactivated when GC happens.
>>>>
>>>> On Thu, Mar 18, 2010 at 3:32 AM, Dominic Williams <
>>>> thedwilliams@googlemail.com> wrote:
>>>>
>>>>> The current ZooKeeper client holds strong references to Watcher
>>> objects.
>>>> I
>>>>> want to change the client so it only holds weak references. Feedback
>>>>> please.
>>
>>
>> --
>> Henry Robinson
>> Software Engineer
>> Cloudera
>> 415-994-6679
>>
> 

Re: Modify ZooKeeper Java client to hold weak references to Watcher objects

[Dominic Williams <th...@googlemail.com>]

On 22 March 2010 16:59, Patrick Hunt <ph...@apache.org> wrote:

> Would "WeakWatcher", a proxy watcher that holds a weak reference to the
> proxied watcher, not also solve this with minimal overhead? You could do
> this today.
>
>
Hi,

Yup a WeakWatcher would certainly provide an interim solution.


>
>  1/ Add a useWeakReferences parameter to new constructor (sets default
>> behaviour)
>> 2/ Add alternative methods, which take useWeakWatcherRef boolean.
>>
>
> I would prefer option 2 over 1, since I could see some users wishing to use
> both weak/strong in the same session. Couldn't we just mark the watcher with
> a "implements WeakWatcher" obviating the need for an explicit parameter?
>

I agree would be clearer just to add parameter to methods. If you set this
through a constructor option people might lose track of what the default
behaviour was.


>
> An additoinal option - we could do the same thing by instead having a
> "ZooKeeperWeakWatches" subclass of zookeeper that provides this additional
> functionality (if we could overcome other objections) which would not impact
> existing users. You might be able to do this today, or with minimal changes
> to the ZooKeeper class...
>
>
>  I would prefer deregisterWatcher, as a result.
>>
>
> Seems like this would be a useful feature. Today you can get away with not
> doing this by using something like a StrongWatcher proxy similar to above
> (or some other way of ignoring the eventual trigger). Each deregister
> requires a roundtrip to the server however (also handle the case when
> disconnected of course). We might want to allow multiple paths to be dereg
> as once... (the server will kill the connection if the request packet size
> exceeds 1mb though so keep that in mind too...). Not sure if there are other
> technical issues to consider.
>

I guess an argument for deregisterWatcher is this: even if you request for
your Watcher to be held with a weak reference, just because you've lost your
own reference doesn't mean than the GC has had time to determine that it is
now unreferenced and that the weak reference held by the Zk client will
return null.

Consequently, if you have code which reacts to events, there is a race
condition where your zombie could come back to life unless it has idempotent
behaviour.

I was thinking about this in relation to a simple set primitive called
ZkDistributedSet I have developed (pasted below).

Just say for example I have an File Explorer type application. As the user
selects a new node, I create a new ZkDistributedSet and assign it to
explorer.currentNodeContents. I also register the FileExplorer to receive
onStateChange events with the ZkDistributedSet base class ZkSyncPrimitive.

Now, as I browse the node hierarchy, I simply do explorer.currentNodeContent
=  new ZkDistributedSet(currentNodePath). A problem is going to be that
occasionally I am going to receive events generated when nodes I visited
earlier have changed.

In this case it doesn't matter so much... the user might simply see an extra
flicker as the File Explorer unnecessarily redraws the contents of the
current node. The question is whether there are other situations where this
could cause a real problem.

It seems probably not. So long as the behaviour of the primitives is
idempotent, and the client code does not make assumptions about the state of
primitives it holds references to when it hears one has changed.


> Patrick
>
>
> Dominic Williams wrote:
>
>> Quite a few platforms make the specification of weak listeners explicit
>> e.g.
>> in ActionScript you can specify a boolean in addEventListener that
>> specifies
>> whether the event source should hold weak references to listeners.
>> Therefore
>> I think whether you want an event source to hold a weak or strong
>> reference
>> to your listener is really an application-level choice i.e. it is the
>> application logic's choice whether to supply an anonymous inner class
>> instance that will disappear unless the event source holds a strong
>> reference, or a named object that you want to be freed from memory when
>> *you* remove your references to it.
>>
>> I think for many application programming problems, there is no nice way to
>> manage memory unless an event source offers to hold weak references to
>> listeners.
>>
>> deregisterWatcher certainly works for things like locks where you do try {
>> lock.lock(); } finally { lock.unlock(); } but how about where you are
>> continually creating objects that maintain a copy of some items beneath
>> nodes (where, say, you are constantly changing your focus from one node to
>> another).
>>
>> In this case, most probably you will want to create a primitive that
>> calls deregisterWatcher in finalize(). But the problem of course is that
>> finalize() will never get called.
>>
>> For that reason, you end up with references to primitives on which you
>> *must* call close() before you lose a reference to them.
>>
>> In practice, this is just not possible to do reliably which is why I don't
>> think there can be a substitute to weak references.
>>
>> Best, Dominic
>>
>> On 19 March 2010 18:47, Henry Robinson <he...@cloudera.com> wrote:
>>
>>  (moved to zookeeper-dev)
>>>
>>> This API exposes internal implementation details which ties future
>>> versions
>>> of the client to supporting a particular set of semantics.
>>>
>>> I would prefer deregisterWatcher, as a result.
>>>
>>> Henry
>>>
>>> On 19 March 2010 03:11, Dominic Williams <thedwilliams@googlemail.com
>>>
>>>> wrote:
>>>> Hi I can see some people might be assigning for example anonymous class
>>>> instances as watchers/handlers, and not keeping any references to them.
>>>>
>>>> To avoid breaking existing use cases, two options:
>>>>
>>>> 1/ Add a useWeakReferences parameter to new constructor (sets default
>>>> behaviour)
>>>>
>>>> 2/ Add alternative methods, which take useWeakWatcherRef boolean.
>>>>
>>>> Internally will be trivial to have both
>>>>
>>>> private final Map<String, Set<Watcher>> dataWatches =
>>>>           new HashMap<String, Set<Watcher>>();
>>>>
>>>> private final Map<String, WeakSet<Watcher>> dataWatchesWeak =
>>>>            new HashMap<String, WeakSet<Watcher>>();
>>>>
>>>> On 18 March 2010 22:47, Ted Dunning <te...@gmail.com> wrote:
>>>>
>>>>  This kind of sounds strange to me.
>>>>>
>>>>> My typical idiom is to create a watcher but not retain any references
>>>>>
>>>> to
>>>
>>>> it
>>>>
>>>>> outside the client.  It sounds to me like your change will cause my
>>>>> watchers
>>>>> to be collected and deactivated when GC happens.
>>>>>
>>>>> On Thu, Mar 18, 2010 at 3:32 AM, Dominic Williams <
>>>>> thedwilliams@googlemail.com> wrote:
>>>>>
>>>>>  The current ZooKeeper client holds strong references to Watcher
>>>>>>
>>>>> objects.
>>>>
>>>>> I
>>>>>
>>>>>> want to change the client so it only holds weak references. Feedback
>>>>>> please.
>>>>>>
>>>>>
>>>
>>> --
>>> Henry Robinson
>>> Software Engineer
>>> Cloudera
>>> 415-994-6679
>>>
>>>
>>