[hibernate-dev] Re: HSEARCH-178

[Sanne Grinovero]

I'm going for the Map approach, to favor cleaner code and because you
appear to be more comfortable with it.
Even so there are two more points to consider:

1)If in the future a code change of the Synchronization (stored in the
WeakMap as a value)
would be changed in any way to get a (hard) reference to the session,
we'll get a memory leak, as in HSEARCH-314.
As much as the threadlocal is dangerous, we have to be carefull with
the WeakMap.

2)Performance: you are reminding this is just a workaround for people
which don't like transactions, but the flushlistener is registered for
everybody.
So nothing will be put in the map (skipping the Synchronization registration) in
a correct usage case, but still during the flushlistener I've to look into
the map to verify there's nothing to do.
I'm not worried about the performance impact of looking into an empty map,
but felt I had to keep you informed.


2009/4/14 Emmanuel Bernard <emmanuel at hibernate.org>:
>
> On  Apr 14, 2009, at 13:03, Sanne Grinovero wrote:
>
>> I've 2 inline answers:
>>
>> 2009/4/14 Emmanuel Bernard <emmanuel at hibernate.org>:
>>>
>>> Read inline
>>>
>>> On  Apr 13, 2009, at 17:22, Sanne Grinovero wrote:
>>>
>>>> After our chat about the topic I thought that I only needed some minor
>>>> changes,
>>>> was quite wrong.
>>>>
>>>> I moved the flush listener to the usual FullTextIndexEventListener,
>>>> using
>>>> delegation at first as we had agreed. This got me into troubles with
>>>> the Serialization test of the FullTextEntityManager, I had to adapt
>>>> EventSourceTransactionContext changing a field to transient and
>>>> having the code manage the case in which the values are lost by
>>>> deserialization.
>>>> After this I removed the delegation moving the code to the
>>>> FullTextIndexEventListener
>>>> which resulted in simpler code.
>>>>
>>>> You made me think about what was happening in case of an error during
>>>> the flush processing in the default listener, so I've replaced the Map
>>>> with a combination
>>>> of non-static ThreadLocal with a weak reference to the flushing Session,
>>>> which is checked for == identity to make sure the synch is relevant to
>>>> the same session,
>>>> in case of a previously not cleaned-up flush by the same Thread (as in
>>>> a Map, but I only need
>>>> the last stored value for the current session, and only for the same
>>>> thread).
>>>
>>> I think I don't like the ThreadLocal approach. What is your reasoning for
>>> using a thread local variable? What would make it less compelling if we
>>> were
>>> not using a TL?
>>
>> the two event listeners "on-post-somethingchanged" and "flush" are
>> executed
>> one after the other right away, atomically from a out-of-hibernate-search
>> point
>> of view. In the test code if I put breakpoints in both the "register
>> synch" routing
>> and "find the synch from the flushlistener" thay both happen only when
>> the client code is asking to flush.
>> This made me think the two routines are always being called by the same
>> thread,
>> and there is no chance to begin doing something else between the first
>> operation
>> and the second operation: it's not possible that, for example, the Session
>> is
>> passivated or suspendend (thinking about long running conversations).
>> The only way to stop the second to happen is having
>> an exception in the default hibernate flush listener: that's the
>> reason for the weak
>> reference to Session.
>> These thoughts leaded me to think that a concurrent map is overkill,
>> especially
>> considering that every thread in the application is to access
>> this map twice per flush: to put in an object, and get it back
>> immediately after that,
>> and no thread trying to say anything to the others or wanting to store
>> it for a longer
>> time. So I thought the ThreadLocal was the natural answer.
>> Still the resulting code is ugly, I'm happy to forget my performance
>> thoughts if we
>> could get with a cleaner solution.
>> I've another version ready using a WeakIdentityHashMap, it's also
>> passing the tests,
>> and is a bit more readable, but I'm unsure about which is better.
>> Shall I commit the one using WeakIdentityHashMap instead of ThreadLocal?
>
> I looked that the TL impl in Java SE and they are hosted on a per Thread
> object basis, so I guess there is no concurrency lock here. So it all boils
> down to which is faster:
>  - a concurrent map look up access
>  - getting the current Thread object + a map lookup (internal TL variable
> impl)
>
> Up to you. I have the tendency to limit my TL use as it has caused issues in
> the past.
> Remember this part of the code is used only as a workaround for people
> misconfiguring HSearch so I am not too fussed about the perf.
>