CompositeList constructors (CVS version)

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

CompositeList constructors (CVS version)

Holger
Hey guys,

a question about the CompositeList constructors:

...
public CompositeList() {
  super(new BasicEventList<EventList<E>>(), (Model)GlazedLists.listCollectionListModel());
}

public CompositeList(ReadWriteLock lock) {
  this();
  readWriteLock = lock;
}
...

The second constructor gets a lock object, that is used
by the CompositeList, but *not* by its source list.
The source list is constructed in the default constructor with
new BasicEventList<EventList<E>>(), so it uses its own lock.

Is this intentional or a bug?

I thought a TransformedList should reuse the lock object from
the source list.

Thanks,
Holger

______________________________________________________________
Verschicken Sie romantische, coole und witzige Bilder per SMS!
Jetzt bei WEB.DE FreeMail: http://f.web.de/?mc=021193

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: CompositeList constructors (CVS version)

Jesse Wilson
Holger ---

You've found a bug, good catch.

Generally I think the way we were doing CompositeList
in the beginning was flawed. The only safe EventList
to add to a CompositeList is one you've created with
the CompositeList's factory method createMemberList().

This is because it's necessary for CompositeLists and
their elements to share both the locks and the publisher.
The publisher is a hard problem, and finally with the new
sequencedependencies I've solved a long lived bug in
our bugtracker:
   https://glazedlists.dev.java.net/issues/show_bug.cgi?id=96

What I need to do is go ahead and deprecate the methods
that allow you to create 'unsafe' CompositeLists. In particular,
this is everything bug:
   CompositeList.createMemberList()
   CompositeList() plain constructor (no args)
   ... and addMemberList() should warn if the locks or publisher differ.
    How to do this warning? I'm not sure. Throwing an exception seems
    rather harsh, system out is off limits. We might need to add a new
    method and deperecate the old one that doesn't warn propertly?

Let me know your thoughts!

Cheers,
Jesse


On 7/28/06, Holger Brands <[hidden email]> wrote:

> Hey guys,
>
> a question about the CompositeList constructors:
>
> ...
> public CompositeList() {
>   super(new BasicEventList<EventList<E>>(), (Model)GlazedLists.listCollectionListModel());
> }
>
> public CompositeList(ReadWriteLock lock) {
>   this();
>   readWriteLock = lock;
> }
> ...
>
> The second constructor gets a lock object, that is used
> by the CompositeList, but *not* by its source list.
> The source list is constructed in the default constructor with
> new BasicEventList<EventList<E>>(), so it uses its own lock.
>
> Is this intentional or a bug?
>
> I thought a TransformedList should reuse the lock object from
> the source list.
>
> Thanks,
> Holger
>
> ______________________________________________________________
> Verschicken Sie romantische, coole und witzige Bilder per SMS!
> Jetzt bei WEB.DE FreeMail: http://f.web.de/?mc=021193
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: Re: CompositeList constructors (CVS version)

Jesse Wilson
Update - I think this is a bug, but I don't think
there are any negative consequences possible.
There's no way to get the read/write lock of the
internal source list, so it doesn't really matter
which lock it is. It could probably be null!

So it's a bug that can't be exercised, ie. you
won't be able to write a test case that demonstrates
it. That's good, but if you'd like to fix it, please feel
free!

Cheers,
Jesse

On 7/28/06, Jesse Wilson <[hidden email]> wrote:

> Holger ---
>
> You've found a bug, good catch.
>
> Generally I think the way we were doing CompositeList
> in the beginning was flawed. The only safe EventList
> to add to a CompositeList is one you've created with
> the CompositeList's factory method createMemberList().
>
> This is because it's necessary for CompositeLists and
> their elements to share both the locks and the publisher.
> The publisher is a hard problem, and finally with the new
> sequencedependencies I've solved a long lived bug in
> our bugtracker:
>    https://glazedlists.dev.java.net/issues/show_bug.cgi?id=96
>
> What I need to do is go ahead and deprecate the methods
> that allow you to create 'unsafe' CompositeLists. In particular,
> this is everything bug:
>    CompositeList.createMemberList()
>    CompositeList() plain constructor (no args)
>    ... and addMemberList() should warn if the locks or publisher differ.
>     How to do this warning? I'm not sure. Throwing an exception seems
>     rather harsh, system out is off limits. We might need to add a new
>     method and deperecate the old one that doesn't warn propertly?
>
> Let me know your thoughts!
>
> Cheers,
> Jesse
>
>
> On 7/28/06, Holger Brands <[hidden email]> wrote:
> > Hey guys,
> >
> > a question about the CompositeList constructors:
> >
> > ...
> > public CompositeList() {
> >   super(new BasicEventList<EventList<E>>(), (Model)GlazedLists.listCollectionListModel());
> > }
> >
> > public CompositeList(ReadWriteLock lock) {
> >   this();
> >   readWriteLock = lock;
> > }
> > ...
> >
> > The second constructor gets a lock object, that is used
> > by the CompositeList, but *not* by its source list.
> > The source list is constructed in the default constructor with
> > new BasicEventList<EventList<E>>(), so it uses its own lock.
> >
> > Is this intentional or a bug?
> >
> > I thought a TransformedList should reuse the lock object from
> > the source list.
> >
> > Thanks,
> > Holger
> >
> > ______________________________________________________________
> > Verschicken Sie romantische, coole und witzige Bilder per SMS!
> > Jetzt bei WEB.DE FreeMail: http://f.web.de/?mc=021193
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: [hidden email]
> > For additional commands, e-mail: [hidden email]
> >
> >
>

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: CompositeList constructors (CVS version)

