Unsafe Construction in GL

[Compulinkltd]

I was reading a copy of Java Concurrency in Practice[1], and they
point out there are certain things you should never do inside a
constructor like call a method that could be overridden, or start a
thread because it can allow the this reference to escape from an
object before it is completely constructed.  I'm concerned that GL may
be doing something else they say you should never do in a constructor:
 publish an inner class instance.  The example they give looks like
this:

public class ThisEscape {
    public ThisEscape(EventSource source) {
        source.registerListener(
            new EventListener() {
                public void onEvent(Event e) {
                    doSomething(e);
                }
            });
    }
}

When ThisEscape publishes the EventListener, it implicitly publishes
the enclosing ThisEscape instance as well, because inner class
instances contain a hidden reference to the enclosing instance.  The
problem is that the event source has access to the "this" reference
before ThisEscape has been fully constructed.  They say it is a
problem even if the publication is the last statement in the
constructor.

The workaround they recommend is to use a static factory method. It's
OK to create a listener or a thread in a constructor, just don't
publish or start it.

public class SafeListener {
    private final EventListener listener;

    private SafeListener() {
        listener = new EventListener() {
            public void onEvent(Event e) {
               doSomething(e);
            }
        };
    }

    public static SafeListener newInstance(EventSource source) {
        SafeListener safe = new SafeListener();
        source.registerListener(safe.listener);
        return safe;
    }
}

When I looked at the Javadoc describing the constructors for classes
like ObservableElementList, I was concerned that might be happening.
When I looked at the source for ObservableElementList and saw lines
like the ones below in the constructor, I became even more concerned:

public ObservableElementList(EventList<E> source, Connector<? super E>
elementConnector) {
    this.elementConnector = elementConnector;

    // attach this list to the element connector so the listeners know
    // which List to notify of their modifications
   this.elementConnector.setObservableElementList(this);

   // for speed, we add all source elements together, rather than individually
    this.observedElements = new ArrayList<E>(source);

    // we initialize the single EventListener registry, as we optimistically
    // assume we'll be using a single listener for all observed elements
    this.singleEventListenerRegistry = new Barcode();
    this.singleEventListenerRegistry.addWhite(0, source.size());

    // add listeners to all source list elements
    for (int i = 0, n = size(); i < n; i++) {
        // connect a listener to the element
        final EventListener listener = this.connectElement(get(i));

        // record the listener in the registry
        this.registerListener(i, listener, false);
    }

    // begin listening to the source list
    source.addListEventListener(this);
}

The elementConnector sees the "this" reference before the
ObservableElementList is fully constructed.  Later, every list element
sees the "this" reference when the listener is registered on each
element.  Finally, the source list sees the "this" reference in the
last line of the constructor.

I'm concerned this idiom exists throughout GL, and it may be difficult
to fix without API changes. To emply the suggested workaround would
require making the constructors private and using factory methods
instead.  Maybe the GL folks already know about this?

Bruce

[1] Java Concurrency in Practice, Brian Goetz, Tim Peierls, Joshua
Bloch, Joseph Bowbeer, David Holmes, and Doug Lea Chapter 3.2.1 Safe
Construction Practices, pg. 41-42

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@...
For additional commands, e-mail: dev-help@...

[Endre Stølsvik-8]

I've also noticed the aspect the OP points out. Although I understand that it doesn't really make a problem until you have errors in your code (exceptions thrown), or you have code that "uses" this escaping in some way.

BUT: Don't make anything private - make it protected. (and don't use final either).

There is way to much privateness in GL already. I've been in need to override some small aspects of GL now and then (primarily with the resource handling (dispose), which I am not happy with) - and I've invariably have had to copy the entire class. And if the required methods aren't private - because they are a public part of the API - the class is instead final!

I sense the "But these are implementation details that we do not want to expose!!!!" and "That is not a part of the API that we will have to maintain!!!!" arguments building up, so I'll just dryly point out: Either I can extend these classes, or I have to copy them pretty much verbatim. Which of those strategies is "most exposed"? If you try to lock things down even more, I'll just pretty much have to "fork" the entire project into my own codebase. And again - which is the worst of these two approaches?

IMHO, The API is the public facing methods. That the /implementations/ are open for extension (non-final classes and protected methods instead of private) doesn't make it into a part of the API as such. So feel free to ruin my extensions for every version of GL issued. ( - and it wouldn't hurt sticking in some extra "hooks" either - like you already had with CollectionList.createChildElementForList(..), which of course is private).

Endre.

