Billie Rinaldi created SLIDER-1242:
--------------------------------------
Summary: 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
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)