Holger
In reply to this post by Holger
Hi Jesse,

what about the following implementation:

public CompositeList() {
   super(new BasicEventList<EventList<E>>(), (Model)GlazedLists.listCollectionListModel());
}

public CompositeList(ReadWriteLock lock) {
   super(new BasicEventList<EventList<E>>(lock), (Model)GlazedLists.listCollectionListModel());
}

Note the difference in the second constructor where the lock is also passed to the
source list.
Basically, we need the second constructor, because our member lists are created before the
CompositeList comes into existence. All member lists are "in the same list pipeline", so they
share the same lock and publisher.

Nevertheless, I think it's a good idea to check the lock and publisher in addMemberList(), at least
with an assertion or so.

Holger

> Holger ---
>
> You've found a bug, good catch.
>
> Generally I think the way we were doing CompositeList
> in the beginning was flawed. The only safe EventList
> to add to a CompositeList is one you've created with
> the CompositeList's factory method createMemberList().
>
> This is because it's necessary for CompositeLists and
> their elements to share both the locks and the publisher.
> The publisher is a hard problem, and finally with the new
> sequencedependencies I've solved a long lived bug in
> our bugtracker:
>    https://glazedlists.dev.java.net/issues/show_bug.cgi?id=96
>
> What I need to do is go ahead and deprecate the methods
> that allow you to create 'unsafe' CompositeLists. In particular,
> this is everything bug:
>    CompositeList.createMemberList()
>    CompositeList() plain constructor (no args)
>    ... and addMemberList() should warn if the locks or publisher differ.
>     How to do this warning? I'm not sure. Throwing an exception seems
>     rather harsh, system out is off limits. We might need to add a new
>     method and deperecate the old one that doesn't warn propertly?
>
> Let me know your thoughts!
>
> Cheers,
> Jesse
>
>
> On 7/28/06, Holger Brands <[hidden email]> wrote:
> > Hey guys,
> >
> > a question about the CompositeList constructors:
> >
> > ...
> > public CompositeList() {
> >   super(new BasicEventList<EventList<E>>(), (Model)GlazedLists.listCollectionListModel());
> > }
> >
> > public CompositeList(ReadWriteLock lock) {
> >   this();
> >   readWriteLock = lock;
> > }
> > ...
> >
> > The second constructor gets a lock object, that is used
> > by the CompositeList, but *not* by its source list.
> > The source list is constructed in the default constructor with
> > new BasicEventList<EventList<E>>(), so it uses its own lock.
> >
> > Is this intentional or a bug?
> >
> > I thought a TransformedList should reuse the lock object from
> > the source list.
> >
> > Thanks,
> > Holger
> >
> > ______________________________________________________________
> > Verschicken Sie romantische, coole und witzige Bilder per SMS!
> > Jetzt bei WEB.DE FreeMail: http://f.web.de/?mc=021193
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: [hidden email]
> > For additional commands, e-mail: [hidden email]
> >
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>


_____________________________________________________________________
Der WEB.DE SmartSurfer hilft bis zu 70% Ihrer Onlinekosten zu sparen!
http://smartsurfer.web.de/?mc=100071&distributionid=000000000071

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: Re: CompositeList constructors (CVS version)

Holger
In reply to this post by Holger
Jesse,

I just saw your second reply after I responded to your first mail.
 
> So it's a bug that can't be exercised, ie. you
> won't be able to write a test case that demonstrates
> it. That's good, but if you'd like to fix it, please feel
> free!

What about potential subclasses of CompositeList?
They could access the protected 'source' member and do
something 'evil' with it.
But agreed, it's somewhat hypothetical...

Holger

