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

[Denis Lila]

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

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

> > - static Vector<AppletPanel> appletPanels = new
> > Vector<AppletPanel>();
> > + private static Vector<AppletPanel> appletPanels = new
> > Vector<AppletPanel>();
> >
> 
> I haven't looked into this part of the code before, but from what I
> can
> see now, there appears to be a synchronization issue here too. We use
> elements() which does not shield against concurrent modification.
> Worse
> yet, it is not fail-fast so it could end up using bad data silently.
> 
> Iterator read and element write to appletPanels should be synchronized
> around the instance.

That's a good point. I completely forgot we were iterating through it.
We also need to synchronize a read earlier because we do "if(!contains) put"
and those 2 operations need to be atomic.

> The SuppressWarnings annotation is indented one more than the start of
> the method below. One of those 2 indents is wrong.

Yeah, Andrew pointed this out. I'm not sure how it happened (I let eclipse
do this sort of thing).

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

Regards,
Denis.

----- Original Message -----
> * Denis Lila <dlila at redhat.com> [2011-04-12 12:21]:
> > Hi.
> >
> > I found a few problems in PluginAppletViewer.java.
> > We were modifying/reading from two static hash maps
> > concurrently. The hash maps were "applets" and "status".
> > status was only modified in a synchronized method,
> > but we were reading from it outside of that method and
> > that's unsafe.
> >
> 
 applets map is written to unsynch. though, so that
> needs to be fixed. Not through ConcurrentMap though IMO. I have
> commented on it below.
> 
> > As far as I can tell, there are no group of operations
> > that need to happen atomically, so using ConcurrentHashMaps
> > should be enough.
> >
> > I've also done a bit of refactoring (removing dead code,
> > adding some SuppressWarnings annotations, making members
> > private, etc).
> >
> > Any criticism is appreciated.
> >
> > Thank you,
> > Denis.
> 
> <snip>
> >
> > - // Applet initialized. Find out it's classloader and add it to the
> > list
> > - String portComponent = doc.getPort() != -1 ? ":" + doc.getPort() :
> > "";
> > - String codeBase = doc.getProtocol() + "://" + doc.getHost() +
> > portComponent;
> > -
> > - if (atts.get("codebase") != null) {
> > - try {
> > - URL appletSrcURL = new URL(codeBase + atts.get("codebase"));
> > - codeBase = appletSrcURL.getProtocol() + "://" +
> > appletSrcURL.getHost();
> > - } catch (MalformedURLException mfue) {
> > - // do nothing
> > - }
> > - }
> > -
> > - // Wait for the panel to initialize
> > - // (happens in a separate thread)
> > - Applet a;
> > -
> >          // Wait for panel to come alive
> >          int maxWait = PluginAppletViewer.APPLET_TIMEOUT; // wait
> >          for panel to come alive
> >          int wait = 0;
> > - while ((panel == null) || (!((NetxPanel) panel).isAlive() && wait
> > < maxWait)) {
> > + while (!panel.isAlive() && wait < maxWait) {
> >              try {
> >                  Thread.sleep(50);
> >                  wait += 50;
> 
> 

> 
> > - private static HashMap<Integer, PluginAppletViewer> applets =
> > - new HashMap<Integer, PluginAppletViewer>();
> > + static ConcurrentMap<Integer, PluginAppletViewer> applets =
> > + new ConcurrentHashMap<Integer, PluginAppletViewer>();
> >
> >      private static PluginStreamHandler streamhandler;
> >
> >      private static PluginCallRequestFactory requestFactory;
> >
> > - private static HashMap<Integer, PAV_INIT_STATUS> status =
> > - new HashMap<Integer, PAV_INIT_STATUS>();
> > + private static ConcurrentMap<Integer, PAV_INIT_STATUS> status =
> > + new ConcurrentHashMap<Integer, PAV_INIT_STATUS>();
> >
> 

> 
> <snip>
> 
> >

> 
> <snip>
> > @@ -1645,7 +1587,8 @@
> >
> >          new Thread(new Runnable()
> >           {
> > - public void run()
> > + @SuppressWarnings("deprecation")
> > + public void run()
> 

> 
> Cheers,
> Deepak