On Apr 14, 2009, at 16:22, Sanne Grinovero wrote:
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.
Yes I know. Ideally we would add that to the Synchornize JavaDoc,
except we can't :)
Add a warning to PostTransactionWorkQueueSynchronization and
IndexWorkFlushEventListener#addSynchronization
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.
I guess we could add a volatile flag marking the activation but i
don't think it is worth the effort.
2009/4/14 Emmanuel Bernard <emman...@hibernate.org>:
On Apr 14, 2009, at 13:03, Sanne Grinovero wrote:
I've 2 inline answers:
2009/4/14 Emmanuel Bernard <emman...@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.
_______________________________________________
hibernate-dev mailing list
hibernate-dev@lists.jboss.org
https://lists.jboss.org/mailman/listinfo/hibernate-dev
