[codec] org.apache.commons.codec.binary.BaseNCodec.strictDecoding

[Gary Gregory <ga...@gmail.com>]

Hi All,

The addition of org.apache.commons.codec.binary.BaseNCodec.strictDecoding
is not quite right IMO, the ivar should be final with some new ctors. ALL
other ivars here are final and the class is documented as thread-safe. I'll
proceed unless someone makes a case for jumping through the extra hoops for
make this setting mutable AND thread-safe.

Gary

Re: [codec] org.apache.commons.codec.binary.BaseNCodec.strictDecoding

[Gary Gregory <ga...@gmail.com>]

On Thu, May 14, 2020 at 3:51 PM Alex Herbert <al...@gmail.com>
wrote:

>
>
> > On 14 May 2020, at 18:03, Gary Gregory <ga...@gmail.com> wrote:
> >
> > Hi All,
> >
> > The addition of org.apache.commons.codec.binary.BaseNCodec.strictDecoding
> > is not quite right IMO, the ivar should be final with some new ctors. ALL
> > other ivars here are final and the class is documented as thread-safe.
> I'll
> > proceed unless someone makes a case for jumping through the extra hoops
> for
> > make this setting mutable AND thread-safe.
>
> The boolean is only used once per decode. Thus it is not thread-unsafe. It
> acts like a late binding behaviour flag. Thus if it is changed half way
> through decoding the effect is still consistent decoding, but maybe not
> what you expected. For example if one thread checks it is strict decoding
> and then starts decoding and meanwhile another thread with the same
> instance updates the property to be non-strict the first thread would get a
> possibly incorrect behaviour. I do not see a way around this as long as the
> strict decoding flag is mutable.
>
> When this property was introduced we had a discussion on whether to use a
> property or overloaded constructor. There are already a lot of
> constructors. Base64 has 5. So to avoid deciding which constructors to add
> I added this as a property. A change to remove the property setter would
> have to target:
>
> BaseNCodec
> BCodec
> BaseNCodecInputStream
> BaseNCodecOutputStream
>
> Which all have the property.
>
> If we are to maintain thread-safe and thread consistent behaviour then the
> flag should be final. An alternative is some work behind the
> isStrictDecoding property to fix the value on the first read of the field.
> So you can use the setStrictDecoding setter to update the property only if
> the decoder has never been used. Otherwise the value passed to the setter
> is ignored and the codec always functions the same as it did on first use.
> This seems worse to implement and document than just having some more
> constructors and a final flag.
>
> So +1 to more constructors and removing the setter.
>

I updated git master.

Note the change from a boolean to the new enum CodecPolicy{ LENIENT, STRICT
} which we can reuse elsewhere instead of yet another boolean toggle.

Gary

>
> Alex
>
>
> >
> > Gary
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>

Re: [codec] org.apache.commons.codec.binary.BaseNCodec.strictDecoding

[Alex Herbert <al...@gmail.com>]

> On 14 May 2020, at 18:03, Gary Gregory <ga...@gmail.com> wrote:
> 
> Hi All,
> 
> The addition of org.apache.commons.codec.binary.BaseNCodec.strictDecoding
> is not quite right IMO, the ivar should be final with some new ctors. ALL
> other ivars here are final and the class is documented as thread-safe. I'll
> proceed unless someone makes a case for jumping through the extra hoops for
> make this setting mutable AND thread-safe.

The boolean is only used once per decode. Thus it is not thread-unsafe. It acts like a late binding behaviour flag. Thus if it is changed half way through decoding the effect is still consistent decoding, but maybe not what you expected. For example if one thread checks it is strict decoding and then starts decoding and meanwhile another thread with the same instance updates the property to be non-strict the first thread would get a possibly incorrect behaviour. I do not see a way around this as long as the strict decoding flag is mutable.

When this property was introduced we had a discussion on whether to use a property or overloaded constructor. There are already a lot of constructors. Base64 has 5. So to avoid deciding which constructors to add I added this as a property. A change to remove the property setter would have to target:

BaseNCodec
BCodec
BaseNCodecInputStream
BaseNCodecOutputStream

Which all have the property.

If we are to maintain thread-safe and thread consistent behaviour then the flag should be final. An alternative is some work behind the isStrictDecoding property to fix the value on the first read of the field. So you can use the setStrictDecoding setter to update the property only if the decoder has never been used. Otherwise the value passed to the setter is ignored and the codec always functions the same as it did on first use. This seems worse to implement and document than just having some more constructors and a final flag.

So +1 to more constructors and removing the setter.

Alex


> 
> Gary


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