>
> Cheers,
> Jesse
>
> On 7/28/06, Jesse Wilson <[hidden email]> wrote:
> > Holger ---
> >
> > You've found a bug, good catch.
> >
> > Generally I think the way we were doing CompositeList
> > in the beginning was flawed. The only safe EventList
> > to add to a CompositeList is one you've created with
> > the CompositeList's factory method createMemberList().
> >
> > This is because it's necessary for CompositeLists and
> > their elements to share both the locks and the publisher.
> > The publisher is a hard problem, and finally with the new
> > sequencedependencies I've solved a long lived bug in
> > our bugtracker:
> >    https://glazedlists.dev.java.net/issues/show_bug.cgi?id=96
> >
> > What I need to do is go ahead and deprecate the methods
> > that allow you to create 'unsafe' CompositeLists. In particular,
> > this is everything bug:
> >    CompositeList.createMemberList()
> >    CompositeList() plain constructor (no args)
> >    ... and addMemberList() should warn if the locks or publisher differ.
> >     How to do this warning? I'm not sure. Throwing an exception seems
> >     rather harsh, system out is off limits. We might need to add a new
> >     method and deperecate the old one that doesn't warn propertly?
> >
> > Let me know your thoughts!
> >
> > Cheers,
> > Jesse
> >
> >
> > On 7/28/06, Holger Brands <[hidden email]> wrote:
> > > Hey guys,
> > >
> > > a question about the CompositeList constructors:
> > >
> > > ...
> > > public CompositeList() {
> > >   super(new BasicEventList<EventList<E>>(), (Model)GlazedLists.listCollectionListModel());
> > > }
> > >
> > > public CompositeList(ReadWriteLock lock) {
> > >   this();
> > >   readWriteLock = lock;
> > > }
> > > ...
> > >
> > > The second constructor gets a lock object, that is used
> > > by the CompositeList, but *not* by its source list.
> > > The source list is constructed in the default constructor with
> > > new BasicEventList<EventList<E>>(), so it uses its own lock.
> > >
> > > Is this intentional or a bug?
> > >
> > > I thought a TransformedList should reuse the lock object from
> > > the source list.
> > >
> > > Thanks,
> > > Holger
> > >
> > > ______________________________________________________________
> > > Verschicken Sie romantische, coole und witzige Bilder per SMS!
> > > Jetzt bei WEB.DE FreeMail: http://f.web.de/?mc=021193
> > >
> > > ---------------------------------------------------------------------
> > > To unsubscribe, e-mail: [hidden email]
> > > For additional commands, e-mail: [hidden email]
> > >
> > >
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>


______________________________________________________________________
XXL-Speicher, PC-Virenschutz, Spartarife & mehr: Nur im WEB.DE Club!
Jetzt gratis testen! http://freemail.web.de/home/landingpad/?mc=021130

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: CompositeList constructors (CVS version)

James Lemieux
In reply to this post by Holger
Holger beat me to it! As I was beginning my reply his post came in with the solution!

   Holger is, as per usual, spot on with his analysis. Specifically, I added the second (currently broken) constructor for his benefit.

   The solution I was going to propose is exactly the one Holger outlined, so obviously I support it, though I agree that there is no way to exploit the lock inconsistency, as Jesse said.

   It may be heavy-handed and cause "working code to break", but throwing an IllegalArgumentException from addMemberList(...) seems like the best approach right now. It's too easy to violate the constraint, especially in Threaded cases where it matters.

   In the cases where you cannot use createMemberList *after* the CompositeList has been constructed, it is difficult to get the lock / publisher correct. That is the toughest case to handle...

James

On 7/28/06, Holger Brands <[hidden email]> wrote:
Hi Jesse,

what about the following implementation:

public CompositeList() {
   super(new BasicEventList<EventList<E>>(), (Model)GlazedLists.listCollectionListModel());
}

public CompositeList(ReadWriteLock lock) {
   super(new BasicEventList<EventList<E>>(lock), (Model)GlazedLists.listCollectionListModel());
}

Note the difference in the second constructor where the lock is also passed to the
source list.
Basically, we need the second constructor, because our member lists are created before the
CompositeList comes into existence. All member lists are "in the same list pipeline", so they
share the same lock and publisher.

Nevertheless, I think it's a good idea to check the lock and publisher in addMemberList(), at least
with an assertion or so.

Holger

> Holger ---
>
> You've found a bug, good catch.
>

> Generally I think the way we were doing CompositeList
> in the beginning was flawed. The only safe EventList
> to add to a CompositeList is one you've created with
> the CompositeList's factory method createMemberList().
>
> This is because it's necessary for CompositeLists and
> their elements to share both the locks and the publisher.
> The publisher is a hard problem, and finally with the new
> sequencedependencies I've solved a long lived bug in
> our bugtracker:
>    https://glazedlists.dev.java.net/issues/show_bug.cgi?id=96
>
> What I need to do is go ahead and deprecate the methods
> that allow you to create 'unsafe' CompositeLists. In particular,
> this is everything bug:
>    CompositeList.createMemberList()
>    CompositeList() plain constructor (no args)
>    ... and addMemberList() should warn if the locks or publisher differ.
>     How to do this warning? I'm not sure. Throwing an exception seems
>     rather harsh, system out is off limits. We might need to add a new
>     method and deperecate the old one that doesn't warn propertly?
>
> Let me know your thoughts!
>
> Cheers,
> Jesse
>
>
> On 7/28/06, Holger Brands <[hidden email]> wrote:
> > Hey guys,
> >

