Re: Immutable SyntexCheckers
Le 20/03/2017 à 08:20, Stefan Seelmann a écrit :
> On 03/20/2017 08:12 AM, Emmanuel Lécharny wrote:
>>
>> Le 20/03/2017 à 07:37, Stefan Seelmann a écrit :
>>> On 03/20/2017 02:10 AM, Emmanuel Lécharny wrote:
public class BooleanSyntaxChecker extends SyntaxChecker
{
/**
* A static instance of BooleanSyntaxChecker
*/
public static final BooleanSyntaxChecker INSTANCE = new
BooleanSyntaxChecker( SchemaConstants.BOOLEAN_SYNTAX );
/** A static instance of the builder */
private static final Builder BUILDER_INSTANCE = new Builder();
/**
* @return An instance of the Builder for this class
*/
public static Builder builder()
{
return BUILDER_INSTANCE;
}
>>> Hm, why a static builder? As it is not immutable there's a chance of
>>> race condition in case two threads use it concurrently.
>> That can't happen, because we have :
>>
>> private static final Builder BUILDER_INSTANCE = new Builder();
>>
>> that is guaranteed to be built during the class loading.
> I agree about the creation of the builder instance.
>
> But if two threads *use* it:
> 1. thread 1 calls builder()
> 2. thread 2 calls builder() and gets the same builder instance
> 3. thread 1 calls setOid("1.1.1")
> 4. thread 2 calls setOid("2.2.2")
> 5. thread 1 calls build() and may get an SC with OID "2.2.2"!
Good catch...
/**
* @return An instance of the Builder for this class
*/
public static Builder builder()
{
return new Builder();
}
would solve the issue, correct ?
(somtime, trying to verdue is wrong).
>
>
--
Emmanuel Lecharny
Symas.com
directory.apache.org
Re: Immutable SyntexCheckers
On 03/20/2017 08:12 AM, Emmanuel Lécharny wrote:
>
>
> Le 20/03/2017 à 07:37, Stefan Seelmann a écrit :
>> On 03/20/2017 02:10 AM, Emmanuel Lécharny wrote:
>>> public class BooleanSyntaxChecker extends SyntaxChecker
>>> {
>>> /**
>>> * A static instance of BooleanSyntaxChecker
>>> */
>>> public static final BooleanSyntaxChecker INSTANCE = new
>>> BooleanSyntaxChecker( SchemaConstants.BOOLEAN_SYNTAX );
>>>
>>> /** A static instance of the builder */
>>> private static final Builder BUILDER_INSTANCE = new Builder();
>>> /**
>>> * @return An instance of the Builder for this class
>>> */
>>> public static Builder builder()
>>> {
>>> return BUILDER_INSTANCE;
>>> }
>> Hm, why a static builder? As it is not immutable there's a chance of
>> race condition in case two threads use it concurrently.
>
> That can't happen, because we have :
>
> private static final Builder BUILDER_INSTANCE = new Builder();
>
> that is guaranteed to be built during the class loading.
I agree about the creation of the builder instance.
But if two threads *use* it:
1. thread 1 calls builder()
2. thread 2 calls builder() and gets the same builder instance
3. thread 1 calls setOid("1.1.1")
4. thread 2 calls setOid("2.2.2")
5. thread 1 calls build() and may get an SC with OID "2.2.2"!
Re: Immutable SyntexCheckers
Le 20/03/2017 à 07:37, Stefan Seelmann a écrit :
> On 03/20/2017 02:10 AM, Emmanuel Lécharny wrote:
>> public class BooleanSyntaxChecker extends SyntaxChecker
>> {
>> /**
>> * A static instance of BooleanSyntaxChecker
>> */
>> public static final BooleanSyntaxChecker INSTANCE = new
>> BooleanSyntaxChecker( SchemaConstants.BOOLEAN_SYNTAX );
>>
>> /** A static instance of the builder */
>> private static final Builder BUILDER_INSTANCE = new Builder();
>> /**
>> * @return An instance of the Builder for this class
>> */
>> public static Builder builder()
>> {
>> return BUILDER_INSTANCE;
>> }
> Hm, why a static builder? As it is not immutable there's a chance of
> race condition in case two threads use it concurrently.
That can't happen, because we have :
private static final Builder BUILDER_INSTANCE = new Builder();
that is guaranteed to be built during the class loading.
--
Emmanuel Lecharny
Symas.com
directory.apache.org
Re: Immutable SyntexCheckers
On 03/20/2017 02:10 AM, Emmanuel Lécharny wrote:
> public class BooleanSyntaxChecker extends SyntaxChecker
> {
> /**
> * A static instance of BooleanSyntaxChecker
> */
> public static final BooleanSyntaxChecker INSTANCE = new
> BooleanSyntaxChecker( SchemaConstants.BOOLEAN_SYNTAX );
>
> /** A static instance of the builder */
> private static final Builder BUILDER_INSTANCE = new Builder();
> /**
> * @return An instance of the Builder for this class
> */
> public static Builder builder()
> {
> return BUILDER_INSTANCE;
> }
Hm, why a static builder? As it is not immutable there's a chance of
race condition in case two threads use it concurrently.
Re: Immutable SyntexCheckers
Le 19/03/2017 à 17:47, Emmanuel Lecharny a écrit :
> Thanks, Stefan.
>
> I'll have a look at generics tonite.
>
> Regarding the need of a builder for each class, I think it would help
> habing a consistent system across all the SCs.
>
> Otherwise, this idea should apply to the other schema objects.
>
>
> Le dim. 19 mars 2017 à 10:58, Stefan Seelmann a
> écrit :
>
>> On 03/17/2017 11:00 AM, Emmanuel Lécharny wrote:
>> > The only requirement is that each SC has its own builder (we can't put
>> > this code in the abstract SC, because it's abstract...)
>
>> With some generics magic it should at least be possible to move common
>> stuff (setOid) to the parent.
I come with that :
in SyntaxChecker base class :
public abstract class SyntaxChecker extends LoadableSchemaObject
{
...
/**
* A static Builder for this class
*/
public abstract static class SCBuilder
{
/** The SyntaxChecker OID */
protected String oid;
/**
* The Builder constructor
*/
protected SCBuilder( String oid )
{
this.oid = oid;
}
/**
* Set the SyntaxChecker's OID
*
* @param oid The OID
* @return The Builder's Instance
*/
public SCBuilder setOid( String oid )
{
this.oid = oid;
return this;
}
public abstract SC build();
}
In BooleanSyntaxChecker inherited class :
public class BooleanSyntaxChecker extends SyntaxChecker
{
/**
* A static instance of BooleanSyntaxChecker
*/
public static final BooleanSyntaxChecker INSTANCE = new
BooleanSyntaxChecker( SchemaConstants.BOOLEAN_SYNTAX );
/** A static instance of the builder */
private static final Builder BUILDER_INSTANCE = new Builder();
/**
* A static Builder for this class
*/
public static class Builder extends SCBuilder
{
/**
* The Builder constructor
*/
private Builder()
{
super( SchemaConstants.BOOLEAN_SYNTAX );
}
/**
* Create a new instance of BooleanSyntaxChecker
* @return A new instance of BooleanSyntaxChecker
*/
@Override
public BooleanSyntaxChecker build()
{
return new BooleanSyntaxChecker( oid );
}
}
/**
* Creates a new instance of BooleanSyntaxChecker.
*/
private BooleanSyntaxChecker( String oid )
{
super( oid );
}
/**
* @return An instance of the Builder for this class
*/
public static Builder builder()
{
return BUILDER_INSTANCE;
}
--
Emmanuel Lecharny
Symas.com
directory.apache.org
Re: Immutable SyntexCheckers
Thanks, Stefan. I'll have a look at generics tonite. Regarding the need of a builder for each class, I think it would help habing a consistent system across all the SCs. Otherwise, this idea should apply to the other schema objects. Le dim. 19 mars 2017 à 10:58, Stefan Seelmann a écrit : > On 03/17/2017 11:00 AM, Emmanuel Lécharny wrote: > > The only requirement is that each SC has its own builder (we can't put > > this code in the abstract SC, because it's abstract...) > > With some generics magic it should at least be possible to move common > stuff (setOid) to the parent. > > Otherwise, I'm not sure if it's worth to create builders for all the > "simple" syntax checkers (Boolean, DirectoryString, etc.) or if just a > singleton immutable instance is sufficient. > > But in general it makes sense to have immutable syntax checkers :) > > > -- Regards, Cordialement, Emmanuel Lécharny www.iktek.com
Re: Immutable SyntexCheckers
On 03/17/2017 11:00 AM, Emmanuel Lécharny wrote: > The only requirement is that each SC has its own builder (we can't put > this code in the abstract SC, because it's abstract...) With some generics magic it should at least be possible to move common stuff (setOid) to the parent. Otherwise, I'm not sure if it's worth to create builders for all the "simple" syntax checkers (Boolean, DirectoryString, etc.) or if just a singleton immutable instance is sufficient. But in general it makes sense to have immutable syntax checkers :)
Immutable SyntexCheckers
Hi guys,
last week-end, I worked on adding a way to get a static instance of
SyntaxCheckers, instead of having to create a new instance every now and
then. As most of those SC aren't going to be modified during the
lifetime of an application, it makes a lot fo sense to have it
constructed once, and only once.
First let me summarize their lifecycle.
SC (SyntaxChecker) are associated with a LDAP Syntax, and are helper
classes used to check that a value is correct, accordingly to the
AttributeType syntax. We have more or less a hundred of such objects.
Most of them are defined in the code base, and will never change, but a
few can take a parameter, like teh TelephoneNumber that can use a regexp
as a matcher.
We wanted to be able to 'inject' some new SC dynamiclaly in the server,
by using a Java class that implements a new checker (this actually
works, but it's not really use as of today). The whole idea was to make
it dead simple - sort of - to extend the Schema, adding application
driven Schema elements. Bottom line, the mechanism is in place.
Now, when do we use those SC ? When we need to check a value. In this
case, we know about the value' AttributeType, and this AT knwos about
its Syntax, which is associated with a SC. The instance are created when
we load the schema at startup, as we store the FQCN of each SC in the
Schema :
dn: m-oid=1.3.6.1.4.1.1466.115.121.1.7,ou=syntaxCheckers,cn=system,ou=schema
m-oid: 1.3.6.1.4.1.1466.115.121.1.7
m-fqcn:
org.apache.directory.api.ldap.model.schema.syntaxCheckers.BooleanSyntaxChecker
objectclass: metaSyntaxChecker
objectclass: metaTop
objectclass: top
creatorsname: uid=admin,ou=system
The relation between a Ldap Syntax and its associated SC is the OID :
they have the same. So when we load the LdapSyntax, we class load its
associated SC. As a matter of fact, LdapSyntax are just descriptive,
when SC are executable.
That being said, there is not much of a reason to have the SC being
mutable. Even for the very few ones that can use a configuration - like
a regexp -, once created, they don't change.
So I changed the existing SC code by adding a static INSTANCE, which is
a reference to the instanciated SC. You don't need anymore to create a
new instance when you need it, you just pull the INSTANCE.
That caused some issue in some tests, that were creating new instances
of a SC, but with a different OID. Using INSTANCE was breaking the
tests, because those tests were expecting a different OID. Actually, the
test was faulty, we should have created a totally new SC instead of
redefining and existing one, but well, that life and it raised a
concern. And this concern was even more important in the tests using teh
TelephoneNumber SC, because you can inject new regexp into it...
So we fixed the tests, and now, it's time to think about a better
solution, where the SC are completely immutable.
The idea is to use internal Builders :
public class BooleanSyntaxChecker extends SyntaxChecker
{
/**
* A static instance of BooleanSyntaxChecker
*/
public static final BooleanSyntaxChecker INSTANCE = new
BooleanSyntaxChecker( SchemaConstants.BOOLEAN_SYNTAX );
/** A static instance of the builder */
private static final Builder BUILDER_INSTANCE = new Builder();
/**
* A static Builder for this class
*/
public static class Builder
{
/** The BooleanSyntaxChecker OID */
private String oid = SchemaConstants.BOOLEAN_SYNTAX;
/**
* The Builder constructor
*/
private Builder()
{
// Nothing to do
}
/**
* Set the SyntaxChecker's OID
*
* @param oid The OID
* @return The Builder's Instance
*/
public Builder setOid( String oid )
{
this.oid = oid;
return this;
}
/**
* Create a new instance of BooleanSyntaxChecker
* @return A new instance of BooleanSyntaxChecker
*/
public BooleanSyntaxChecker build()
{
return new BooleanSyntaxChecker( oid );
}
}
/**
* Creates a new instance of BooleanSyntaxChecker.
*/
private BooleanSyntaxChecker( String oid )
{
super( oid );
}
/**
* @return An instance of the Builder for this class
*/
public static Builder builder()
{
return BUILDER_INSTANCE;
}
The constructor is now private, so you can't instanciate the class. What
you have to do is either to get the static INSTANCE, and you get an
immutable and static version of the class, or you 'build' a completely
new instance using code like :
BooleanSyntaxChecker checker = BooleanSyntaxChecker.builder().build();
(which is actually the same as the static INSTANCE, but as a different
object), or :
BooleanSyntaxChecker checker =
BooleanSyntaxChecker.builder().setOid( "1.2.3.4"
