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

[Denis Lila]

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

I replied to that e-mail. Some of those were invalid
and I pointed those out. The others I think were good
points but they weren't directly related to this patch,
so I think they should be fixed in a future changeset.

The changes to destroyApplet are directly related to
this, but I would rather put them in the attached
refactoring patch because there's nothing wrong with
any of the changes in this patch and if I put them in this
patch, I'll have to remake the refactoring patch and
that will not be an efficient use of time.

Thank you,
Denis.

----- Original Message -----
> * 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?
> >
> 

> 
> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: RefactorPAV.patch
Type: text/x-patch
Size: 12330 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20110412/e78fe5a8/RefactorPAV.patch