> > a question about the CompositeList constructors:
> >
> > ...
> > public CompositeList() {
> >   super(new BasicEventList<EventList<E>>(), (Model)GlazedLists.listCollectionListModel());
> > }
> >
> > public CompositeList(ReadWriteLock lock) {
> >   this();
> >   readWriteLock = lock;
> > }
> > ...
> >
> > The second constructor gets a lock object, that is used
> > by the CompositeList, but *not* by its source list.
> > The source list is constructed in the default constructor with
> > new BasicEventList<EventList<E>>(), so it uses its own lock.
> >
> > Is this intentional or a bug?
> >
> > I thought a TransformedList should reuse the lock object from
> > the source list.
> >
> > Thanks,
> > Holger
> >
> > ______________________________________________________________
> > Verschicken Sie romantische, coole und witzige Bilder per SMS!
> > Jetzt bei WEB.DE FreeMail: http://f.web.de/?mc=021193
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: [hidden email]
> > For additional commands, e-mail: [hidden email]
> >
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>


_____________________________________________________________________
Der WEB.DE SmartSurfer hilft bis zu 70% Ihrer Onlinekosten zu sparen!
http://smartsurfer.web.de/?mc=100071&distributionid=000000000071

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]


Reply | Threaded
Open this post in threaded view
|

Re: CompositeList constructors (CVS version)

Holger
In reply to this post by Holger
Ok, if you agree I will

- enter a bug for this
- fix the bug with the proposed solution
- add checks to addMemberList(...) throwing IllegalArgumentExceptions
- add some tests for the changes.


BTW,
the next two weeks I'm going on summer holiday.
So I'm probably a few days offline in that time.

Holger

>Holger beat me to it! As I was beginning my reply his post came in with the solution!
>
>    Holger is, as per usual, spot on with his analysis. Specifically, I added the second (currently broken) constructor for his benefit.
>
>
>    The solution I was going to propose is exactly the one Holger outlined, so obviously I support it, though I agree that there is no way to exploit the lock inconsistency, as Jesse said.
>
>    It may be heavy-handed and cause "working code to break", but throwing an IllegalArgumentException from addMemberList(...) seems like the best approach right now. It's too easy to violate the constraint, especially in Threaded cases where it matters.
>
>
>    In the cases where you cannot use createMemberList *after* the CompositeList has been constructed, it is difficult to get the lock / publisher correct. That is the toughest case to handle...
>
> James
>
>
> On 7/28/06, Holger Brands <[hidden email]> wrote:
> Hi Jesse,
>
> what about the following implementation:
>
> public CompositeList() {
>    super(new BasicEventList<EventList<E>>(), (Model)GlazedLists.listCollectionListModel());
> }
>
> public CompositeList(ReadWriteLock lock) {
>
>    super(new BasicEventList<EventList<E>>(lock), (Model)GlazedLists.listCollectionListModel());
> }
>
> Note the difference in the second constructor where the lock is also passed to the
> source list.
>
> Basically, we need the second constructor, because our member lists are created before the
> CompositeList comes into existence. All member lists are "in the same list pipeline", so they
> share the same lock and publisher.
>
>
> Nevertheless, I think it's a good idea to check the lock and publisher in addMemberList(), at least
> with an assertion or so.
>
> Holger
>
> > Holger ---
> >
> > You've found a bug, good catch.
> >
> > Generally I think the way we were doing CompositeList
> > in the beginning was flawed. The only safe EventList
> > to add to a CompositeList is one you've created with
> > the CompositeList's factory method createMemberList().
>
> >
> > This is because it's necessary for CompositeLists and
> > their elements to share both the locks and the publisher.
> > The publisher is a hard problem, and finally with the new
> > sequencedependencies I've solved a long lived bug in
>
> > our bugtracker:
> >    https://glazedlists.dev.java.net/issues/show_bug.cgi?id=96
> >
> > What I need to do is go ahead and deprecate the methods
>
> > that allow you to create 'unsafe' CompositeLists. In particular,
> > this is everything bug:
> >    CompositeList.createMemberList()
> >    CompositeList() plain constructor (no args)
> >    ... and addMemberList() should warn if the locks or publisher differ.
>
> >     How to do this warning? I'm not sure. Throwing an exception seems
> >     rather harsh, system out is off limits. We might need to add a new
> >     method and deperecate the old one that doesn't warn propertly?
>
> >
> > Let me know your thoughts!
> >
> > Cheers,
> > Jesse
> >
> >
> > On 7/28/06, Holger Brands <[hidden email]> wrote:
> > > Hey guys,
> > >
> > > a question about the CompositeList constructors:
> > >
> > > ...
> > > public CompositeList() {
> > >   super(new BasicEventList<EventList<E>>(), (Model)GlazedLists.listCollectionListModel());
>
> > > }
> > >
> > > public CompositeList(ReadWriteLock lock) {
> > >   this();
> > >   readWriteLock = lock;
> > > }
> > > ...
> > >
> > > The second constructor gets a lock object, that is used
>
> > > by the CompositeList, but *not* by its source list.
> > > The source list is constructed in the default constructor with
> > > new BasicEventList<EventList<E>>(), so it uses its own lock.
>
> > >
> > > Is this intentional or a bug?
> > >
> > > I thought a TransformedList should reuse the lock object from
> > > the source list.
> > >
> > > Thanks,
> > > Holger
>
> > >
> > > ______________________________________________________________
> > > Verschicken Sie romantische, coole und witzige Bilder per SMS!
> > > Jetzt bei WEB.DE
>  FreeMail: http://f.web.de/?mc=021193
> > >
> > > ---------------------------------------------------------------------
> > > To unsubscribe, e-mail:
> [hidden email]
> > > For additional commands, e-mail: [hidden email]
> > >
> > >
> >
> > ---------------------------------------------------------------------
>
> > To unsubscribe, e-mail: [hidden email]
> > For additional commands, e-mail:
> [hidden email]
> >
>
>
> _____________________________________________________________________
> Der WEB.DE SmartSurfer hilft bis zu 70% Ihrer Onlinekosten zu sparen!
>
> http://smartsurfer.web.de/?mc=100071&distributionid=000000000071
>
> ---------------------------------------------------------------------
>
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>
>
>


