pydev-cvs

[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...@us...>]

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
diff -C2 -d -r1.25 -r1.26
*** PyOutlinePage.java	4 Feb 2008 23:39:05 -0000	1.25
--- PyOutlinePage.java	21 Aug 2008 20:56:28 -0000	1.26
***************
*** 259,273 ****
                      	unlinkAll();
  	    				StructuredSelection sel = (StructuredSelection)event.getSelection();
  	                    if(sel.size() == 1) { // only sync the editing view if it is a single-selection
  	                        ParsedItem firstElement = (ParsedItem) sel.getFirstElement();
  	                        ErrorDescription errorDesc = firstElement.getErrorDesc();
                              if(errorDesc != null && errorDesc.message != null){
  	                            int len = errorDesc.errorEnd-errorDesc.errorStart;
  	                            editorView.setSelection(errorDesc.errorStart, len);
! 	                            return;
  	                        }
  	                    }
! 	    				SimpleNode[] node = model.getSelectionPosition(sel);
! 	    				editorView.revealModelNodes(node);
                      }finally{
                      	relinkAll();
--- 259,279 ----
                      	unlinkAll();
  	    				StructuredSelection sel = (StructuredSelection)event.getSelection();
+ 	    				
+ 	    				boolean alreadySelected = false;
  	                    if(sel.size() == 1) { // only sync the editing view if it is a single-selection
  	                        ParsedItem firstElement = (ParsedItem) sel.getFirstElement();
  	                        ErrorDescription errorDesc = firstElement.getErrorDesc();
+ 	                        
+ 	                        //select the error
                              if(errorDesc != null && errorDesc.message != null){
  	                            int len = errorDesc.errorEnd-errorDesc.errorStart;
  	                            editorView.setSelection(errorDesc.errorStart, len);
! 	                            alreadySelected = true;
  	                        }
  	                    }
! 	                    if(!alreadySelected){
! 		    				SimpleNode[] node = model.getSelectionPosition(sel);
! 		    				editorView.revealModelNodes(node);
! 	                    }
                      }finally{
                      	relinkAll();
***************
*** 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;

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

[Radim K. <ra...@ku...>]

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
>

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
> >
>