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

[Deepak Bhole]

* 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