Scott Blum created CURATOR-294:
----------------------------------
Summary: Optimize TreeCache, fix possible concurrency issue
Key: CURATOR-294
URL: https://issues.apache.org/jira/browse/CURATOR-294
Project: Apache Curator
Issue Type: Improvement
Components: Recipes
Reporter: Nick Hill
Assignee: Scott Blum
Hi, I have been looking at the TreeCache impl and have some questions.
It doesn't look right to me that there's separate atomic refs for a node's data
and stat. It seems the stat in a ChildData object obtained from
getCurrentData() might not correspond to the data that it's with. This could be
problematic when doing conditional state changes given assumptions about that
data.
An obvious and simple solution to this would be to have a single
AtomicReference<ChildData> field instead, which would have the additional
significant benefit of eliminating ChildData obj creation on every cache
access. PathChildrenCache works this way, but my understanding was that
TreeCache is intended to be a (more flexible) replacement.
Furthermore I'd propose that the data field of ChildData be just a final byte[]
instead of an AtomicReference. This would avoid needing two volatile reads to
get to the data, and mean that "sharing" these (per above) is a bit safer. The
ChildData byte[] AtomicReference is only used by
PathChildrenCache.clearDataBytes() (not currently used by TreeCache at all),
and that capability could be easily maintained by having PathChildrenCache use
it's own simple subclass of ChildData containing the atomic reference.
If similar capability were to be added to TreeCache, I'd suggest it would be
better to just replace the node's ChildData object with a copy that has the
byte[] field nulled out (but same stat ref).
I'm fairly new to the code so apologies if there's something I've
missed/misunderstood! But if there is agreement, I'd also be happy to prepare a
PR.
Regards,
Nick
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