______________________________________________________________
Verschicken Sie romantische, coole und witzige Bilder per SMS!
Jetzt bei WEB.DE FreeMail: http://f.web.de/?mc=021193

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: CompositeList constructors (CVS version)

Holger
In reply to this post by Holger
Jesse,

> Ok, if you agree I will
>
> - enter a bug for this
> - fix the bug with the proposed solution
> - add checks to addMemberList(...) throwing IllegalArgumentExceptions
> - add some tests for the changes.
>

Is this ok for you?

When creating member lists before the CompositeList,
there must be a way to share not only the lock but also
the publisher with CompositeList, right?

So, should I change the signature of the existing constructor

public CompositeList(ReadWriteLock lock)

or add a new constructor

public CompositeList(ReadWriteLock lock, ListEventPublisher publisher)

The latter wouldn't break existing code. But on the other hand,
the constructor with the lock object alone does not make sense anymore...

Should code changes that break existing code be deferred to a major release (2.0)?

Thanks,
Holger

>
> BTW,
> the next two weeks I'm going on summer holiday.
> So I'm probably a few days offline in that time.
>
> Holger
>
> >Holger beat me to it! As I was beginning my reply his post came in with the solution!
> >
> >    Holger is, as per usual, spot on with his analysis. Specifically, I added the second (currently broken) constructor for his benefit.
> >
> >
> >    The solution I was going to propose is exactly the one Holger outlined, so obviously I support it, though I agree that there is no way to exploit the lock inconsistency, as Jesse said.
> >
> >    It may be heavy-handed and cause "working code to break", but throwing an IllegalArgumentException from addMemberList(...) seems like the best approach right now. It's too easy to violate the constraint, especially in Threaded cases where it matters.
> >
> >
> >    In the cases where you cannot use createMemberList *after* the CompositeList has been constructed, it is difficult to get the lock / publisher correct. That is the toughest case to handle...
> >
> > James
> >
> >
> > On 7/28/06, Holger Brands <[hidden email]> wrote:
> > Hi Jesse,
> >
> > what about the following implementation:
> >
> > public CompositeList() {
> >    super(new BasicEventList<EventList<E>>(), (Model)GlazedLists.listCollectionListModel());
> > }
> >
> > public CompositeList(ReadWriteLock lock) {
> >
> >    super(new BasicEventList<EventList<E>>(lock), (Model)GlazedLists.listCollectionListModel());
> > }
> >
> > Note the difference in the second constructor where the lock is also passed to the
> > source list.
> >
> > Basically, we need the second constructor, because our member lists are created before the
> > CompositeList comes into existence. All member lists are "in the same list pipeline", so they
> > share the same lock and publisher.
> >
> >
> > Nevertheless, I think it's a good idea to check the lock and publisher in addMemberList(), at least
> > with an assertion or so.
> >
> > Holger
> >
> > > Holger ---
> > >
> > > You've found a bug, good catch.
> > >
> > > Generally I think the way we were doing CompositeList
> > > in the beginning was flawed. The only safe EventList
> > > to add to a CompositeList is one you've created with
> > > the CompositeList's factory method createMemberList().
> >
> > >
> > > This is because it's necessary for CompositeLists and
> > > their elements to share both the locks and the publisher.
> > > The publisher is a hard problem, and finally with the new
> > > sequencedependencies I've solved a long lived bug in
> >
> > > our bugtracker:
> > >    https://glazedlists.dev.java.net/issues/show_bug.cgi?id=96
> > >
> > > What I need to do is go ahead and deprecate the methods
> >
> > > that allow you to create 'unsafe' CompositeLists. In particular,
> > > this is everything bug:
> > >    CompositeList.createMemberList()
> > >    CompositeList() plain constructor (no args)
> > >    ... and addMemberList() should warn if the locks or publisher differ.
> >
> > >     How to do this warning? I'm not sure. Throwing an exception seems
> > >     rather harsh, system out is off limits. We might need to add a new
> > >     method and deperecate the old one that doesn't warn propertly?
> >
> > >
> > > Let me know your thoughts!
> > >
> > > Cheers,
> > > Jesse
> > >
> > >
> > > On 7/28/06, Holger Brands <[hidden email]> wrote:
> > > > Hey guys,
> > > >
> > > > a question about the CompositeList constructors:
> > > >
> > > > ...
> > > > public CompositeList() {
> > > >   super(new BasicEventList<EventList<E>>(), (Model)GlazedLists.listCollectionListModel());
> >
> > > > }
> > > >
> > > > public CompositeList(ReadWriteLock lock) {
> > > >   this();
> > > >   readWriteLock = lock;
> > > > }
> > > > ...
> > > >
> > > > The second constructor gets a lock object, that is used
> >
> > > > by the CompositeList, but *not* by its source list.
> > > > The source list is constructed in the default constructor with
> > > > new BasicEventList<EventList<E>>(), so it uses its own lock.
> >
> > > >
> > > > Is this intentional or a bug?
> > > >
> > > > I thought a TransformedList should reuse the lock object from
> > > > the source list.
> > > >
> > > > Thanks,
> > > > Holger
> >
> > > >
> > > > ______________________________________________________________
> > > > Verschicken Sie romantische, coole und witzige Bilder per SMS!
> > > > Jetzt bei WEB.DE
> >  FreeMail: http://f.web.de/?mc=021193
> > > >
> > > > ---------------------------------------------------------------------
> > > > To unsubscribe, e-mail:
> > [hidden email]
> > > > For additional commands, e-mail: [hidden email]
> > > >
> > > >
> > >
> > > ---------------------------------------------------------------------
> >
> > > To unsubscribe, e-mail: [hidden email]
> > > For additional commands, e-mail:
> > [hidden email]
> > >
> >
> >
> > _____________________________________________________________________
> > Der WEB.DE SmartSurfer hilft bis zu 70% Ihrer Onlinekosten zu sparen!
> >
> > http://smartsurfer.web.de/?mc=100071&distributionid=000000000071
> >
> > ---------------------------------------------------------------------
> >
> > To unsubscribe, e-mail: [hidden email]
> > For additional commands, e-mail: [hidden email]
> >
> >
> >
> >
>
>
> ______________________________________________________________
> Verschicken Sie romantische, coole und witzige Bilder per SMS!
> Jetzt bei WEB.DE FreeMail: http://f.web.de/?mc=021193
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>


