[
https://issues.apache.org/jira/browse/CURATOR-164?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14514918#comment-14514918
]
Mike Drob commented on CURATOR-164:
-----------------------------------
nit: Can we call the class something other than Entry? That may lead to
confusion with {{java.util.Map.Entry}}.
style: In makeNodeCache you can avoid the ternary and null check by doing {{if
(!watchInstances) return null; // no work; fail fast}}
compatibility: Should updateService be throwing an Exception? Old behaviour was
to incorrectly reregister the unregistered service, right? Maybe better to just
log and ignore the request rather than exploding? I don't know where that
exception will propagate to otherwise.
style, documentation: In unregisterService, can you split the remove and call
to unregister into two lines? Since we're dealing with concurrency issues, it
makes that logic easier to reason about, and doesn't give the illusion of
atomicity. (Should that change be atomic and/or synchronized around? I think
it's fine, but just want to make sure we explicitly acknowledge this.)
correctness: In internalUnregisterService, that looks almost like a
double-checked locking. Could entry be set to null after the if, but before we
get to the synchronized block? Probably a good idea to make a local copy.
> curator-x-discovery: unregisterService is not guaranteed to remove the
> service, due to reconnectListener concurrency issue
> --------------------------------------------------------------------------------------------------------------------------
>
> Key: CURATOR-164
> URL: https://issues.apache.org/jira/browse/CURATOR-164
> Project: Apache Curator
> Issue Type: Bug
> Components: Framework
> Affects Versions: 2.7.0
> Reporter: Rasmus Berg Palm
> Assignee: Jordan Zimmerman
> Priority: Critical
> Fix For: 2.8.0
>
>
> In ServiceDiscoveryImpl:
> When unregistering a service, the reconnect listener might fire while
> deleting the path.
> This can cause a condition where the delete finishes successfully, the
> service is removed from services, and then the reRegisterServices completes
> successfully and the service is added back in ZK and in services, end result
> being that the service was not removed, even though unregisterService did not
> throw any exceptions.
> Essentially the use of the internal 'services' cache makes for a nightmare of
> concurrency issues. I put this as critical as the library it's really not
> usable IMO.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