On Mon, Sep 7, 2009 at 17:51, Bruce Alspaugh <compulinkltd@...> wrote:

I was reading a copy of Java Concurrency in Practice[1], and they
point out there are certain things you should never do inside a
constructor like call a method that could be overridden, or start a
thread because it can allow the this reference to escape from an
object before it is completely constructed.  I'm concerned that GL may
be doing something else they say you should never do in a constructor:
 publish an inner class instance.  The example they give looks like
this:

public class ThisEscape {
   public ThisEscape(EventSource source) {
       source.registerListener(
           new EventListener() {
               public void onEvent(Event e) {
                   doSomething(e);
               }
           });
   }
}

When ThisEscape publishes the EventListener, it implicitly publishes
the enclosing ThisEscape instance as well, because inner class
instances contain a hidden reference to the enclosing instance.  The
problem is that the event source has access to the "this" reference
before ThisEscape has been fully constructed.  They say it is a
problem even if the publication is the last statement in the
constructor.

The workaround they recommend is to use a static factory method. It's
OK to create a listener or a thread in a constructor, just don't
publish or start it.

public class SafeListener {
   private final EventListener listener;

   private SafeListener() {
       listener = new EventListener() {
           public void onEvent(Event e) {
              doSomething(e);
           }
       };
   }

   public static SafeListener newInstance(EventSource source) {
       SafeListener safe = new SafeListener();
       source.registerListener(safe.listener);
       return safe;
   }
}

When I looked at the Javadoc describing the constructors for classes
like ObservableElementList, I was concerned that might be happening.
When I looked at the source for ObservableElementList and saw lines
like the ones below in the constructor, I became even more concerned:

public ObservableElementList(EventList<E> source, Connector<? super E>
elementConnector) {
   this.elementConnector = elementConnector;

   // attach this list to the element connector so the listeners know
   // which List to notify of their modifications
  this.elementConnector.setObservableElementList(this);

  // for speed, we add all source elements together, rather than individually
   this.observedElements = new ArrayList<E>(source);

   // we initialize the single EventListener registry, as we optimistically
   // assume we'll be using a single listener for all observed elements
   this.singleEventListenerRegistry = new Barcode();
   this.singleEventListenerRegistry.addWhite(0, source.size());

   // add listeners to all source list elements
   for (int i = 0, n = size(); i < n; i++) {
       // connect a listener to the element
       final EventListener listener = this.connectElement(get(i));

       // record the listener in the registry
       this.registerListener(i, listener, false);
   }

   // begin listening to the source list
   source.addListEventListener(this);
}

The elementConnector sees the "this" reference before the
ObservableElementList is fully constructed.  Later, every list element
sees the "this" reference when the listener is registered on each
element.  Finally, the source list sees the "this" reference in the
last line of the constructor.

I'm concerned this idiom exists throughout GL, and it may be difficult
to fix without API changes. To emply the suggested workaround would
require making the constructors private and using factory methods
instead.  Maybe the GL folks already know about this?

Bruce

[1] Java Concurrency in Practice, Brian Goetz, Tim Peierls, Joshua
Bloch, Joseph Bowbeer, David Holmes, and Doug Lea Chapter 3.2.1 Safe
Construction Practices, pg. 41-42

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@...
For additional commands, e-mail: dev-help@...

[Compulinkltd]

I agree that the dispose methods are a real problem. See my post:

http://www.nabble.com/WeakReferenceProxy-td25299875.html

However, I have to defend the GL designers when they followed best
practices in places where they prohibited inheritance. The central
problem is that extending a class breaks encapsulation.

Good API design is hard enough.  As Joshua Bloch points out[1], it is
very difficult to design a class that can be safely extended because of
all the problems you can run into[2].  Unlike C++, Java is a single
inheritance language which significantly limits what you can do with
inheritance.  Instead of extending a class, I recommend to use
composition whenever possible[3]. I like his quote, "If a class
implements some interface that captures its essence, such as Set, List,
or Map, then you should feel no compunction about prohibiting
subclassing.  The wrapper class pattern, described in Item 16, provides
a superior alternative to inheritance for augmenting the functionality."
pg. 91. For GL, EventList is that interface.

By the way, "Effective Java" is one of the best Java books I have ever
read.  Even James Gosling himself speaks highly of it.  It should be
required reading for all Java developers.  It is that good.

Bruce

[1] Effective Java Second Edition, Joshua Bloch
[2] Effective Java Item 17: Design and document for inheritance or else
prohibit it.
[3] Effective Java Item 16: Favor composition over inheritance

