hadoop-zookeeper-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Chris Darroch (JIRA)" <j...@apache.org>
Subject [jira] Commented: (ZOOKEEPER-318) remove locking in zk_hashtable.c or add locking in collect_keys()
Date Thu, 26 Feb 2009 21:35:01 GMT

    [ https://issues.apache.org/jira/browse/ZOOKEEPER-318?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12677147#action_12677147

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.

View raw message