______________________________________________________________
Verschicken Sie romantische, coole und witzige Bilder per SMS!
Jetzt bei WEB.DE FreeMail: http://f.web.de/?mc=021193

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: CompositeList constructors (CVS version)

James Lemieux
I vote for the "breaking change" simply because I'd be *highly* surprised if anyone is using that constructor except you, Holger. It's been introduced quite recently expressly for you. The fix is trivial if it does happen to break anyone else.

J

On 7/30/06, Holger Brands <[hidden email]> wrote:
Jesse,

> Ok, if you agree I will
>
> - enter a bug for this
> - fix the bug with the proposed solution
> - add checks to addMemberList(...) throwing IllegalArgumentExceptions
> - add some tests for the changes.
>

Is this ok for you?

When creating member lists before the CompositeList,
there must be a way to share not only the lock but also
the publisher with CompositeList, right?

So, should I change the signature of the existing constructor

public CompositeList(ReadWriteLock lock)

or add a new constructor

public CompositeList(ReadWriteLock lock, ListEventPublisher publisher)

The latter wouldn't break existing code. But on the other hand,
the constructor with the lock object alone does not make sense anymore...

Should code changes that break existing code be deferred to a major release (2.0)?

Thanks,
Holger

>
> BTW,
> the next two weeks I'm going on summer holiday.

