Chris Darroch commented on ZOOKEEPER-318:

Well, my own take on things like this and ZOOKEEPER-262 is that it's always 
good to keep things clean and simple -- a good day of programming in my book is 
one that removes more lines of code than are created and yet keeps the same or 
better functionality.

Aside from any incremental performance gains, I think the big win with both of 
these patches is that they make the purpose of the code that much more 
apparent.  A significant part of programming, I believe, is psychology.  A 
programmer who comes across a package laced with pthread_mutex_lock() 
statements immediately makes two pretty reasonable assumptions: the code is 
used in a multi-threaded context, and it's MT-safe.

In this case, both assumptions are incorrect; the code isn't used in an MT 
context and if it were to be, collect_keys() appears to be lacking the 
necessary locks and I suspect it would be MT-unsafe.  There could always be 
other subtle MT-related bugs which haven't been shaken out too, should one 
start using it in MT code.

Thus my own feeling is that it's better to simplify and remove these locks for 
a variety of reasons: it makes the code more self-documenting; easier to read, 
understand, and revise; and marginally faster.

Should the hashtables need to be used in an MT context in the future, the 
existing code can always be recovered quickly from SVN.  If there's an 
explanatory note in the SVN log that mentions the collect_keys() issue, all the 
better; then whoever might need to do this work will be prompted to think that 
aspect through as well.

That's just my two cents, of course.  :-)

> remove locking in zk_hashtable.c or add locking in collect_keys()
> -----------------------------------------------------------------
>                 Key: ZOOKEEPER-318
>                 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-318
>             Project: Zookeeper
>          Issue Type: Bug
>          Components: c client
>    Affects Versions: 3.0.0, 3.0.1, 3.1.0
>            Reporter: Chris Darroch
>             Fix For: 3.2.0, 4.0.0
>         Attachments: ZOOKEEPER-318.patch
> From a review of zk_hashtable.c it appears to me that all functions which 
> manipulate the hashtables are called from the IO thread, and therefore any 
> need for locking is obviated.
> If I'm wrong about that, then I think at a minimum collect_keys() should 
> acquire a lock in the same manner as collect_session_watchers().  Both 
> iterate over hashtable contents (in the latter case using copy_table()).
> However, from what I can see, the only function (besides the init/destroy 
> functions used when creating a zhandle_t) called from the completion thread 
> is deliverWatchers(), which simply iterates over a "delivery" list created 
> from the hashtables by collectWatchers().  The activateWatcher() function 
> contains comments which describe it being called by the completion thread, 
> but in fact it is called by the IO thread in zookeeper_process().
> I believe all calls to collectWatchers(), activateWatcher(), and 
> collect_keys() are made by the IO thread in zookeeper_interest(), 
> zookeeper_process(), check_events(), send_set_watches(), and handle_error().  
> Note that queue_session_event() is aliased as PROCESS_SESSION_EVENT, but 
> appears only in handle_error() and check_events().
> Also note that handle_error() is called only in zookeeper_process() and 
> handle_socket_error_msg(), which is used only by the IO thread, so far as I 
> can see.

This message is automatically generated by JIRA.
You can reply to this email to add a comment to the issue online.

Reply via email to