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

[Deepak Bhole]

* Dr Andrew John Hughes <ahughes at redhat.com> [2011-04-12 19:07]:
> On 16:49 Tue 12 Apr     , Denis Lila wrote:
> > > while (!applets.containsKey(identifier) && // Map is populated only by
> > > reFrame
> > > (wait < maxWait)) {
> > > 
> > > try {
> > > Thread.sleep(50);
> > > wait += 50;
> > > } catch (InterruptedException ie) {
> > > // just wait
> > > }
> > > }
> > > 
> > > Surely this should be a timed wait and additions to applets should be
> > > accompanied by
> > > a notifyAll?
> > 
> > That's what I thought when I first saw this code. But this works, and I
> > see no practical reason to change it. If you really think we should, I
> > could do it, but it should be it's own changeset.
> > 
> 
> I'm not saying it should be in the same changeset.  I was enumerating problems
> that still exist with these collections to counter your claim that switching
> to CHMs will suddenly fix everything.  It won't.
> 
> The practical difference is this code will wake the thread every fifty milliseconds
> and reading from the map.  Using notifyAll would only wake waiting threads when the
> map has changed.
> 

I agree that this is an issue. I will post a separate patch for it.

I am also working on another patch to remove use of HashMaps completely.
All this discussion has made me realize that we are unnecessarily using
HashMaps for something that needs simple indexed access. Please do
commit the CHM change though, since it is already done and there is no
guarantee that my patch won't have to be backed out later.

Cheers,
Deepak

> > > PluginAppletViewer oldFrame = applets.get(identifier);
> > > 
> > > // We should not try to destroy an applet during
> > > // initialization. It may cause an inconsistent state,
> > > // which would bad if it's a trusted applet that
> > > // read/writes to files
> > > waitForAppletInit((NetxPanel) applets.get(identifier).panel);
> > > 
> > > The two get calls could retrieve different values if a put occurs in
> > > the interim.
> > 
> > Yes, but it doesn't matter. oldFrame is dead code.
> > 
> 
> Then it needs ripping out so it doesn't confuse people.
> 
> > > The cast to AppletPanel is redundant.
> > 
> > The enumeration is not using generics, so we need the cast.
> > 
> 
> Ok, so the enumeration needs fixing too.
> 
> Is your other patch handling all this generification?  I thought we'd caught most
> of them.
> 
> > 
> > > Indentation change. Shouldn't be in this patch.
> > ...<snip>...
> > > This looks like just brackets being added. Shouldn't be in this patch.
> > 
> > The indentation changed because the code was wrapped in
> > a synchronized block. Not putting it in this patch would be
> > breaking style conventions.
> > 
> 
> Ok.
> 
> Please don't remove code from the mail which is being discussed.  It was
> difficult to verify this again here because you removed the code.
> 
> > > To be frank, this class is a mess.
> > 
> > I couldn't agree more.
> > And it's not just this class. It's almost every class in the
> > icedtea-web sun.applet package.
> > 
> > As for the points you made that I didn't reply to:
> > None of them were pointing out any mistakes in my patch,
> 
> No, they were pointing out mistakes in your assumptions that
> a switch to CHMs is a fix-all.  It's a start, that's all, but
> I'd prefer everything was fixed.
> 
> > so can I push this patch, 
> 
> Yes.
> 
> > and address some of them
> > (in particular the comments on destroyApplet) in my second
> > patch (the refactoring one)?
> > 
> 
> Don't mix them up with other changes.  Separate patches please.
> 
> > Thank you,
> > Denis.
> > 
> > ----- Original Message -----
> > > On 15:23 Tue 12 Apr , Denis Lila wrote:
> > > 
> > > snip...
> > > 
> > >
> > snip...
> 
> -- 
> Andrew :)
> 
> Free Java Software Engineer
> Red Hat, Inc. (http://www.redhat.com)
> 
> Support Free Java!
> Contribute to GNU Classpath and IcedTea
> http://www.gnu.org/software/classpath
> http://icedtea.classpath.org
> PGP Key: F5862A37 (https://keys.indymedia.org/)
> Fingerprint = EA30 D855 D50F 90CD F54D  0698 0713 C3ED F586 2A37