On Dec 12, 2008, at 12:26, Sanne Grinovero wrote:
Hi all,
reading the code it appears the JIRA reporter is right: the static
ThreadLocal is using a WeakHashMap but the value
is holding a reference to the Configuration, which also happens to
be the key.
(see SearchConfigurationFromHibernateCore 's constructor, called at
line 30 in ContextHolder)
So we could make a copy of this configuration object to break the
reference, but I would prefer
to remove the ThreadLocal completely: assuming we always have the
automatic registration in Search 3.1
as it depends on newest Annotations, my proposal is to:
humm, I remember we talked about that. What was the chain of thought
that lead us to think that everybody using HSearch uses annotation?
It might be the case if the future for Hibernate 3.5or + when we merge
back annotations and em into the same distro but even then we need to
backport that registry mechanism into core.
1) In FullTextIndexEventListener.initialize() whe do nothing if
already initialized
How do you know if it's already initialized? I mean wo the TL trick.
2) Change EventListenerRegister to always REMOVE whatever additional
SearchListener if finds, to make sure there is only one instance.
ok if it's in Core
The current EventListenerRegister semantic is not looking safe at the
moment anyway: if someone was to register a
FullTextIndexEventListener's
extension we end up with multiple Search listeners anyway.
As there is a TODO in FullTextIndexEventListener to "make it final as
soon as FullTextIndexCollectionEventListener is removed" I'd suggest
to also
3) make FullTextIndexEventListener final now
OK for 3.2 where we can get rid of CollectionEL
4) change the current FullTextIndexCollectionEventListener to a no-op
listener, eventually removing/replacing it in EventListenerRegister.
no we can remove it in 3.2
And not needed but I think would improve it more:
5A) change the current FullTextIndexEventListener no-arg constructor
to do nothing at initialize() to avoid unnecessary processing at
startup,
in case someone is still declaring it by configuration.
I don't understand. The constructor does nothing, only initialize does.
5B) add a 1-arg constructor to it, to be used by
EventListenerRegister, so that it's possible at last to have correct
final fields in FullTextIndexEventListener
to achieve a safe concurrent implementation (EventListenerRegister has
a reference to the configuration so can pass it directly to the
constructor);
You assume that the Configuration object is fully defined when the
event listener registration happens. I am not sure this is something
Core agrees on. initialize() is were core agrees that Configuration is
frozen.
IMHO this way the startup would get a nice cleanup: safe for
concurrency, no more ThreadLocal worries, no more unused
SearchFactories being
initialized.
what do you think?
Sanne
_______________________________________________
hibernate-dev mailing list
hibernate-dev(a)lists.jboss.org
https://lists.jboss.org/mailman/listinfo/hibernate-dev