Endre Stølsvik wrote:

> I've also noticed the aspect the OP points out. Although I understand
> that it doesn't really make a problem until you have errors in your code
> (exceptions thrown), or you have code that "uses" this escaping in some way.
>
> BUT: *Don't make anything private - make it protected.* (and don't use
> final either).
>
> There is way to much privateness in GL already. I've been in need to
> override some small aspects of GL now and then (primarily with the
> resource handling (dispose), which I am not happy with) - and I've
> invariably have had to copy the entire class. And if the required
> methods aren't private - because they are a public part of the API - the
> class is instead final!
>
> I sense the "But these are implementation details that we do not want to
> expose!!!!" and "That is not a part of the API that we will have to
> maintain!!!!" arguments building up, so I'll just dryly point out:
> Either I can extend these classes, or I have to copy them pretty much
> verbatim. Which of those strategies is "most exposed"? If you try to
> lock things down even more, I'll just pretty much have to "fork" the
> entire project into my own codebase. And again - which is the worst of
> these two approaches?
>
> IMHO, The API is the public facing methods. That the /implementations/
> are open for extension (non-final classes and protected methods instead
> of private) doesn't make it into a part of the API as such. So feel free
> to ruin my extensions for every version of GL issued. ( - and it
> wouldn't hurt sticking in some extra "hooks" either - like you already
> had with CollectionList.createChildElementForList(..), which of course
> is private).
>
> Endre.
>
> On Mon, Sep 7, 2009 at 17:51, Bruce Alspaugh <compulinkltd@...
> <mailto:compulinkltd@...>> wrote:
>
>     I was reading a copy of Java Concurrency in Practice[1], and they
>     point out there are certain things you should never do inside a
>     constructor like call a method that could be overridden, or start a
>     thread because it can allow the this reference to escape from an
>     object before it is completely constructed.  I'm concerned that GL may
>     be doing something else they say you should never do in a constructor:
>      publish an inner class instance.  The example they give looks like
>     this:
>
>     public class ThisEscape {
>        public ThisEscape(EventSource source) {
>            source.registerListener(
>                new EventListener() {
>                    public void onEvent(Event e) {
>                        doSomething(e);
>                    }
>                });
>        }
>     }
>
>     When ThisEscape publishes the EventListener, it implicitly publishes
>     the enclosing ThisEscape instance as well, because inner class
>     instances contain a hidden reference to the enclosing instance.  The
>     problem is that the event source has access to the "this" reference
>     before ThisEscape has been fully constructed.  They say it is a
>     problem even if the publication is the last statement in the
>     constructor.
>
>     The workaround they recommend is to use a static factory method. It's
>     OK to create a listener or a thread in a constructor, just don't
>     publish or start it.
>
>     public class SafeListener {
>        private final EventListener listener;
>
>        private SafeListener() {
>            listener = new EventListener() {
>                public void onEvent(Event e) {
>                   doSomething(e);
>                }
>            };
>        }
>
>        public static SafeListener newInstance(EventSource source) {
>            SafeListener safe = new SafeListener();
>            source.registerListener(safe.listener);
>            return safe;
>        }
>     }
>
>     When I looked at the Javadoc describing the constructors for classes
>     like ObservableElementList, I was concerned that might be happening.
>     When I looked at the source for ObservableElementList and saw lines
>     like the ones below in the constructor, I became even more concerned:
>
>     public ObservableElementList(EventList<E> source, Connector<? super E>
>     elementConnector) {
>        this.elementConnector = elementConnector;
>
>        // attach this list to the element connector so the listeners know
>        // which List to notify of their modifications
>       this.elementConnector.setObservableElementList(this);
>
>       // for speed, we add all source elements together, rather than
>     individually
>        this.observedElements = new ArrayList<E>(source);
>
>        // we initialize the single EventListener registry, as we
>     optimistically
>        // assume we'll be using a single listener for all observed elements
>        this.singleEventListenerRegistry = new Barcode();
>        this.singleEventListenerRegistry.addWhite(0, source.size());
>
>        // add listeners to all source list elements
>        for (int i = 0, n = size(); i < n; i++) {
>            // connect a listener to the element
>            final EventListener listener = this.connectElement(get(i));
>
>            // record the listener in the registry
>            this.registerListener(i, listener, false);
>        }
>
>        // begin listening to the source list
>        source.addListEventListener(this);
>     }
>
>     The elementConnector sees the "this" reference before the
>     ObservableElementList is fully constructed.  Later, every list element
>     sees the "this" reference when the listener is registered on each
>     element.  Finally, the source list sees the "this" reference in the
>     last line of the constructor.
>
>     I'm concerned this idiom exists throughout GL, and it may be difficult
>     to fix without API changes. To emply the suggested workaround would
>     require making the constructors private and using factory methods
>     instead.  Maybe the GL folks already know about this?
>
>     Bruce
>
>     [1] Java Concurrency in Practice, Brian Goetz, Tim Peierls, Joshua
>     Bloch, Joseph Bowbeer, David Holmes, and Doug Lea Chapter 3.2.1 Safe
>     Construction Practices, pg. 41-42
>
>     ---------------------------------------------------------------------
>     To unsubscribe, e-mail: dev-unsubscribe@...
>     <mailto:dev-unsubscribe@...>
>     For additional commands, e-mail: dev-help@...
>     <mailto:dev-help@...>
>
>

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@...
For additional commands, e-mail: dev-help@...

