Remove [Mutable]InterceptorConfiguration for 1.5.1? Also some thread-safety concerns

[David Jencks <da...@yahoo.com>]

A while back there was some discussion about just how much container  
functionality apacheds needs to duplicate for spring, and IIUC there  
was general agreement that [Mutable]InterceptorConfiguration could be  
replaced by just letting the container create the interceptors directly.

I started looking at this issue this morning and think it would be  
pretty easy to fix so I'm wondering if there's any chance it could  
get into 1.5.1.

I also see what looks to me like double-checked-locking problems with  
InterceptorChain setting up the chain of Entry objects.  IIUC there  
are a bunch of methods to insert,remove, etc interceptors in the  
running server but the actual code that traverses the interceptor  
chain is not at all synchronized.  This is a double-checked-locking  
scenario and can lead to the live interceptor chain using  
incompletely initialized interceptor objects.  The usual fix nowadays  
for such problems is to make the variables pointing to the possibly  
incompletely initialized objects volatile.  IIUC this would be the  
InterceptorChain.Entry prevEntry and nextEntry fields.

AFAICT the runtime change-the-chain methods are not called in the  
apacheds code base at the moment, so I think an acceptable  
alternative would be to remove those methods and rely on no one  
starting to use the server until it's fully initialized.  This would  
involve removing the InterceptorChain addFirst, addLast, addBefore,  
addAfter methods.


OK... back to the how-much-extra-wrapping question.

I haven't started to code yet but I think the best solution for now  
will be to:

- In each interceptor class that actually uses an interceptor  
configuration, replace that with individual attributes (I think only  
the replication interceptor has any attributes at the moment)
- remove the InterceptorConfiguration cfg argument from Interceptor.init

At this point we won't need InterceptorConfiguration any more, the  
container (spring) can create the interceptors itself, and  
InterceptorChain will get a list of interceptors it needs to  
initialize rather than a list of InterceptorConfigurations.

thanks
david jencks


  

Re: Remove [Mutable]InterceptorConfiguration for 1.5.1? Also some thread-safety concerns

[David Jencks <da...@yahoo.com>]

I opened https://issues.apache.org/jira/browse/DIRSERVER-1023 for  
this and attached my patch.  This patch includes the xbean-spring  
patch and the server.xml is for xbean-spring.  All the integration  
tests pass and the server works fine in the geronimo directory plugin.

Here's how the interceptor configuration now looks (using the xbean- 
spring format):

     <interceptors>
       <normalizationService/>
       <authenticationService/>
       <referralService/>
       <authorizationService/>
       <defaultAuthorizationService/>
       <exceptionService/>
       <operationalAttributeService/>
       <!-- Uncomment to enable the password policy service
       <passwordPolicyService/>
       <keyDerivationService/>
       -->
       <schemaService/>
       <subentryService/>
       <collectiveAttributeService/>
       <eventService/>
       <triggerService/>

         <!-- Uncomment to enable replication service
         <replicationService >
           <replicationConfiguration>
             <replicationConfiguration
               serverPort="10390"
               peerReplicas="instance_b@localhost:10392">
               <replicaId>
                 <bean xmlns="http://www.springframework.org/schema/ 
beans" class="org.apache.directory.mitosis.common.ReplicaId">
                   <constructor-arg>
                     <value>instance_a</value>
                   </constructor-arg>
                 </bean>
               </replicaId>
             </bean>
           </replicationConfiguration>
         </replicationService>
         -->
     </interceptors>


The only interceptor with any configuration is the  
ReplicationService.  It now has a setter for ReplicationConfiguration  
which spring will call.  Interceptors are still initialized by the  
InterceptorChain which I am not thrilled about but changing that  
would definitely be too big to get into 1.5.1.

I don't know how to test the ReplicationService so if someone who  
does could try it out that would be great.

Some other comments on the patch:

