Re: [Pydev-cvs] org.python.pydev/src/org/python/pydev/outline PyOutlinePage.java, 1.25, 1.26 OutlineLinkWithEditorAction.java, 1.8, 1.9

[Fabio Z. <fa...@gm...>]

Hi Radim... Just fixed it. Thanks for reviewing it!

Cheers,

Fabio


On Fri, Aug 22, 2008 at 2:52 AM, Radim Kubacki <ra...@ku...> wrote:

> A comment about your synchronization patch:
>
> When I saw it I thought it cannot work. But I was surprised that
> autoboxing is so smart that
> Integer i; i++;
> causes conversion from Integer to int, increment and then (even if it
> is postfix increment) conversion back to Integer and storing to i
> variable. So far so good.
>
> 2nd idea: why volatile when every access is done inside this
> synchronized block?
>
> And now there is a reason why it does not work as you probably expect:
> Integer is an immutable object after each ++/-- you have another
> instance to synchronize on. So there can be more threads inside this
> synchronized block than one. For example if linkLevel is 0 one thread
> can start relinkAll and increment it, second thread calls unlinkAll
> and executes remove*() before add*() is called in 1st thread because
> it now sees different object assgned to linkLevel (and it volatile to
> inrease the chance that it will happen).
>
> Use
> private final Object lock = new Object();
> private int linkLevel = 1
> + synchronized(lock)
>
> Without the need for 1.4 compatibility you could also use
> AtomicInteger.{increment|decrement}AndGet()
>
> Yet another reason why your patch is bad: if you read
>
> http://java.sun.com/docs/books/jls/third_edition/html/conversions.html#5.1.7
> carefully you will see that the intent is to provide the same instance
> for the same primitive values. Synchronizing on a publicly known
> object is wrong. If someone else starts to do the same at some other
> random place in the codebase you can get interesting contention
> (better) or deadlocks (worse).
>
> -Radim
>
> On Thu, Aug 21, 2008 at 1:56 PM, Fabio Zadrozny
> <fa...@us...> wrote:
> > Update of /cvsroot/pydev/org.python.pydev/src/org/python/pydev/outline
> > In directory sc8-pr-cvs1.sourceforge.net:
> /tmp/cvs-serv19176/src/org/python/pydev/outline
> >
> > Modified Files:
> >        PyOutlinePage.java OutlineLinkWithEditorAction.java
> > Log Message:
> > Minor changes for console / possible race condition on linking with
> outline page.
> >
> > Index: PyOutlinePage.java
> > ===================================================================
> > RCS file:
> /cvsroot/pydev/org.python.pydev/src/org/python/pydev/outline/PyOutlinePage.java,v
> > retrieving revision 1.25
> > retrieving revision 1.26
> > ***************
> > *** 299,313 ****
> >      }
> >
> >        void unlinkAll() {
> > !               removeSelectionChangedListener(selectionListener);
> > !               if(linkWithEditor != null){
> > !                       linkWithEditor.unlink();
> >                }
> >        }
> >
> >        void relinkAll() {
> > !               addSelectionChangedListener(selectionListener);
> > !               if(linkWithEditor != null){
> > !                       linkWithEditor.relink();
> >                }
> >        }
> > --- 305,343 ----
> >      }
> >
> > +     /**
> > +      * Used to hold a link level to know when it should be unlinked or
> relinked, as calls can be 'cascaded'
> > +      */
> > +     private volatile Integer linkLevel = 1;
> > +
> > +     /**
> > +      * Stops listening to changes (the linkLevel is used so that
> multiple unlinks can be called and later
> > +      * multiple relinks should be used)
> > +      */
> >        void unlinkAll() {
> > !               synchronized (linkLevel) {
> > !                       linkLevel--;
> > !                       if(linkLevel == 0){
> > !
> removeSelectionChangedListener(selectionListener);
> > !                               if(linkWithEditor != null){
> > !                                       linkWithEditor.unlink();
> > !                               }
> > !                       }
> >                }
> >        }
> >
> > +       /**
> > +        * Starts listening to changes again if the number of relinks
> matches the number of unlinks
> > +        */
> >        void relinkAll() {
> > !               synchronized (linkLevel) {
> > !                       linkLevel++;
> > !                       if(linkLevel == 1){
> > !
> addSelectionChangedListener(selectionListener);
> > !                               if(linkWithEditor != null){
> > !                                       linkWithEditor.relink();
> > !                               }
> > !                       }else if(linkLevel > 1){
> > !                               throw new RuntimeException("Error:
> relinking without unlinking 1st");
> > !                       }
> >                }
> >        }
> >
> > Index: OutlineLinkWithEditorAction.java
> > ===================================================================
> > RCS file:
> /cvsroot/pydev/org.python.pydev/src/org/python/pydev/outline/OutlineLinkWithEditorAction.java,v
> > retrieving revision 1.8
> > retrieving revision 1.9
> > diff -C2 -d -r1.8 -r1.9
> > *** OutlineLinkWithEditorAction.java    5 Feb 2008 23:11:12 -0000
> 1.8
> > --- OutlineLinkWithEditorAction.java    21 Aug 2008 20:56:28 -0000
>  1.9
> > ***************
> > *** 25,28 ****
> > --- 25,34 ----
> >   * editor.
> >   *
> > +  * Design notes:
> > +  * It's linked on the constructor and unlinked in the destructor.
> > +  *
> > +  * It considers that it's always linked, even if the action is
> inactive, but before executing it, a
> > +  * check is done to see if it's active or not.
> > +  *
> >   * @author Fabio
> >   */
> > ***************
> > *** 40,43 ****
> > --- 46,52 ----
> >      }
> >
> > +     /**
> > +      * When called, it STOPS hearing notifications to update the
> outline when the cursor changes positions.
> > +      */
> >        public void unlink() {
> >                PyEdit edit = pyEdit.get();
> > ***************
> > *** 47,50 ****
> > --- 56,62 ----
> >        }
> >
> > +       /**
> > +        * When called, it STARTS hearing notifications to update the
> outline when the cursor changes positions.
> > +        */
> >        public void relink() {
> >                PyEdit edit = pyEdit.get();
> > ***************
> > *** 158,161 ****
> > --- 170,176 ----
> >
> >
> > +     /**
> > +      * @return The parsed item that should be selected given the
> startLine passed.
> > +      */
> >      private ParsedItem findSel(ParsedItem r, int startLine) {
> >          ParsedItem prev = null;
> >
> >
> > -------------------------------------------------------------------------
> > This SF.Net email is sponsored by the Moblin Your Move Developer's
> challenge
> > Build the coolest Linux based applications with Moblin SDK & win great
> prizes
> > Grand prize is a trip for two to an Open Source event anywhere in the
> world
> > http://moblin-contest.org/redirect.php?banner_id=100&url=/
> > _______________________________________________
> > Pydev-cvs mailing list
> > Pyd...@li...
> > https://lists.sourceforge.net/lists/listinfo/pydev-cvs
> >
>