[Endre Stølsvik-8]

[I of course have both Effective Java and JCIP, as I guess we all do!]

I think you might have lost my point: I specifically, twice even, stated that I don't want this to be a part of "the public API", and that the coders of GL would be totally free to completely trash my code on every revision-release when it comes to places where I've used inheritance. Document this decision in the javadoc, possibly of both class and the protected methods, and one would be good.

Problem is that composition can't fix the problem I have as GL stands. All of GL would have to be rewritten in that case. So, given these final classes and private methods, I'm stuck with copying the entire class to fix (add) one damn line of code. Lovely. Thus, your "high-flying argument" about composition vs. inheritance doesn't help me jack.

Also, the prohibition (finalness and private) isn't consitent, and one typically extend or tailor GL by extending AbstractEventList or TransformedEventList. So the point isn't valid at all, IMHO, at least as the codebase stands. Thus, opening up would make the situation better, not worse. On a rewrite, or on breaching major version, one could implement a composition logic throughout, and make everything final.

Two situations: CollectionList takes a Model (good - composition), but the decision about which implementation of ChildElement to use is a private method (as is the the interface, and the two inner implementation classes), even though the class is NOT final. Thus, to implement "recursing dispose", I had to copy the entire thing. For UniqueList, a simple extension would actually have fixed my problem - but here the class is final.

Generally, on these situations, I personally feel that javadoccing the intention of inheritance not being a "supported means of using GL" is better than just messing me over by making the class final, and methods private. See, problem is that one can't envision all the usage scenarios ones user have - maybe they even come up with something that really should be turned into a proper part of the API - while by making a class final, you pretty much state "I am just too intelligent for you, and my decision here is .. final".

.. and GlazedLists isn't in java.util quite yet, either.

Kind regards,

Endre.

On Mon, Sep 7, 2009 at 22:04, Bruce Alspaugh <compulinkltd@...> wrote:

I agree that the dispose methods are a real problem. See my post:

http://www.nabble.com/WeakReferenceProxy-td25299875.html

However, I have to defend the GL designers when they followed best practices in places where they prohibited inheritance. The central problem is that extending a class breaks encapsulation.

Good API design is hard enough.  As Joshua Bloch points out[1], it is very difficult to design a class that can be safely extended because of all the problems you can run into[2].  Unlike C++, Java is a single inheritance language which significantly limits what you can do with inheritance.  Instead of extending a class, I recommend to use composition whenever possible[3]. I like his quote, "If a class implements some interface that captures its essence, such as Set, List, or Map, then you should feel no compunction about prohibiting subclassing.  The wrapper class pattern, described in Item 16, provides a superior alternative to inheritance for augmenting the functionality." pg. 91. For GL, EventList is that interface.

By the way, "Effective Java" is one of the best Java books I have ever read.  Even James Gosling himself speaks highly of it.  It should be required reading for all Java developers.  It is that good.

Bruce

[1] Effective Java Second Edition, Joshua Bloch
[2] Effective Java Item 17: Design and document for inheritance or else prohibit it.
[3] Effective Java Item 16: Favor composition over inheritance

Endre Stølsvik wrote:

I've also noticed the aspect the OP points out. Although I understand that it doesn't really make a problem until you have errors in your code (exceptions thrown), or you have code that "uses" this escaping in some way.

BUT: *Don't make anything private - make it protected.* (and don't use final either).

There is way to much privateness in GL already. I've been in need to override some small aspects of GL now and then (primarily with the resource handling (dispose), which I am not happy with) - and I've invariably have had to copy the entire class. And if the required methods aren't private - because they are a public part of the API - the class is instead final!

