Re: [hibernate-dev] Hibernate Search: Master/Slave DirectoryProviders improvements proposal
Hi Hardy,
A -- nice I'm getting the chance to teach something to you gurus..
yes this is really required, locking is fine, but there are two problems there;
a) visibility: JVM makes no guarantee another thread will see the changes
made to a variable by another thread; it could see it "later on" and could also
never see the updated value. Also when several variables are updated,
the other thread
could see them updated in different order (so returning another
Dir.Provider than
intended by other state variables). Only "final" instance variables
are to be considered
safe.
b) variable update atomicity: when updating a reference you are
changing several
byte variables, another thread could read some bytes of a new value and some of
an older version. This depends on architecture and JVM implementation,
but should really be avoided.
Just remember that all communication between threads must always be
synchronized,
or even better use some of the really good java.util.concurrent.
That's why I am recommending the AtomicReference, this class has been
made exactly
to solve this problem, and on the architectures where it is unneeded
it is just compiled
to the fastest but SAFE implementation; the same considerations are true
for any java type, even primitives: one line of code in java doesn't
mean it's an
atomic op (sorry for repeating the obvious).
For a very good example on this problem:
http://en.wikipedia.org/wiki/Double_checked_locking_pattern
D -- how stupid, I'm sorry I didn't think about that!
thanks, I suppose D is ok then.
As C and D are just little improvements, but A and B look like a bugs
needing a fix with some urgency, I would like to address A and B first; may I
open a single JIRA for both? both issues are related to the same files
and I would
like to avoid having to produce two different patches, potentially conflicting
with myself.
regards,
Sanne
2008/4/27 Hardy Ferentschik <hibernate(a)ferentschik.de>:
Hi Sanne,
> A) ... but all comunication beetween the client thread calling
"getDirectory()" and the CopyDirectory
>
> which defines the current directory is unprotected; also volatile isn't
enough
> to fix it, IMHO an AtomicReference would be more appropriate.
>
Is this really required? Is synchronisation not ensured via
SearchFactoryImplementor.getLockableDirectoryProviders()?
> B) ... Also I would like to address the use of finalize() to stop the
Timer,
>
> there is a TODO about it already;
>
Fair point. Initially a non daemon thread was used. Once we switched to a
deamon thread we
could probably have got rid of the call to timer.canel().
> C) FileHelper not listening to Thread.interrupt() requests
> As FileHelper.synchronize is used in the mentioned Timers to copy
> potentially big files,
> it would be nice to check for Thread.isInterrupted() and also catch
> ClosedByInterruptException in file-copy operations so to abort all I/O
> operations and
> not just the current one to abort faster on JVM shutdown.
> (It doesn't look like the copy safety was guaranteed anyway).
>
Sounds reasonable.
> D) Master and Slave share configuration properties.
> Wouldn't it be natural to have two different property keys to manage
> the shared index path? I imagine the typical scenario would be to have the
two
> configured on different servers and copy through some NFS, but it looks
like
> we are forcing people to use exactly the same path on both machines as the
two
> classes read the same props.
>
The properties are different. Yes, the master and the slave both define:
hibernate.search.default.sourceBase and hibernate.search.default.indexBase,
but remember
that these are in two different configuration files. Master and slave are
two seperate machines
or at the very least two different JVMs. Nothing stops you to have
different mount points. I guess the
docuemtnation might suggest this.
You can definitely create a Jira issue for B and D. Not sure about A
though.
Maybe someone else can have another look at the code.
--Hardy