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

[Denis Lila]

> 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?

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