[hibernate-dev] About Search's Threadlocal and memory leak: automatic listener registration

[Emmanuel Bernard]

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 at lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/hibernate-dev