The Source for Java Technology Collaboration

Home » java.net Forums » Mobile & Embedded » LWUIT

Thread: Overlarge synchronized blocks in Display causing deadlocks?

Welcome, Guest Help
Login Login
Guest Settings Guest Settings
This question is not answered. Helpful answers available: 2. Correct answers available: 1.

Reply to this Thread Reply to this Thread Search Forum Search Forum Back to Thread List Back to Thread List

Permlink Replies: 4 - Last Post: Jun 24, 2009 6:15 AM by: Chen Fishbein
michaelmaguire

Posts: 55
Overlarge synchronized blocks in Display causing deadlocks?
Posted: Jun 22, 2009 12:57 PM
 
  Click to reply to this thread Reply

We are in the process of converting our app over to using LWUIT.

Recently I converted our app's internal LOG class over to using LWUIT, which means that throughout our code, we make calls which must update the LWUIT version of our LogViewerForm (when it's opened). Because some of these calls are from different threads, we used Display.callSerially() to ensure UI updates to our LogViewerForm occur on the EDT.

This change turned out to be an instant deadlock stress tester.

Display.callSerially() (very reasonably) synchronizes a lock object to ensure it adds the Runnable to pendingSerialCalls correctly:

    public void callSerially(Runnable r){
        if(isEdt()) {
            throw new IllegalStateException("Call serially must never be invoked from the EDT");
        }
        synchronized(lock) {
            pendingSerialCalls.addElement(r);
            lock.notify();
        }
    }


The issue appears to be that mainEDTLoop is calling back out into our client code while syncrhronized on this same lock.

If at any point inside our client code we do a callSerially() while already holding e.g. our own storage lock, then if the EDT thread in mainEDTLoop calls back out into our code while holding its lock and our code then tries to synchronize on our storage lock (e.g. to get a list of items to paint) then we are deadlocked.