I sense the "But these are implementation details that we do not want to expose!!!!" and "That is not a part of the API that we will have to maintain!!!!" arguments building up, so I'll just dryly point out: Either I can extend these classes, or I have to copy them pretty much verbatim. Which of those strategies is "most exposed"? If you try to lock things down even more, I'll just pretty much have to "fork" the entire project into my own codebase. And again - which is the worst of these two approaches?

IMHO, The API is the public facing methods. That the /implementations/ are open for extension (non-final classes and protected methods instead of private) doesn't make it into a part of the API as such. So feel free to ruin my extensions for every version of GL issued. ( - and it wouldn't hurt sticking in some extra "hooks" either - like you already had with CollectionList.createChildElementForList(..), which of course is private).

Endre.

On Mon, Sep 7, 2009 at 17:51, Bruce Alspaugh <compulinkltd@... <mailto:compulinkltd@...>> wrote:

   I was reading a copy of Java Concurrency in Practice[1], and they
   point out there are certain things you should never do inside a
   constructor like call a method that could be overridden, or start a
   thread because it can allow the this reference to escape from an
   object before it is completely constructed.  I'm concerned that GL may
   be doing something else they say you should never do in a constructor:
    publish an inner class instance.  The example they give looks like
   this:

   public class ThisEscape {
      public ThisEscape(EventSource source) {
          source.registerListener(
              new EventListener() {
                  public void onEvent(Event e) {
                      doSomething(e);
                  }
              });
      }
   }

   When ThisEscape publishes the EventListener, it implicitly publishes
   the enclosing ThisEscape instance as well, because inner class
   instances contain a hidden reference to the enclosing instance.  The
   problem is that the event source has access to the "this" reference
   before ThisEscape has been fully constructed.  They say it is a
   problem even if the publication is the last statement in the
   constructor.

   The workaround they recommend is to use a static factory method. It's
   OK to create a listener or a thread in a constructor, just don't
   publish or start it.

   public class SafeListener {
      private final EventListener listener;

      private SafeListener() {
          listener = new EventListener() {
              public void onEvent(Event e) {
                 doSomething(e);
              }
          };
      }

      public static SafeListener newInstance(EventSource source) {
          SafeListener safe = new SafeListener();
          source.registerListener(safe.listener);
          return safe;
      }
   }

   When I looked at the Javadoc describing the constructors for classes
   like ObservableElementList, I was concerned that might be happening.
   When I looked at the source for ObservableElementList and saw lines
   like the ones below in the constructor, I became even more concerned:

   public ObservableElementList(EventList<E> source, Connector<? super E>
   elementConnector) {
      this.elementConnector = elementConnector;

      // attach this list to the element connector so the listeners know
      // which List to notify of their modifications
     this.elementConnector.setObservableElementList(this);

     // for speed, we add all source elements together, rather than
   individually
      this.observedElements = new ArrayList<E>(source);

      // we initialize the single EventListener registry, as we
   optimistically
      // assume we'll be using a single listener for all observed elements
      this.singleEventListenerRegistry = new Barcode();
      this.singleEventListenerRegistry.addWhite(0, source.size());

      // add listeners to all source list elements
      for (int i = 0, n = size(); i < n; i++) {
          // connect a listener to the element
          final EventListener listener = this.connectElement(get(i));

          // record the listener in the registry
          this.registerListener(i, listener, false);
      }

      // begin listening to the source list
      source.addListEventListener(this);
   }

   The elementConnector sees the "this" reference before the
   ObservableElementList is fully constructed.  Later, every list element
   sees the "this" reference when the listener is registered on each
   element.  Finally, the source list sees the "this" reference in the
   last line of the constructor.

   I'm concerned this idiom exists throughout GL, and it may be difficult
   to fix without API changes. To emply the suggested workaround would
   require making the constructors private and using factory methods
   instead.  Maybe the GL folks already know about this?

   Bruce

   [1] Java Concurrency in Practice, Brian Goetz, Tim Peierls, Joshua
   Bloch, Joseph Bowbeer, David Holmes, and Doug Lea Chapter 3.2.1 Safe
   Construction Practices, pg. 41-42

   ---------------------------------------------------------------------
   To unsubscribe, e-mail: dev-unsubscribe@...

   <mailto:dev-unsubscribe@...>

   For additional commands, e-mail: dev-help@...

   <mailto:dev-help@...>

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@...
For additional commands, e-mail: dev-help@...