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

[Sanne Grinovero]

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:

1) In FullTextIndexEventListener.initialize() whe do nothing if
already initialized
2) Change EventListenerRegister to always REMOVE whatever additional
SearchListener if finds, to make sure there is only one instance.

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
4) change the current FullTextIndexCollectionEventListener to a no-op
listener, eventually removing/replacing it in EventListenerRegister.

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.
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);

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