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

[Deepak Bhole]

* 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.
> 

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.

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.

> 
> I'll split the patch in 2 parts and send those in a few minutes.
> 

OK.

Deepak