We have found that the following fix where the scope of the synchronized(lock) in Display.mainEDTLoop() is smaller seems to work well without any unwanted side effects:

    void mainEDTLoop() {
        try {
            
            /**
             * BlueWhaleSystems fix: Mark Burton  18 Jun 2009
             * 
             * An overlarge synchronized block in mainEDTLoop() appears to be the cause of both of these deadlocks.
             * 
             * Avoid deadlocks by only locking around minimum amount of code needed.
             * We shouldn't need to lock around the whole loop here -- the calls inside are called
             * elsewhere without a lock already obtained and appear to already have their own 
             * internal calls to synchronize(lock) as needed.
             */
            //synchronized(lock)
            {
                // when there is no current form the EDT is useful only
                // for features such as call serially
                while(impl.getCurrentForm() == null) {
                    
                    /**
                     * BlueWhaleSystems fix: Mark Burton  18 Jun 2009
                     * 
                     * An overlarge synchronized block in mainEDTLoop() appears to be the cause of both of these deadlocks.
                     * 
                     * Avoid deadlocks by only locking around minimum amount of code needed.
                     * Just lock around shouldEDTSleep() and the lock.wait(). 
                     */
                    synchronized(lock){
                        if(shouldEDTSleep()) {
                            lock.wait();
                        }
                    }
 
                    // paint transition or intro animations and don't do anything else if such
                    // animations are in progress...
                    if(animationQueue != null && animationQueue.size() > 0) {
                        paintTransitionAnimation();
                        continue;
                    }
                    processSerialCalls();
                }
            }
    [...]


but we'd appreciate a commentary from LWUIT team about whether our proposed solution will break something else.

In general, calling an API like this, I wouldn't expect it to hold a lock while calling back out into client code.

thorsten_s

Posts: 94
Re: Overlarge synchronized blocks in Display causing deadlocks?
Posted: Jun 22, 2009 10:33 PM   in response to: michaelmaguire
 
  Click to reply to this thread Reply

Hi,
this is interesting and I also think it would be nice if the serial calls would be running without holding the display lock. But it has been a while since I have had a deep lock at the inner EDT workings, so I don't know if this is possible without changing lots of stuff.

Maybe the quick way would be to provide public access to the Display.lock object.

To Michael: I think the safer way (for now) would be to then synchronize on the Display.lock object instead of introducing another lock within your app. But I am only a blackberry sidekick, let's wait for the officials :)

Kind regards,
Thorsten.

Chen Fishbein
Re: Overlarge synchronized blocks in Display causing deadlocks?
Posted: Jun 24, 2009 1:18 AM   in response to: thorsten_s
  Click to reply to this thread Reply

Hi,
The synchronization code in the mainEDTLoop method will happen only once
when there is no Form visible yet on the screen and this code block must
be synchronized.
The EDT is mainly inside the while(true) method on the bottom invoking
edtLoopImpl which is most of the time not synchronized.

Regards,
Chen


lwuit-users@mobileandembedded.org wrote:
> Hi,
> this is interesting and I also think it would be nice if the serial calls would be running without holding the display lock. But it has been a while since I have had a deep lock at the inner EDT workings, so I don't know if this is possible without changing lots of stuff.
>
> Maybe the quick way would be to provide public access to the Display.lock object.
>
> To Michael: I think the safer way (for now) would be to then synchronize on the Display.lock object instead of introducing another lock within your app. But I am only a blackberry sidekick, let's wait for the officials :)
>
> Kind regards,
> Thorsten.
> [Message sent by forum member 'thorsten_s' (thorsten_s)]
>
> http://forums.java.net/jive/thread.jspa?messageID=352448
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: users-unsubscribe@lwuit.dev.java.net
> For additional commands, e-mail: users-help@lwuit.dev.java.net
>
>
>

---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@lwuit.dev.java.net
For additional commands, e-mail: users-help@lwuit.dev.java.net


michaelmaguire

Posts: 55
Re: Overlarge synchronized blocks in Display causing deadlocks?
Posted: Jun 24, 2009 5:30 AM   in response to: Chen Fishbein
 
  Click to reply to this thread Reply

Chen,

Unfortunately "only once" is still enough to cause deadlocks. We have also run into the same issue in Display.setCurrent() (see below).

This will burn anyone coding to LWUIT who uses their own synchronization object(s) and attempts to use callSerially() from a worker thread.

We have a fix which works for us -- we're just trying to feed back to the LWUIT project so this problem doesn't burn others.

Effectively what's going on here are callbacks from the LWUIT API. I think it's a widely accepted 'best practice' for API design not to perform a callback into client code while holding locks. e.g. a trivially quick Google produces:

"Are you holding locks when you issue that callback?"

http://www.eclipse.org/webtools/development/arch_and_design/pq-apis-20041115.ppt

and:

http://www.codeproject.com/Articles/37474/Threadsafe-Events.aspx

and:

http://www.springerlink.com/content/mx26021211357444/

etc.

We're finding the same problem in Display.setCurrent() with a similar fix to the one above that appears to be working:

    void setCurrent(final Form newForm){
        if(edt == null) {
            throw new IllegalStateException("Initialize must be invoked before setCurrent!");
        }
        
        if(editingText) {
            switch(showDuringEdit) {
                case SHOW_DURING_EDIT_ALLOW_DISCARD:
                    break;
                case SHOW_DURING_EDIT_ALLOW_SAVE:
                    impl.saveTextEditingState();
                    break;
                case SHOW_DURING_EDIT_EXCEPTION:
                    throw new IllegalStateException("Show during edit");
                case SHOW_DURING_EDIT_IGNORE:
                    return;
                case SHOW_DURING_EDIT_SET_AS_NEXT:
                    impl.setCurrentForm(newForm);
                    return;
            }
        }
        
        if(!isEdt()) {
            callSerially(new RunnableWrapper(newForm, null));
            return;
        }
        
        Form current = impl.getCurrentForm();
        if(current != null){
            current.hidePopups();
            if(current.isInitialized()) {
                current.deinitializeImpl();
            }
        }
        if(!newForm.isInitialized()) {
            newForm.initComponentImpl();
        }
        
        newForm.setShouldCalcPreferredSize(true);
        newForm.layoutContainer();
 
        /**
         * BlueWhaleSystems fix: Tatiana Rybak 23 Jun 2009
         * 
         * See ticket: 3343 LWUIT: Deadlock when restarting the emulator -- two thread are both locking on Display.lock and Storage.iGlobalLock
         * See ticket: 3354 LWUIT: YAD (Yet another deadlock) - not yet sure where it's happening.
         * See ticket: 3370 LWUIT: Another dead lock between the Display.lock and Storage.iGlobalLock.
         * 
         * An overlarge synchronized block in setCurrent() appears to be the cause of both of these deadlocks.
         * 
         * Avoid deadlocks by only locking around minimum amount of code needed.
         * We shouldn't need to lock around the all the code here -- the calls inside are called
         * elsewhere without a lock already obtained and appear to already have their own 
         * internal calls to synchronize(lock) as needed.
         */
        //synchronized(lock) {
            boolean transitionExists = false;
            if(animationQueue != null && animationQueue.size() > 0) {
                Object o = animationQueue.lastElement();
                if(o instanceof Transition) {
                    current = (Form)((Transition)o).getDestination();
                    impl.setCurrentForm(current);
                }
            }
 
            // make sure the fold menu occurs as expected then set the current
            // to the correct parent!
            if(current != null && current instanceof Dialog && ((Dialog)current).isMenu()) {
                Transition t = current.getTransitionOutAnimator();
                if(t != null) {
                    // go back to the parent form first
                    if(((Dialog)current).getPreviousForm() != null) {
                        initTransition(t.copy(), current, ((Dialog)current).getPreviousForm());
                    }
                }
                current = ((Dialog)current).getPreviousForm();
                impl.setCurrentForm(current);
            }
            
            // prevent the transition from occurring from a form into itself
            if(newForm != current) {
                if((current != null && current.getTransitionOutAnimator() != null) || newForm.getTransitionInAnimator() != null) {
                    
                    /**
                     * BlueWhaleSystems fix: Tatiana Rybak 23 Jun 2009
                     *
                     * See ticket: 3343 LWUIT: Deadlock when restarting the emulator -- two thread are both locking on Display.lock and Storage.iGlobalLock
                     * See ticket: 3354 LWUIT: YAD (Yet another deadlock) - not yet sure where it's happening.
                     * See ticket: 3370 LWUIT: Another dead lock between the Display.lock and Storage.iGlobalLock.
                     * 
                     * An overlarge synchronized block in setCurrent() appears to be the cause of both of these deadlocks.
                     *
                     * Avoid deadlocks by only locking around minimum amount of code needed.
                     * Just lock around creating new animationQueue and the lock.wait(). 
                     */ 
                    synchronized( lock ) {
                        if(animationQueue == null) {
                            animationQueue = new Vector();
                        }
                    }
                    
                    // prevent form transitions from breaking our dialog based
                    // transitions which are a bit sensitive
                    if(current != null && (!(newForm instanceof Dialog))) {
                        Transition t = current.getTransitionOutAnimator();
                        if(current != null && t != null) {
                            initTransition(t.copy(), current, newForm);
                            transitionExists = true;
                        }
                    }
                    if(current != null && !(current instanceof Dialog)) {
                        Transition t = newForm.getTransitionInAnimator();
                        if(t != null) {
                            initTransition(t.copy(), current, newForm);
                            transitionExists = true;
                        }
                    }
                }
            } 
            
            /**
             * BlueWhaleSystems fix: Tatiana Rybak 23 Jun 2009
             *
             * See ticket: 3343 LWUIT: Deadlock when restarting the emulator -- two thread are both locking on Display.lock and Storage.iGlobalLock
             * See ticket: 3354 LWUIT: YAD (Yet another deadlock) - not yet sure where it's happening.
             * See ticket: 3370 LWUIT: Another dead lock between the Display.lock and Storage.iGlobalLock.
             * 
             * An overlarge synchronized block in setCurrent() appears to be the cause of both of these deadlocks.
             *
             * Avoid deadlocks by only locking around minimum amount of code needed.
             * Just lock around creating new animationQueue and the lock.wait(). 
             */             
            synchronized( lock ) {
                lock.notify();
            }            
            
            if(!transitionExists) {
                if(animationQueue == null || animationQueue.size() == 0) {
                    setCurrentForm(newForm);
                } else {
                    // we need to add an empty transition to "serialize" this
                    // screen change...
                    Transition t = CommonTransitions.createEmpty();
                    initTransition(t, current, newForm);
                }
            }
        //}
    }


If there's a reason you know why the whole bIocks must be synchronized in these cases, it would be really great if you could let us know so that we can all help to fix the issues so that the LWUIT API doesn't call into client code while holding a lock.

If there's a reason why the fixes above don't work and it's absolutely impossible to find some other fix (I don't think it should be) then I think the general notes on the LWUIT API need a comment about threading (as in 'not thread safe if client uses it's own lock(s) and invokes Display.callSerially() from other threads') and as well I think we need to take Thorsten's advice and open up access to Display's lock so that apps requiring syncrhonization and worker thread access to the UI can share that single lock (although I think this would be unfortunate).

Chen Fishbein
Re: Overlarge synchronized blocks in Display causing deadlocks?
Posted: Jun 24, 2009 6:15 AM   in response to: michaelmaguire
  Click to reply to this thread Reply

Hi,
The synchronization at the starting of the EDT was to prevent dead locks
with the midp thread(there are some vm that will break when you
uncomment that code).
Regarding the other sync stuff I'll have a look

Chen

lwuit-users@mobileandembedded.org wrote:
> Chen,
>
> Unfortunately "only once" is still enough to cause deadlocks. We have also run into the same issue in Display.setCurrent() (see below).
>
> This will burn anyone coding to LWUIT who uses their own synchronization object(s) and attempts to use callSerially() from a worker thread.
>
> We have a fix which works for us -- we're just trying to feed back to the LWUIT project so this problem doesn't burn others.
>
> Effectively what's going on here are callbacks from the LWUIT API. I think it's a widely accepted 'best practice' for API design not to perform a callback into client code while holding locks. e.g. a trivially quick Google produces:
>
> "Are you holding locks when you issue that callback?"
>
> http://www.eclipse.org/webtools/development/arch_and_design/pq-apis-20041115.ppt
>
> and:
>
> http://www.codeproject.com/Articles/37474/Threadsafe-Events.aspx
>
> and:
>
> http://www.springerlink.com/content/mx26021211357444/
>
> etc.
>
> We're finding the same problem in Display.setCurrent() with a similar fix to the one above that appears to be working:
>
>
>     void setCurrent(final Form newForm){
>         if(edt == null) {
>             throw new IllegalStateException("Initialize must be invoked before setCurrent!");
>         }
>         
>         if(editingText) {
>             switch(showDuringEdit) {
>                 case SHOW_DURING_EDIT_ALLOW_DISCARD:
>                     break;
>                 case SHOW_DURING_EDIT_ALLOW_SAVE:
>                     impl.saveTextEditingState();
>                     break;
>                 case SHOW_DURING_EDIT_EXCEPTION:
>                     throw new IllegalStateException("Show during edit");
>                 case SHOW_DURING_EDIT_IGNORE:
>                     return;
>                 case SHOW_DURING_EDIT_SET_AS_NEXT:
>                     impl.setCurrentForm(newForm);
>                     return;
>             }
>         }
>         
>         if(!isEdt()) {
>             callSerially(new RunnableWrapper(newForm, null));
>             return;
>         }
>         
>         Form current = impl.getCurrentForm();
>         if(current != null){
>             current.hidePopups();
>             if(current.isInitialized()) {
>                 current.deinitializeImpl();
>             }
>         }
>         if(!newForm.isInitialized()) {
>             newForm.initComponentImpl();
>         }
>         
>         newForm.setShouldCalcPreferredSize(true);
>         newForm.layoutContainer();
>
>         /**
>          * BlueWhaleSystems fix: Tatiana Rybak 23 Jun 2009
>          * 
>          * See ticket: 3343 LWUIT: Deadlock when restarting the emulator -- two thread are both locking on Display.lock and Storage.iGlobalLock
>          * See ticket: 3354 LWUIT: YAD (Yet another deadlock) - not yet sure where it's happening.
>          * See ticket: 3370 LWUIT: Another dead lock between the Display.lock and Storage.iGlobalLock.
>          * 
>          * An overlarge synchronized block in setCurrent() appears to be the cause of both of these deadlocks.
>          * 
>          * Avoid deadlocks by only locking around minimum amount of code needed.
>          * We shouldn't need to lock around the all the code here -- the calls inside are called
>          * elsewhere without a lock already obtained and appear to already have their own 
>          * internal calls to synchronize(lock) as needed.
>          */
>         //synchronized(lock) {
>             boolean transitionExists = false;
>             if(animationQueue != null && animationQueue.size() > 0) {
>                 Object o = animationQueue.lastElement();
>                 if(o instanceof Transition) {
>                     current = (Form)((Transition)o).getDestination();
>                     impl.setCurrentForm(current);
>                 }
>             }
>
>             // make sure the fold menu occurs as expected then set the current
>             // to the correct parent!
>             if(current != null && current instanceof Dialog && ((Dialog)current).isMenu()) {
>                 Transition t = current.getTransitionOutAnimator();
>                 if(t != null) {
>                     // go back to the parent form first
>                     if(((Dialog)current).getPreviousForm() != null) {
>                         initTransition(t.copy(), current, ((Dialog)current).getPreviousForm());
>                     }
>                 }
>                 current = ((Dialog)current).getPreviousForm();
>                 impl.setCurrentForm(current);
>             }
>             
>             // prevent the transition from occurring from a form into itself
>             if(newForm != current) {
>                 if((current != null && current.getTransitionOutAnimator() != null) || newForm.getTransitionInAnimator() != null) {
>                     
>                     /**
>                      * BlueWhaleSystems fix: Tatiana Rybak 23 Jun 2009
>                      *
>                      * See ticket: 3343 LWUIT: Deadlock when restarting the emulator -- two thread are both locking on Display.lock and Storage.iGlobalLock
>                      * See ticket: 3354 LWUIT: YAD (Yet another deadlock) - not yet sure where it's happening.
>                      * See ticket: 3370 LWUIT: Another dead lock between the Display.lock and Storage.iGlobalLock.
>                      * 
>                      * An overlarge synchronized block in setCurrent() appears to be the cause of both of these deadlocks.
>                      *
>                      * Avoid deadlocks by only locking around minimum amount of code needed.
>                      * Just lock around creating new animationQueue and the lock.wait(). 
>                      */ 
>                     synchronized( lock ) {
>                         if(animationQueue == null) {
>                             animationQueue = new Vector();
>                         }
>                     }
>                     
>                     // prevent form transitions from breaking our dialog based
>                     // transitions which are a bit sensitive
>                     if(current != null && (!(newForm instanceof Dialog))) {
>                         Transition t = current.getTransitionOutAnimator();
>                         if(current != null && t != null) {
>                             initTransition(t.copy(), current, newForm);
>                             transitionExists = true;
>                         }
>                     }
>                     if(current != null && !(current instanceof Dialog)) {
>                         Transition t = newForm.getTransitionInAnimator();
>                         if(t != null) {
>                             initTransition(t.copy(), current, newForm);
>                             transitionExists = true;
>                         }
>                     }
>                 }
>             } 
>             
>             /**
>              * BlueWhaleSystems fix: Tatiana Rybak 23 Jun 2009
>              *
>              * See ticket: 3343 LWUIT: Deadlock when restarting the emulator -- two thread are both locking on Display.lock and Storage.iGlobalLock
>              * See ticket: 3354 LWUIT: YAD (Yet another deadlock) - not yet sure where it's happening.
>              * See ticket: 3370 LWUIT: Another dead lock between the Display.lock and Storage.iGlobalLock.
>              * 
>              * An overlarge synchronized block in setCurrent() appears to be the cause of both of these deadlocks.
>              *
>              * Avoid deadlocks by only locking around minimum amount of code needed.
>              * Just lock around creating new animationQueue and the lock.wait(). 
>              */             
>             synchronized( lock ) {
>                 lock.notify();
>             }            
>             
>             if(!transitionExists) {
>                 if(animationQueue == null || animationQueue.size() == 0) {
>                     setCurrentForm(newForm);
>                 } else {
>                     // we need to add an empty transition to "serialize" this
>                     // screen change...
>                     Transition t = CommonTransitions.createEmpty();
>                     initTransition(t, current, newForm);
>                 }
>             }
>         //}
>     }
> 

>
> If there's a reason you know why the whole bIocks must be synchronized in these cases, it would be really great if you could let us know so that we can all help to fix the issues so that the LWUIT API doesn't call into client code while holding a lock.
>
> If there's a reason why the fixes above don't work and it's absolutely impossible to find some other fix (I don't think it should be) then I think the general notes on the LWUIT API need a comment about threading (as in 'not thread safe if client uses it's own lock(s) and invokes Display.callSerially() from other threads') and as well I think we need to take Thorsten's advice and open up access to Display's lock so that apps requiring syncrhonization and worker thread access to the UI can share that single lock (although I think this would be unfortunate).
> [Message sent by forum member 'michaelmaguire' (michaelmaguire)]
>
> http://forums.java.net/jive/thread.jspa?messageID=352715
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: users-unsubscribe@lwuit.dev.java.net
> For additional commands, e-mail: users-help@lwuit.dev.java.net
>
>
>

---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@lwuit.dev.java.net
For additional commands, e-mail: users-help@lwuit.dev.java.net





 XML java.net RSS