[icedtea-web] RFC: Patch to use wait() when waiting for conditions to become true
Dr Andrew John Hughes
ahughes at redhat.com
Thu Apr 14 13:32:49 PDT 2011
On 09:22 Thu 14 Apr , Deepak Bhole wrote:
> * Dr Andrew John Hughes <ahughes at redhat.com> [2011-04-14 07:41]:
> > On 01:56 Thu 14 Apr , Deepak Bhole wrote:
> > > * Deepak Bhole <dbhole at redhat.com> [2011-04-13 20:48]:
> > > > Hi,
> > > >
> > > > Andrew mentioned in one of the previous threads that the current plugin
> > > > code waits in an inefficient manner. It wakes up periodically to
> > > > check for conditions, and goes back to sleep, and keeps doing that
> > > > cyclically.
> > > >
> > > > The attached patch uses a new function that uses object.wait for
> > > > the object of interest.
> > > >
> > >
> > > Please see attached new patch. After leaving the office I realized that
> > > there is a potential race condition in the old patch with locking the
> > > object inside the wait function. Patch updated to do the locking
> > > outside (and function comment updated to clearly state that).
> > >
> >
> > Can you elaborate? If it's the same thread acquiring the lock in both
> > cases, then it should be fine as they are re-entrant i.e. if the function
> > attempts to lock it and the thread already holds the lock, it's a no-op.
> >
>
> Before, the locking was inside the function, like so:
>
> while !<check contition on OBJ>
> waitTillTimeout(OBJ, timeout)
> ...
> ...
>
>
> function ... waitTillTimeout(OBJ, timeout) {
>
> lock (OBJ)
> OBJ.wait()
> ...
> }
>
> The problem was that condition on OBJ could've been false at check, but then
> become true before waitTillTimeout() was called. This would result in an
> incorrect wait until timeout. By moving the lock to before condition checking,
> the code guarantees that waiting is done only if needed.
>
Oh yes, that wouldn't work. As I said, we need closures to do that all in one function :-(
> As I wrote this, I realized that in order for the above to work, we would have
> to lock the hashmap on update too since CHM's locking works differently and may
> not lock the full table. I will post a separate patch in a bit with the change.
> In the mean time, I have replied to other concerns below.
>
As I suggested in the previous mail, we should probably switch back to HMs and do
all locking in the one place.
> > > Cheers,
> > > Deepak
> >
> > Comments inline. As Jiri said, a ChangeLog would be good too.
> >
>
> Oops, sorry I should've mentioned, the ChangeLog is the same as before
> (only the locking area changed). Here it is:
>
> 2011-04-13 Deepak Bhole <dbhole at redhat.com>
>
> * plugin/icedteanp/java/sun/applet/PluginAppletViewer.java (createPanel):
> use Object.wait() to wait, rather than pariodic sleep.
> (framePanel): Notify threads waiting on the applets map instance.
> (appletStateChanged): Notify all threads waiting on the panel that just
> changed state.
> (handleMessage): Use the new waitTillTimeout function to wait, rather than
> periodically waking up.
> (updateStatus): Notify all threads waiting on status map.
> (waitForAppletInit): Use the new waitTillTimeout function to wait, rather
> than periodically waking up.
> (waitTillTimeout): New function. For a given non-null object, waits until
> the specified timeout, or, if an interrupt was thrown during wait, returns
> immediately.
Ok, didn't even read the previous e-mail when I saw it had been superceded.
>
> > > - // just wait
> > > + // Wait implemented the long way so that
> > > + // PluginAppletViewer.waitTillTimeout() needn't be exposed.
> > > + long maxTimeToSleep = PluginAppletViewer.APPLET_TIMEOUT;
> > > +
> > > + synchronized(panel) {
> > > + while (!panel.isAlive() && maxTimeToSleep > 0) {
> > > + long sleepStart = System.currentTimeMillis();
> > > +
> > > + try {
> > > + panel.wait(maxTimeToSleep);
> > > + } catch (InterruptedException e) {} // Just loop back
> > > +
> > > + maxTimeToSleep -= (System.currentTimeMillis() - sleepStart);
> >
> > So this one matches src.wait() below? It's a little unclear due to the differing
> > variable names.
> >
> > Is there a reason why this doesn't use waitTillTimeout?
> >
>
> I mentioned it in the comments, it is so that
> PluginAppletViewer.waitTillTimeout() could remain private (the above is in a
> separate class). I intend to re-factor a lot of that code (already started
> working on it yesterday) that will address it there. In the mean time, I
> saw no harm in duplicating a few lines of code.
Oh ok, wasn't clear from just the patch.
>
> > > }
> > > }
> > >
> > > @@ -307,6 +310,10 @@
> > > panel.addAppletListener(appletFrame.appletEventListener);
> > >
> > > applets.put(identifier, appletFrame);
> > > +
> > > + synchronized(applets) {
> > > + applets.notifyAll();
> > > + }
> > >
> > > PluginDebug.debug(panel, " framed");
> > > }
> >
> > This change is correct, but I'm a bit dubious about having one set of
> > locks in the CHM for the put and our own lock on applets here. Maybe
> > revert back to HashMaps and handle all locking in one place?
> >
>
> We would then have to lock on all gets too. CHMs locking is more
> efficient as it locks on only parts of the table. That is why I chose to
> do locking only around code that needs it (the API requires wait and
> notifyall to be calls from threads that own the monitor lock).
CHMs aren't locked on gets either. They do have visibility guarantees.
I'm a little worried about using the monitor of an object that we know does its
own internal locking. If we're keeping CHMs, we should use our own locks in this
class so we don't interfere. It doesn't matter what objects are used to synchronise
on, just that we are consistent.
>
> > > - } catch (InterruptedException ie) {
> > > - // just wait
> > > + int maxTimeToSleep = APPLET_TIMEOUT;
> > > + synchronized(status) {
> > > + while (!status.get(identifier).equals(PAV_INIT_STATUS.INIT_COMPLETE) &&
> > > + maxTimeToSleep > 0) {
> > > + maxTimeToSleep -= waitTillTimeout(status, maxTimeToSleep);
> >
> > Is status guaranteed to contain a mapping for identifier?
> >
>
> Yes, it is.
>
Ok was worried about the null which could be avoided by inverting the test.
> > > }
> > > }
> > >
> > > @@ -686,17 +693,21 @@
> > > // FIXME: how do we determine what security context this
> > > // object should belong to?
> > > Object o;
> > > -
> > > - // Wait for panel to come alive
> > > - int maxWait = APPLET_TIMEOUT; // wait for panel to come alive
> > > - int wait = 0;
> > > - while ((panel == null) || (!panel.isAlive() && wait < maxWait)) {
> > > +
> > > + // First, wait for panel to instantiate
> > > + int waited = 0;
> > > + while (panel == null && waited < APPLET_TIMEOUT) {
> > > try {
> > > Thread.sleep(50);
> > > - wait += 50;
> > > - } catch (InterruptedException ie) {
> > > - // just wait
> > > - }
> > > + waited += 50;
> > > + } catch (InterruptedException ie) {} // discard, loop back
> > > + }
> > > +
> > > + // Next, wait for panel to come alive
> > > + long maxTimeToSleep = APPLET_TIMEOUT;
> > > + synchronized(panel) {
> > > + while (!panel.isAlive())
> > > + maxTimeToSleep -= waitTillTimeout(panel, maxTimeToSleep);
> >
> > I presume panel is global to this instance?
> >
> > Maybe we should introduce something else to provide the conditional wait so you
> > don't need this split between the null/sleep(50) loop and the initialising/notification loop.
> >
>
> Yeah I wasn't too happy about having to keep the above. I thought about
> introducing an additional lock for the object, but the truth is that the
> sleep will happen for a very short time, if it ever does. Furthermore,
> the above would get called only for liveconnect based applets, so it
> didn't seem worth it. I can still do it if you prefer though.
>
I think it would be preferable to make the code clearer. It also removes the confusion with
src.wait above and any possible conflict with CHM locking. Easier enough to use just an
Object or the Lock/Condition objects provided by java.util.concurrent.
> >
> > Would have been nice to have closures for this. Then you could pass the condition
> > to this method, and have the whole block handled here. :-)
> >
>
> Only 20 more months if all goes well! :)
>
> Cheers,
> Deepak
>
> > > }
> >
> >
> > --
> > 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
--
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
More information about the distro-pkg-dev
mailing list