> So I'm probably a few days offline in that time.
>
> Holger
>
> >Holger beat me to it! As I was beginning my reply his post came in with the solution!
> >
> >    Holger is, as per usual, spot on with his analysis. Specifically, I added the second (currently broken) constructor for his benefit.
> >
> >
> >    The solution I was going to propose is exactly the one Holger outlined, so obviously I support it, though I agree that there is no way to exploit the lock inconsistency, as Jesse said.
> >
> >    It may be heavy-handed and cause "working code to break", but throwing an IllegalArgumentException from addMemberList(...) seems like the best approach right now. It's too easy to violate the constraint, especially in Threaded cases where it matters.
> >
> >
> >    In the cases where you cannot use createMemberList *after* the CompositeList has been constructed, it is difficult to get the lock / publisher correct. That is the toughest case to handle...
> >
> > James
> >
> >
> > On 7/28/06, Holger Brands <[hidden email]> wrote:
> > Hi Jesse,
> >
> > what about the following implementation:
> >
> > public CompositeList() {
> >    super(new BasicEventList<EventList<E>>(), (Model)GlazedLists.listCollectionListModel());
> > }
> >
> > public CompositeList(ReadWriteLock lock) {
> >
> >    super(new BasicEventList<EventList<E>>(lock), (Model)GlazedLists.listCollectionListModel());
> > }
> >
> > Note the difference in the second constructor where the lock is also passed to the
> > source list.
> >
> > Basically, we need the second constructor, because our member lists are created before the
> > CompositeList comes into existence. All member lists are "in the same list pipeline", so they
> > share the same lock and publisher.
> >
> >
> > Nevertheless, I think it's a good idea to check the lock and publisher in addMemberList(), at least
> > with an assertion or so.
> >
> > Holger
> >
> > > Holger ---
> > >
> > > You've found a bug, good catch.
> > >
> > > Generally I think the way we were doing CompositeList
> > > in the beginning was flawed. The only safe EventList
> > > to add to a CompositeList is one you've created with
> > > the CompositeList's factory method createMemberList().
> >
> > >
> > > This is because it's necessary for CompositeLists and
> > > their elements to share both the locks and the publisher.
> > > The publisher is a hard problem, and finally with the new
> > > sequencedependencies I've solved a long lived bug in
> >
> > > our bugtracker:
> > >    https://glazedlists.dev.java.net/issues/show_bug.cgi?id=96
> > >
> > > What I need to do is go ahead and deprecate the methods
> >
> > > that allow you to create 'unsafe' CompositeLists. In particular,
> > > this is everything bug:
> > >    CompositeList.createMemberList()
> > >    CompositeList() plain constructor (no args)
> > >    ... and addMemberList() should warn if the locks or publisher differ.
> >
> > >     How to do this warning? I'm not sure. Throwing an exception seems
> > >     rather harsh, system out is off limits. We might need to add a new
> > >     method and deperecate the old one that doesn't warn propertly?
> >
> > >
> > > Let me know your thoughts!
> > >
> > > Cheers,
> > > Jesse
> > >
> > >
> > > On 7/28/06, Holger Brands < [hidden email]> wrote:
> > > > Hey guys,
> > > >
> > > > a question about the CompositeList constructors:
> > > >
> > > > ...
> > > > public CompositeList() {
> > > >   super(new BasicEventList<EventList<E>>(), (Model)GlazedLists.listCollectionListModel());
> >
> > > > }
> > > >
> > > > public CompositeList(ReadWriteLock lock) {
> > > >   this();
> > > >   readWriteLock = lock;
> > > > }
> > > > ...
> > > >
> > > > The second constructor gets a lock object, that is used
> >
> > > > by the CompositeList, but *not* by its source list.
> > > > The source list is constructed in the default constructor with
> > > > new BasicEventList<EventList<E>>(), so it uses its own lock.
> >
> > > >
> > > > Is this intentional or a bug?
> > > >
> > > > I thought a TransformedList should reuse the lock object from
> > > > the source list.
> > > >
> > > > Thanks,
> > > > Holger
> >
> > > >
> > > > ______________________________________________________________
> > > > Verschicken Sie romantische, coole und witzige Bilder per SMS!
> > > > Jetzt bei WEB.DE
> >  FreeMail: http://f.web.de/?mc=021193
> > > >
> > > > ---------------------------------------------------------------------
> > > > To unsubscribe, e-mail:
> > [hidden email]
> > > > For additional commands, e-mail: [hidden email]
> > > >
> > > >
> > >
> > > ---------------------------------------------------------------------
> >
> > > To unsubscribe, e-mail: [hidden email]
> > > For additional commands, e-mail:
> > [hidden email]
> > >
> >
> >
> > _____________________________________________________________________
> > Der WEB.DE SmartSurfer hilft bis zu 70% Ihrer Onlinekosten zu sparen!
> >
> > http://smartsurfer.web.de/?mc=100071&distributionid=000000000071
> >
> > ---------------------------------------------------------------------
> >
> > To unsubscribe, e-mail: [hidden email]
> > For additional commands, e-mail: [hidden email]
> >
> >
> >
> >
>
>
> ______________________________________________________________
> Verschicken Sie romantische, coole und witzige Bilder per SMS!
> Jetzt bei WEB.DE FreeMail: http://f.web.de/?mc=021193
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>


