Thread safety, static variables, etc

[James M Snell <ja...@gmail.com>]

As Garrett has pointed out, the current approach to handling the Abdera
configuration model has a number of important thread safety and class
loader issues that had largely been ignored in the initial drop of the
code.  After talking with Garrett a bit, we both feel that now might be
the best time to address those issues, before we get much further down
the road.

There are several important challenges with the current code:

1. The use of static singletons for the default Parser, Factory and
XPath instances is not thread safe and can cause problems when Abdera is
deployed in managed environments like WebSphere, Tomcat, etc.

2. The current ConfigProperties and ServiceUtil implementations are not
thread safe and don't properly handle class loaders.  For instance, if
the Abdera jars are included in tomcat's classpath instead of a webapps
classpath, the webapp cannot override the default config properties.

3. Using ThreadLocal (as I originally attempted to do) actually won't
work.  I remembered after checking that code in a bit earlier today that
there were some significant problems with using ThreadLocal in managed
environments and thread pools.  Specifically, the thread local storage
might not be cleared out when the thread is returned to the pool.  There
are right ways of using ThreadLocal and the way I was intending to use
it isn't it.

There are other problems but that's enough to get started :-)

What I propose is a significant modification to the config mechanism.
Rather than acquiring instances of the Parser, Factory, etc using the
xxxx.INSTANCE pattern (e.g. Parser.INSTANCE, Factory.INSTANCE, etc) we
would create a top level instance object that would completely avoid
using statics for configuration properties.

  Abdera abdera = new Abdera();
  Parser parser = abdera.getParser();
  Factory factory = abdera.getFactory();

A new AbderaConfiguration object would replace the current
ConfigProperties static utility class.

  AbderaConfiguration config = AbderaConfiguration.getDefault();
  Abdera abdera = new Abdera(config);

There are a number of advantages to this approach beyond improving
thread safety and classloader handling.

For one, the current code requires that the default implementations of
the Parser, Factory and XPath objects be present in the classpath even
if they are not being used.  With the new approach, if you're not using
the FOM* implementation, it doesn't have to be present in the classpath
at all.

Also, it will provide us a way of configuring Abdera at runtime via the
new AbderaConfiguration object.  e.g. we can register extension
factories dynamically.

This would be a significant change that would touch a lot of the current
code and tests.

Thoughts?

- James

Re: Thread safety, static variables, etc

[James M Snell <ja...@gmail.com>]

Yes, the concurrency issues should be fixed in the trunk but no, there
are no tests yet to make sure.  Not all of the code is threadsafe tho.
None of the model.* classes should be considered threadsafe.  You'll
definitely need to provide your own synchronization if you want to do
something silly like, allowing multiple threads to modify a single Entry
:-).

A suite of concurrency tests would be very welcome.

- James

Ugo Cei wrote:
> 
> On Aug 22, 2006, at 9:58 PM, James M Snell wrote:
> 
>> As Garrett has pointed out, the current approach to handling the Abdera
>> configuration model has a number of important thread safety and class
>> loader issues that had largely been ignored in the initial drop of the
>> code.  After talking with Garrett a bit, we both feel that now might be
>> the best time to address those issues, before we get much further down
>> the road.
> 
> I think I am being bitten by concurrency issues when using 0.1.0. Are
> they supposedly fixed in trunk? In case they are, I could produce a
> suite of tests that fail against 0.1.0 and run them against trunk to see
> if they succeed.
> 
> What I am asking is whether there are any tests already for this, so I
> can see if my cases are already covered by those. If this is not the
> case, I might add my tests to the current suite.
> 
>     Ugo
> 
> 
> 
> --Ugo Cei
> Blog: http://agylen.com/
> Open Source Zone: http://oszone.org/
> Evil or Not?: http://evilornot.info/
> Company: http://www.sourcesense.com/
> 
> 
> 

Re: Thread safety, static variables, etc

[Ugo Cei <ug...@gmail.com>]

On Aug 22, 2006, at 9:58 PM, James M Snell wrote:

> As Garrett has pointed out, the current approach to handling the  
> Abdera
> configuration model has a number of important thread safety and class
> loader issues that had largely been ignored in the initial drop of the
> code.  After talking with Garrett a bit, we both feel that now  
> might be
> the best time to address those issues, before we get much further down
> the road.

I think I am being bitten by concurrency issues when using 0.1.0. Are  
they supposedly fixed in trunk? In case they are, I could produce a  
suite of tests that fail against 0.1.0 and run them against trunk to  
see if they succeed.

What I am asking is whether there are any tests already for this, so  
I can see if my cases are already covered by those. If this is not  
the case, I might add my tests to the current suite.

	Ugo



-- 
Ugo Cei
Blog: http://agylen.com/
Open Source Zone: http://oszone.org/
Evil or Not?: http://evilornot.info/
Company: http://www.sourcesense.com/

Re: Thread safety, static variables, etc

[Elias Torres <el...@torrez.us>]

Garrett Rooney wrote:
> On 8/22/06, James M Snell <ja...@gmail.com> wrote:
[snip]
> 
> I never really liked the .INSTANCE stuff anyway ;-)
> 
> =garrett
> 

+1

Re: Thread safety, static variables, etc