- Interceptors now have names.  These default to the class name of  
the interceptor, which makes it impossible to  accidently insert 2 of  
the same interceptor into the same chain by for instance giving them  
different names.  Since the ReplicationService has some  
configuration, you can set it's name.
- there are a lot of bypass lists.  I changed them so it's easier to  
see which interceptors are missing by commenting out the lines for  
missing interceptors rather than removing the lines.
- I eliminated the double-checked-locking problem by making the non- 
final pointers in Entry volatile.  I think it would make a lot more  
sense to:

a. have an arraylist of interceptors or entries rather than a double- 
linked-list
b. do not change these lists after they are set up but rather create  
a new one and swap it with the old one.  This would mean there is  
only one volatile needed for thread safety
c. consider having different lists of interceptors or entries for  
different purposes rather than computing skips on each traversal from  
the bypass set.

This is IMO really a fairly small change that considerably simplifies  
the server.xml and removes the silliest use of configuration objects,  
so I'd like to see it get into 1.5.1.  If people like the idea but  
want to stick with plain spring server.xml I can come up with a new  
patch.

thanks
david jencks

On Aug 18, 2007, at 2:21 PM, David Jencks wrote:

>
> On Aug 18, 2007, at 12:14 PM, Alex Karasulu wrote:
>
>> I glad to see you involved and thinking about these issues and  
>> aspects
>> of the server David.  I know we're going to gain a lot from your  
>> knowledge
>> in these areas.
>>
>> With respect to the Spring discussions I think we hit a stalemate  
>> a few
>> weeks back.  I know I've spoken to both you and Chris about this  
>> who feel
>> Spring can do much of what we would like to do.  As you know my  
>> stance
>> is to avoid being dependent on Spring.
>>
>> However I think we have some idea of how we can remain container  
>> independent
>> while still leveraging Spring.  We still need some discussions on  
>> this but I don't
>> think they're going to happen in the next 3 days.
>
> So maybe we're in danger of confusing 2 issues:
>
> 1. spring vs some other container.  On this I don't care very much  
> but I haven't seen anything proposed that is as easy to use,  
> flexible, and non-invasive as spring.  In any case surely 1.5.1 is  
> going out with spring as the container. (my xbean-spring proposal  
> fits under this item).  Designing components to be container  
> agnostic is a really good goal.... leading to
>
> 2. reasonable component design.  To me this includes not putting  
> container functionality into the components.  Currently at least  
> the [Mutable]InterceptorConfiguration classes are definitely mostly  
> doing stuff that should be done by the container, whatever it might  
> end up being.  One result is really ugly spring configuration.
>
> I'm proposing to work on (2) a bit, I think it would make the  
> server.xml quite a bit more comprehensible both for plain spring  
> and xbean-spring versions of server.xml
>>
>> Keep in mind 1.5.1 is a feature release where we're just releasing  
>> to get feature
>> regardless of stability into the hands of our users.  We can  
>> release at any point
>> in time again thereafter so don't feel pressured to pump your  
>> changes into this
>> release.
>
> well, I think the geronimo directory plugin will probably get  
> released against 1.5.1 and the interceptor configuration is really  
> bugging me and I have a couple extra hours.... so I thought I'd see  
> what I could do here.
>
> thanks
> david jencks
>
>>
>> Alex
>>
>> On 8/18/07, David Jencks <da...@yahoo.com> wrote:
>> A while back there was some discussion about just how much container
>> functionality apacheds needs to duplicate for spring, and IIUC there
>> was general agreement that [Mutable]InterceptorConfiguration could be
>> replaced by just letting the container create the interceptors  
>> directly.
>>
>> I started looking at this issue this morning and think it would be
>> pretty easy to fix so I'm wondering if there's any chance it could
>> get into 1.5.1.
>>
>> I also see what looks to me like double-checked-locking problems with
>> InterceptorChain setting up the chain of Entry objects.  IIUC there
>> are a bunch of methods to insert,remove, etc interceptors in the
>> running server but the actual code that traverses the interceptor
>> chain is not at all synchronized.  This is a double-checked-locking
>> scenario and can lead to the live interceptor chain using
>> incompletely initialized interceptor objects.  The usual fix nowadays
>> for such problems is to make the variables pointing to the possibly
>> incompletely initialized objects volatile.  IIUC this would be the
>> InterceptorChain.Entry prevEntry and nextEntry fields.
>>
>> AFAICT the runtime change-the-chain methods are not called in the
>> apacheds code base at the moment, so I think an acceptable
>> alternative would be to remove those methods and rely on no one
>> starting to use the server until it's fully initialized.  This would
>> involve removing the InterceptorChain addFirst, addLast, addBefore,
>> addAfter methods.
>>
>>
>> OK... back to the how-much-extra-wrapping question.
>>
>> I haven't started to code yet but I think the best solution for now
>> will be to:
>>
>> - In each interceptor class that actually uses an interceptor
>> configuration, replace that with individual attributes (I think only
>> the replication interceptor has any attributes at the moment)
>> - remove the InterceptorConfiguration cfg argument from  
>> Interceptor.init
>>
>> At this point we won't need InterceptorConfiguration any more, the
>> container (spring) can create the interceptors itself, and
>> InterceptorChain will get a list of interceptors it needs to
>> initialize rather than a list of InterceptorConfigurations.
>>
>> thanks
>> david jencks
>>
>>
>>
>>
>