______________________________________________________________
Verschicken Sie romantische, coole und witzige Bilder per SMS!
Jetzt bei WEB.DE FreeMail: http://f.web.de/?mc=021193

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]


Reply | Threaded
Open this post in threaded view
|

Re: Re: CompositeList constructors (CVS version)

Jesse Wilson
Hey guys ---

I think we firstly we should recommend that
everyone use the createMemberList() method
whenever they create EventLists that might
make their way into a CompositeList.

> > > - enter a bug for this
sure!

> > > - fix the bug with the proposed solution
sure!

> > > - add checks to addMemberList(...) throwing IllegalArgumentExceptions
This breaks compatibility, but in an obvious clean
way where a developer will recognize the problem
immediately. I think it's okay, but we'll have to make
a note of it in our release notes for our next rev.
This is a constraint that we had always intended but
never enforced. As long as we are loud about announcing
it, I don't think this will be much of a problem.

In general, I'm very serious about not breaking compatibility.
If ever we break compatibility, we must be very loud
about it and very justified. We must always provide a
mechanical migration path for our users. Otherwise they'll
hate us.

> > > - add some tests for the changes.
Sure.

> > Is this ok for you?
You bet. Feel free to improvise as necessary.

> > When creating member lists before the CompositeList,
> > there must be a way to share not only the lock but also
> > the publisher with CompositeList, right?
You got it.

> > So, should I change the signature of the existing constructor
> > public CompositeList(ReadWriteLock lock)
> > or add a new constructor
> > public CompositeList(ReadWriteLock lock, ListEventPublisher publisher)
Add the new constructor, deprecate the old one with
a brief description of why. We can strip out the deprecated
stuff at some point in the distant future.

> > Should code changes that break existing code be deferred to a major
> release (2.0)?
Probably. But in this case as long as we're loud and
the signatures don't change, I think we'll be okay.

> > > BTW,
> > > the next two weeks I'm going on summer holiday.
> > > So I'm probably a few days offline in that time.

Happy travels! That reminds me - I don't know much about you
Holger - what you do, what your favourite color is, your age... feel free
to provide a brief introductory bio of yourself if you'd like.

Cheers,
Jesse

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: Re: CompositeList constructors (CVS version)

Holger
In reply to this post by Holger
Ok, I've fixed the issue, see

https://glazedlists.dev.java.net/issues/show_bug.cgi?id=356

Holger

> Hey guys ---
>
> I think we firstly we should recommend that
> everyone use the createMemberList() method
> whenever they create EventLists that might
> make their way into a CompositeList.
>
> > > > - enter a bug for this
> sure!
>
> > > > - fix the bug with the proposed solution
> sure!
>
> > > > - add checks to addMemberList(...) throwing IllegalArgumentExceptions
> This breaks compatibility, but in an obvious clean
> way where a developer will recognize the problem
> immediately. I think it's okay, but we'll have to make
> a note of it in our release notes for our next rev.
> This is a constraint that we had always intended but
> never enforced. As long as we are loud about announcing
> it, I don't think this will be much of a problem.
>
> In general, I'm very serious about not breaking compatibility.
> If ever we break compatibility, we must be very loud
> about it and very justified. We must always provide a
> mechanical migration path for our users. Otherwise they'll
> hate us.
>
> > > > - add some tests for the changes.
> Sure.
>
> > > Is this ok for you?
> You bet. Feel free to improvise as necessary.
>
> > > When creating member lists before the CompositeList,
> > > there must be a way to share not only the lock but also
> > > the publisher with CompositeList, right?
> You got it.
>
> > > So, should I change the signature of the existing constructor
> > > public CompositeList(ReadWriteLock lock)
> > > or add a new constructor
> > > public CompositeList(ReadWriteLock lock, ListEventPublisher publisher)
> Add the new constructor, deprecate the old one with
> a brief description of why. We can strip out the deprecated
> stuff at some point in the distant future.
>
> > > Should code changes that break existing code be deferred to a major
> > release (2.0)?
> Probably. But in this case as long as we're loud and
> the signatures don't change, I think we'll be okay.
>
> > > > BTW,
> > > > the next two weeks I'm going on summer holiday.
> > > > So I'm probably a few days offline in that time.
>
> Happy travels! That reminds me - I don't know much about you
> Holger - what you do, what your favourite color is, your age... feel free
> to provide a brief introductory bio of yourself if you'd like.
>
> Cheers,
> Jesse
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>


______________________________________________________________
Verschicken Sie romantische, coole und witzige Bilder per SMS!
Jetzt bei WEB.DE FreeMail: http://f.web.de/?mc=021193

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]