[
https://issues.apache.org/jira/browse/SLIDER-1242?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16148087#comment-16148087
]
Gour Saha commented on SLIDER-1242:
-----------------------------------
[~billie.rinaldi] one thing comes to my mind - Let's say in line A below if the
key actually exists, but the value is null for some reason. I don't know if
this scenario can ever occur as per the code logic, but let's assume that it is
possible. In that case containsKey in line B will return true and hence this
key will never get a new ConcurrentHashMap set to its value. I think it will be
safer to modify the second check in line B to "this.exportGroups.get(groupName)
== null" as well. What do you think?
{code}
A. if (this.exportGroups.get(groupName) == null) {
synchronized (this.exportGroups) {
B. if (!this.exportGroups.containsKey(groupName)) {
this.exportGroups.put(groupName, new ConcurrentHashMap<String,
List<ExportEntry>>());
}
}
}
{code}
> Review uses of double-checked locking
> -------------------------------------
>
> Key: SLIDER-1242
> URL: https://issues.apache.org/jira/browse/SLIDER-1242
> Project: Slider
> Issue Type: Bug
> Reporter: Billie Rinaldi
> Assignee: Billie Rinaldi
> Fix For: Slider 1.0.0
>
> Attachments: SLIDER-1242.1.patch
>
>
> There are several places where we perform double-checked locking. Even though
> the practice is discouraged, I believe it is technically correct when the
> check is performed on the presence of a ConcurrentHashMap key, which is how
> we are using it.
> However, in two places, AgentProviderService#getCurrentExports and
> AgentProviderService#getAllocatedPorts, containsKey is used instead of get to
> perform the check. I am seeing some indication that containsKey is not
> sufficient, and that get must be used for double-checked locking to be
> correct. There is a comment in the ConcurrentHashMap#containsKey method that
> says "same as get() except no need for volatile value read" -- and I think
> that volatile value read is what we need for correctness.
> Also, in the [ConcurrentHashMap api
> doc|https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/ConcurrentHashMap.html],
> it specifically mentions get and does not mention containsKey: "Any non-null
> result returned from get(key) and related access methods bears a
> happens-before relation with the associated insertion or update" and "an
> update operation for a given key bears a happens-before relation with any
> (non-null) retrieval for that key reporting the updated value."
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