Re: Remove [Mutable]InterceptorConfiguration for 1.5.1? Also some thread-safety concerns

[David Jencks <da...@yahoo.com>]

On Aug 18, 2007, at 12:14 PM, Alex Karasulu wrote:

> I glad to see you involved and thinking about these issues and aspects
> of the server David.  I know we're going to gain a lot from your  
> knowledge
> in these areas.
>
> With respect to the Spring discussions I think we hit a stalemate a  
> few
> weeks back.  I know I've spoken to both you and Chris about this  
> who feel
> Spring can do much of what we would like to do.  As you know my stance
> is to avoid being dependent on Spring.
>
> However I think we have some idea of how we can remain container  
> independent
> while still leveraging Spring.  We still need some discussions on  
> this but I don't
> think they're going to happen in the next 3 days.

So maybe we're in danger of confusing 2 issues:

1. spring vs some other container.  On this I don't care very much  
but I haven't seen anything proposed that is as easy to use,  
flexible, and non-invasive as spring.  In any case surely 1.5.1 is  
going out with spring as the container. (my xbean-spring proposal  
fits under this item).  Designing components to be container agnostic  
is a really good goal.... leading to

2. reasonable component design.  To me this includes not putting  
container functionality into the components.  Currently at least the  
[Mutable]InterceptorConfiguration classes are definitely mostly doing  
stuff that should be done by the container, whatever it might end up  
being.  One result is really ugly spring configuration.

I'm proposing to work on (2) a bit, I think it would make the  
server.xml quite a bit more comprehensible both for plain spring and  
xbean-spring versions of server.xml
>
> Keep in mind 1.5.1 is a feature release where we're just releasing  
> to get feature
> regardless of stability into the hands of our users.  We can  
> release at any point
> in time again thereafter so don't feel pressured to pump your  
> changes into this
> release.

well, I think the geronimo directory plugin will probably get  
released against 1.5.1 and the interceptor configuration is really  
bugging me and I have a couple extra hours.... so I thought I'd see  
what I could do here.

thanks
david jencks