[Garrett Rooney <ro...@electricjellyfish.net>]

On 8/22/06, James M Snell <ja...@gmail.com> wrote:
> As Garrett has pointed out, the current approach to handling the Abdera
> configuration model has a number of important thread safety and class
> loader issues that had largely been ignored in the initial drop of the
> code.  After talking with Garrett a bit, we both feel that now might be
> the best time to address those issues, before we get much further down
> the road.
>
> There are several important challenges with the current code:
>
> 1. The use of static singletons for the default Parser, Factory and
> XPath instances is not thread safe and can cause problems when Abdera is
> deployed in managed environments like WebSphere, Tomcat, etc.
>
> 2. The current ConfigProperties and ServiceUtil implementations are not
> thread safe and don't properly handle class loaders.  For instance, if
> the Abdera jars are included in tomcat's classpath instead of a webapps
> classpath, the webapp cannot override the default config properties.
>
> 3. Using ThreadLocal (as I originally attempted to do) actually won't
> work.  I remembered after checking that code in a bit earlier today that
> there were some significant problems with using ThreadLocal in managed
> environments and thread pools.  Specifically, the thread local storage
> might not be cleared out when the thread is returned to the pool.  There
> are right ways of using ThreadLocal and the way I was intending to use
> it isn't it.
>
> There are other problems but that's enough to get started :-)
>
> What I propose is a significant modification to the config mechanism.
> Rather than acquiring instances of the Parser, Factory, etc using the
> xxxx.INSTANCE pattern (e.g. Parser.INSTANCE, Factory.INSTANCE, etc) we
> would create a top level instance object that would completely avoid
> using statics for configuration properties.
>
>   Abdera abdera = new Abdera();
>   Parser parser = abdera.getParser();
>   Factory factory = abdera.getFactory();

For what it's worth, there are a number of use cases that just need to
grab a Parser, or just need to grab a Factory, so it wouldn't be
outside the realm of possibility to provide something like:

  Parser p = Abdera.getParser();

This would, of course, do the equivalent of new Abders().getParser()
under the hood, but would be nicer looking for people who don't care
about the overhead of looking up what underlying implementation to do
etc.

> This would be a significant change that would touch a lot of the current
> code and tests.

If there isn't another sane work around for the various problems, I'd
say go for it, make the huge sweeping change now before people get
attached to the current way of doing things.

I never really liked the .INSTANCE stuff anyway ;-)

=garrett

Re: Thread safety, static variables, etc

[Stephen Duncan <st...@gmail.com>]

+1.  All sounds better (and more familiar) to me, and the sooner the
change is made the better, so now's the time.

-Stephen

On 8/22/06, James M Snell <ja...@gmail.com> wrote:
> As Garrett has pointed out, the current approach to handling the Abdera
> configuration model has a number of important thread safety and class
> loader issues that had largely been ignored in the initial drop of the
> code.  After talking with Garrett a bit, we both feel that now might be
> the best time to address those issues, before we get much further down
> the road.
>
> There are several important challenges with the current code:
>
> 1. The use of static singletons for the default Parser, Factory and
> XPath instances is not thread safe and can cause problems when Abdera is
> deployed in managed environments like WebSphere, Tomcat, etc.
>
> 2. The current ConfigProperties and ServiceUtil implementations are not
> thread safe and don't properly handle class loaders.  For instance, if
> the Abdera jars are included in tomcat's classpath instead of a webapps
> classpath, the webapp cannot override the default config properties.
>
> 3. Using ThreadLocal (as I originally attempted to do) actually won't
> work.  I remembered after checking that code in a bit earlier today that
> there were some significant problems with using ThreadLocal in managed
> environments and thread pools.  Specifically, the thread local storage
> might not be cleared out when the thread is returned to the pool.  There
> are right ways of using ThreadLocal and the way I was intending to use
> it isn't it.
>
> There are other problems but that's enough to get started :-)
>
> What I propose is a significant modification to the config mechanism.
> Rather than acquiring instances of the Parser, Factory, etc using the
> xxxx.INSTANCE pattern (e.g. Parser.INSTANCE, Factory.INSTANCE, etc) we
> would create a top level instance object that would completely avoid
> using statics for configuration properties.
>
>   Abdera abdera = new Abdera();
>   Parser parser = abdera.getParser();
>   Factory factory = abdera.getFactory();
>
> A new AbderaConfiguration object would replace the current
> ConfigProperties static utility class.
>
>   AbderaConfiguration config = AbderaConfiguration.getDefault();
>   Abdera abdera = new Abdera(config);
>
> There are a number of advantages to this approach beyond improving
> thread safety and classloader handling.
>
> For one, the current code requires that the default implementations of
> the Parser, Factory and XPath objects be present in the classpath even
> if they are not being used.  With the new approach, if you're not using
> the FOM* implementation, it doesn't have to be present in the classpath
> at all.
>
> Also, it will provide us a way of configuring Abdera at runtime via the
> new AbderaConfiguration object.  e.g. we can register extension
> factories dynamically.
>
> This would be a significant change that would touch a lot of the current
> code and tests.
>
> Thoughts?
>
> - James
>


-- 
Stephen Duncan Jr
www.stephenduncanjr.com