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

[Denis Lila]

Eh, I forgot the attachment.

----- 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.
> >
> 
> 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. 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;
> 
> 
> You have discarded the check for panel == null. Why?
> 
> > - 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>();
> >
> 
> 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.
> 
> <snip>
> 
> >
> > - 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.
> 
> <snip>
> > @@ -1645,7 +1587,8 @@
> >
> >          new Thread(new Runnable()
> >           {
> > - public void run()
> > + @SuppressWarnings("deprecation")
> > + public void run()
> 
> The SuppressWarnings annotation is indented one more than the start of
> the method below. One of those 2 indents is wrong.
> 
> Cheers,
> Deepak
-------------- next part --------------
A non-text attachment was scrubbed...
Name: PAVConcurrency_v2.patch
Type: text/x-patch
Size: 5585 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20110412/41e97af6/PAVConcurrency_v2.patch