>
> Alex
>
> On 8/18/07, David Jencks <da...@yahoo.com> wrote:
> A while back there was some discussion about just how much container
> functionality apacheds needs to duplicate for spring, and IIUC there
> was general agreement that [Mutable]InterceptorConfiguration could be
> replaced by just letting the container create the interceptors  
> directly.
>
> I started looking at this issue this morning and think it would be
> pretty easy to fix so I'm wondering if there's any chance it could
> get into 1.5.1.
>
> I also see what looks to me like double-checked-locking problems with
> InterceptorChain setting up the chain of Entry objects.  IIUC there
> are a bunch of methods to insert,remove, etc interceptors in the
> running server but the actual code that traverses the interceptor
> chain is not at all synchronized.  This is a double-checked-locking
> scenario and can lead to the live interceptor chain using
> incompletely initialized interceptor objects.  The usual fix nowadays
> for such problems is to make the variables pointing to the possibly
> incompletely initialized objects volatile.  IIUC this would be the
> InterceptorChain.Entry prevEntry and nextEntry fields.
>
> AFAICT the runtime change-the-chain methods are not called in the
> apacheds code base at the moment, so I think an acceptable
> alternative would be to remove those methods and rely on no one
> starting to use the server until it's fully initialized.  This would
> involve removing the InterceptorChain addFirst, addLast, addBefore,
> addAfter methods.
>
>
> OK... back to the how-much-extra-wrapping question.
>
> I haven't started to code yet but I think the best solution for now
> will be to:
>
> - In each interceptor class that actually uses an interceptor
> configuration, replace that with individual attributes (I think only
> the replication interceptor has any attributes at the moment)
> - remove the InterceptorConfiguration cfg argument from  
> Interceptor.init
>
> At this point we won't need InterceptorConfiguration any more, the
> container (spring) can create the interceptors itself, and
> InterceptorChain will get a list of interceptors it needs to
> initialize rather than a list of InterceptorConfigurations.
>
> thanks
> david jencks
>
>
>
>

Re: Remove [Mutable]InterceptorConfiguration for 1.5.1? Also some thread-safety concerns

[Alex Karasulu <ak...@apache.org>]

I glad to see you involved and thinking about these issues and aspects
of the server David.  I know we're going to gain a lot from your knowledge
in these areas.

With respect to the Spring discussions I think we hit a stalemate a few
weeks back.  I know I've spoken to both you and Chris about this who feel
Spring can do much of what we would like to do.  As you know my stance
is to avoid being dependent on Spring.

However I think we have some idea of how we can remain container independent
while still leveraging Spring.  We still need some discussions on this but I
don't
think they're going to happen in the next 3 days.

Keep in mind 1.5.1 is a feature release where we're just releasing to get
feature
regardless of stability into the hands of our users.  We can release at any
point
in time again thereafter so don't feel pressured to pump your changes into
this
release.

Alex

On 8/18/07, David Jencks <da...@yahoo.com> wrote:
>
> A while back there was some discussion about just how much container
> functionality apacheds needs to duplicate for spring, and IIUC there
> was general agreement that [Mutable]InterceptorConfiguration could be
> replaced by just letting the container create the interceptors directly.
>
> I started looking at this issue this morning and think it would be
> pretty easy to fix so I'm wondering if there's any chance it could
> get into 1.5.1.
>
> I also see what looks to me like double-checked-locking problems with
> InterceptorChain setting up the chain of Entry objects.  IIUC there
> are a bunch of methods to insert,remove, etc interceptors in the
> running server but the actual code that traverses the interceptor
> chain is not at all synchronized.  This is a double-checked-locking
> scenario and can lead to the live interceptor chain using
> incompletely initialized interceptor objects.  The usual fix nowadays
> for such problems is to make the variables pointing to the possibly
> incompletely initialized objects volatile.  IIUC this would be the
> InterceptorChain.Entry prevEntry and nextEntry fields.
>
> AFAICT the runtime change-the-chain methods are not called in the
> apacheds code base at the moment, so I think an acceptable
> alternative would be to remove those methods and rely on no one
> starting to use the server until it's fully initialized.  This would
> involve removing the InterceptorChain addFirst, addLast, addBefore,
> addAfter methods.
>
>
> OK... back to the how-much-extra-wrapping question.
>
> I haven't started to code yet but I think the best solution for now
> will be to:
>
> - In each interceptor class that actually uses an interceptor
> configuration, replace that with individual attributes (I think only
> the replication interceptor has any attributes at the moment)
> - remove the InterceptorConfiguration cfg argument from Interceptor.init
>
> At this point we won't need InterceptorConfiguration any more, the
> container (spring) can create the interceptors itself, and
> InterceptorChain will get a list of interceptors it needs to
> initialize rather than a list of InterceptorConfigurations.
>
> thanks
> david jencks
>
>
>
>