[RFC][plugin]: fix concurrency problems in PAV.java

[Deepak Bhole]

* Denis Lila <dlila at redhat.com> [2011-04-12 16:25]:
> > Perhaps I should have clarified a it more -- I couldn't come up with
> > negative consequences in *our* case as we use integer keys which hash
> > to
> > themselves, so there would never be any lower level buckets to worry
> > about.
> > 
> > So essentially we are using HashMap as an indexed list where an index
> > will have only one element and it is never clobbered.
> 
> It's true that all the hashes will be different, but just
> because two keys have different hashes does not mean that
> they will go in different chains, so the example I gave
> is still possible, even in our case.
> 
> > That said, your point about making an assumption about the internal
> > implementation is a valid concern. Though we are safe now, there is no
> > guarantee of it in the future.
> 
> Nice! So, can I push it?
> 

Andrew posted some valid concerns in another thread. After those are all
resolved, OK from me.

Cheers,
Deepak

> ----- Original Message -----
> > * Denis Lila <dlila at redhat.com> [2011-04-12 14:54]:
> > > > You have discarded the check for panel == null. Why?
> > >
> > > Because it's a local variable. If it's null, that's an
> > > infinite loop. Also, if it's null, it can't get
> > > there anyway, because an NPE would be thrown a few lines
> > > prior.
> > >
> > 
> > Ah okay, seems like that code was copied and pasted from elsewhere.
> > The
> > panel == null check occurs in multiple places.
> > 
> > > > It is unsafe to read only if we iterate over it, which we don't.
> > > > ConcurrentHapMap does not lock when retrieving either. From the
> > > > JavaDocs:
> > > >
> > > > "However, even though all operations are thread-safe, retrieval
> > > > operations do not entail locking ..."
> > > >
> > > > So status is ok.
> > >
> > > > As mentioned above, status is OK as a HashMap. I think rather than
> > > > making applets a ConcurrentMap, write to it should be synchronized
> > > > instead. The map is written to only in one location, and it
> > > > happens
> > > > only
> > > > once per applet init, so race for it will be rare. OTOH, we read
> > > > it in
> > > > many places, and since ConcurrentHashMap does not guarantee
> > > > non-blocking
> > > > read in perpetuity, I think we should avoid it.
> > >
> > > I don't think status is ok, and I don't think synchronizing only the
> > > writes to applets is right. The docs for HashMap say "If multiple
> > > threads access a hash map concurrently, and at least one of the
> > > threads modifies the map structurally, it must be synchronized
> > > externally". We satisfy these requirements so we must use a
> > > ConcurrentHashMap (or do our own locking). The reason for this is
> > > that HashMap was not build with concurrent access in mind and it's
> > > not safe for "get" to be traversing the internal data structures
> > > of the map while some other thread is modifying these data
> > > structures.
> > >
> > > For example:
> > > suppose a map is implemented by chaining, and suppose we have a
> > > concurrent get(x) and remove(y) in progress. Suppose also that
> > > x.hashCode()==y.hashCode(), so they're in the same chain, and that
> > > x is after y in the chain. y is removed from the chain with a
> > > sequence of operations like this:
> > > p = y.prev
> > > n = y.next
> > > p.next = n
> > > n.prev = p
> > > y.prev = null
> > > y.next = null
> > >
> > > Now, the chain traversal in get(x) could get to p when p.next == y,
> > > so then it goes to y, but then y.next=null could execute, and get(x)
> > > will think that the end of the chain has been reached and it will
> > > wrongly return null.
> > >
> > 
> 
> > 
> 
> > 
> > >
> 
> > 
> > Deepak