[
https://issues.apache.org/jira/browse/ZOOKEEPER-2141?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15216993#comment-15216993
]
Edward Ribeiro commented on ZOOKEEPER-2141:
-------------------------------------------
Hi [~adammilnesmith] and [~fournc], I am *really* sorry for only having
(limited) time to review/check this patch after it has been committed to trunk.
:( So, please, take it more like a couple of highlights, if you like.
The patch is really cool, and great work, but I have seen some things that
raised some questions I would like to point, plus a couple of minor (i.e.,
irrelevant) issues.
1. The piece of code below, from {{ReferenceCountedACLCache}}, worried me a bit:
{code}
public synchronized boolean equals(Object o) {
if (this == o) return true;
if (o == null || getClass() != o.getClass()) return false;
ReferenceCountedACLCache that = (ReferenceCountedACLCache) o;
synchronized (that) {
if (aclIndex != that.aclIndex) return false;
}
(...)
{code}
See, we are acquiring a lock of object {{foo1}} and then acquiring the lock of
object {{foo2}} ({{that}}, in this case) inside it. My question is: wouldn't
this potentially leads to a DEADLOCK? I mean, imagine a situation below, or
something alike:
{code}
Thread-1: foo1.equals(foo2);
Thread-2: foo2.equals(foo1);
{code}
I guess that with the *right thread interleaving*, it could lead to a deadlock.
I was so amused by this possibility that wrote a dummy snippet to evaluate
this, as you can see here (with appropriate insertion of {{Thread.sleeps}} to
reproduce the desired interleaving):
https://gist.github.com/eribeiro/bd2aca6fbae0255b64ba0b4f896e6a8d
and got some runs to successfully reproduce a deadlock as seen here:
https://gist.github.com/eribeiro/96f95b295a4b90c0d261057d4542063a
Disclaimer: maybe this situation NEVER happens in this particular case (if so,
sorry for disturbing you, *really*), but I thought it was worth mentioning. ;)
I *guess* a possible solution would be to make {{aclIndex}} a {{volatile}}
variable as a "lightweight" alternative to {{synchronized}}, right? Volatile
makes has a happens before semantic and it prevents reordering of statements.
Wdyt?
2. The inherited code has the following snippet (now moved to
{{ReferenceCountedACLCache}}):
{code}
public synchronized Long convertAcls(List<ACL> acls) {
if (acls == null)
return -1L;
{code}
What if the acls list is empty, i.e., {{acls = new ArrayList<>()}}? It's okay
to cache an empty ACL {{List}} or could we just change the lines above to:
{code}
public synchronized Long convertAcls(List<ACL> acls) {
if (acls == null || acls.isEmpty())
return -1L;
{code}
Again, this can NEVER happen in this case above so, I am all for letting it
as-is today. :)
3. The comment in the following snippet seems a bit out of date (list of
longs???):
{code}
/**
* converts the list of acls to a list of longs.
* Increments the reference counter for this ACL.
* @param acls
* @return a list of longs that map to the acls
*/
public synchronized Long convertAcls(List<ACL> acls) {
{code}
4. AtomicLongWithEquals extends AtomicLong. I failed to see a _good_ reason to
create a custom AtomicLong, as its instances are not used as keys. What is the
rationale behind this? Disclaimer: IMO, extending a j.u. library is an
anti-pattern as we don't have control over it and future changes in the parent
class can easily break the codebase.
5. ReferenceCountedACLCache.OPEN_UNSAFE_ACL_ID could be made static besides
being final.
6. ReferenceCountedACLCacheTest#convertACLsNTimes performs n-1 convertions as
seen here (see the ``<`` and the ``-1``):
{code}
for (int i = 0; i < num -1; i++) {
cache.convertAcls(acl);
}
{code}
Is this the intended behaviour?
There are other irrelevant observations, but I will refrain myself from citing
them by now.
Cheers!
Eddie
> ACL cache in DataTree never removes entries
> -------------------------------------------
>
> Key: ZOOKEEPER-2141
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2141
> Project: ZooKeeper
> Issue Type: Bug
> Affects Versions: 3.4.6
> Reporter: Karol Dudzinski
> Assignee: Adam Milne-Smith
> Fix For: 3.4.9, 3.5.2
>
> Attachments: ZOOKEEPER-2141-3.4.patch, ZOOKEEPER-2141.patch,
> ZOOKEEPER-2141.patch, ZOOKEEPER-2141.patch, ZOOKEEPER-2141.patch,
> ZOOKEEPER-2141.patch
>
>
> The problem and potential solutions are discussed in
> http://mail-archives.apache.org/mod_mbox/zookeeper-user/201502.mbox/browser
> I will attach a proposed patch in due course.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
