excalibur-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Berin Loritsch <blorit...@d-haven.org>
Subject Re: refactoring StoreJanitorImpl
Date Wed, 10 Nov 2004 21:26:52 GMT
On Wed, 2004-11-10 at 15:49, Giacomo Pati wrote:
> On Wed, 10 Nov 2004, Berin Loritsch wrote:
> 
> > Giacomo Pati wrote:
> >
> >> Vadim brought up the idea to contribute the Cocoon Event package 
> >> replacement (RunnableManager) to Excalibur. Personally, I'm not sure 
> >> about it as in the end we have the Event package back here in a different 
> >> form. So, maybe you guys have a look at it at 
> >> http://svn.apache.org/repos/asf/cocoon/trunk/src/java/org/apache/cocoon/components/thread/
> >> and give us your thoughts about it.
> >
> > OK, here is my surmizing of what is in that package:
> >
> > 1) It is a thin wrapper around the Doug Lea Concurrent library
> 
> Yup.
> 
> > 2) That makes it pretty close to Java5 java.util.concurrent stuff
> > 3) The LinkedQueue will has race conditions waiting to happen with the
> >   size tracking--only use it with one thread at a time.
> 
> Hmm, you mean the insert method, right?
> 
> > 4) The RunnableManager is a lot of code without one test--it makes me
> > nervous.
> 
> Sure. Tests will follow.
> 
> > For the most part it is a big departure from the Excalibur/D-Haven event
> > packages and it is pretty close to the Doug Lea Concurrent library.  I
> > can see some potential issues with synchronization--especially as it
> > relates to tracking queue size.
> 
> Where?

Take a look at the LinkedListQueue.

There is no synchronization around the queue size, and that is altered
prior to calling the underlying channel.  If multiple threads access
that at the same time it will get out of sync.  It can also potentially
get out of sync when the underlying channel generates an error and
doesn't perform the expected operation.  The size isn't even declared
volatile (which is of dubious value as it is).

> 
> > The RunnableManager is a non-trivial class that might actually be
> > better split up.
> 
> Any suggestions are welcome.

I don't have anything specific, but as I started writing the tests
for the DefaultCommandManager in D-Haven event, it became increasingly
clear that I needed to split it up and test things separately.  A big
clue is if you have any inner classes, it might be beneficial to
split them out to first class citizens.  That's where I started when
I did that work.

> 
> > I highly recommend writing a bunch of test cases
> > with JUnit and Clover until the whole thing is covered by tests.
> > While I know Vadim does good work, it surprised me the number of
> > issues I found in my own code.
> 
> Don't blame Vadim about it, the RunnableManager is my s**t :-). Many 
> thank for looking at it. I'd rather stick with the d-haven event package 
> but the cocoon community has decided otherwise (you either know or can 
> query theaimsgroup about it).

Sorry, I saw Vadim's name on some of the classes, and assumed it was
on all of them.

Did they look at the D-Haven Event package?  Another reason for me
splitting it out of Avalon at the time was... well... due to
interpersonal volatility at the time.

I know that Stefano would really like to have as much in house for
Cocoon so that it doesn't look like building on sand any more, but
some libraries are just not easy--to write or test.


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@excalibur.apache.org
For additional commands, e-mail: dev-help@excalibur.apache.org
Apache Excalibur Project -- URL: http://excalibur.apache.org/


Mime
View raw message