|- There is a race condition when an Entity is re-entrant. In
|EntityInstanceInterceptor.invoke you set the context's transaction in the
|synchronized(ctx) block rather than in the synchronized(ctx.getTxLock())
|block. Thus it is possible for 2 different threads in 2 different
|transactions to get through the first Tx synchronized block and both do
|ctx.setTransaction(...)
let me make that analysis again and a bit finer
if there is a transaction associated with the ctx and you re-enter with a
different transaction it will lock, so only the thread associated with the
on-going transaction will go through. No race there.
if there is no transaction associated with the ctx and you re-enter from
that context with 2 new and different transactions first of all I am not
sure you can do that (you spawn 2 threads in your EJB? to go and start 2 new
separate transactions to talk to yourself again? wow, you're more twiste
than I thought my friend ;-). But anyway...
both will go through the first block and one will win the ctx
synchronization set the transaction. So the second one goes through and
even though the context is locked gets reentrant status. You agree here that
the more general 2 threads case (not reentrant will work perfectly right? it
will lock on ctx, I am frankly more concerned about that;-). So you come in
and you set the tx... yes, that is correct --in the case there is no
transaction in the ctx.
I believe the solution there is to re-check the tx in ctx inside the else
reentrant block and break with a null context to make a 1 loop (no wait)
back in the synchronized(ctx.getLock()), that will work a wait.
|. I suggest maybe that you put the
|synchronized(ctx)
|block inside the synchronized(ctx.getTxLock()) block. I hope I'm making
|sense here.
the ctx inside ctx.getLock() idea doesn't really work, the problem is that
when you wait you only release the lock you are waiting on (the one
pertaining to the object that got wait()) but doesn't release the others.
That results in a real deadlock in the stack since you never release the ctx
(I know I did it :).
The solution I outlined above should work
In short
synchronized (a){
synchronized (b){
b.wait();
}
}
doesn't result in the thread releasing the a lock (amazing huh?).
|- In EntitytInstanceInterceptor.invoke in the finally block. If an
|exception is thrown, you should do a ctx.getTxLock().notifyAll() as well as
|ctx.notifyAll(). An exception could happen in loadEntity, thus, no Sync
|would be registered with the TM, thus threads waiting on ctx.getTxLock()
|would never be awakened.
that is correct, good catch, forgot pure and simple (that check at line 278
!ctx.isTxSynchronized() that right there tells me "free everyone"). I
didn't catch that in test since we don't test "failure of server db",
interesting... we should, we should nip connections
|- I really think that doing a wait() rather than a wait(5000) is a really
|really bad idea. If a transaction timeout happens, I don't think a thread
|that is in a ctx.wait() or a ctx.getTxLock().wait() will ever wakeup and
|actually do a rollback and release all it's locked entities and resources.
hmmm analyse that again. If someone is waiting and the transaction
times-out (the one waiting) the upon waking up you see that in the loop. If
the transaction in the component times out, well actually that is one of the
bugs I fixed, then you do get notified with a txLock.notifyAll().
what case can go wrong?
|I am VERY unfamiliar with the TM code, but take a look at
|TxCapsule.timedOut(). This method only marks the transaction as ROLLING
|back and does nothing else. I can find no where in the jboss
|codebase where
|Thread.interrupt() is called in relation to transaction timeouts.
|(I'd test
|out this theory myself, but I'm on "vacation" and won't really be able to
|until Monday.)
I am not following you, expand when you come back. A transaction time out
doesn't interupt the thread I don't even think we need to, it wakes up and
tests for timeout.
|I guess there is 2 solutions here. One, have transaction timeouts
|interrupt
|their threads, or Two, have a wait(5000) block in EntityInstanceInterceptor
|as well as:
re: taste wait(5000) or wait(), I actually found the above bug by turning
the wait() :)
take your previous catch, you got it by looking at the code, if not then we
would have gotten it immediately because someone would have said "lock,
freeze". I would rather hear about it early on than not, when it is
stabilized and we go with a production version I have no problem about
putting wait(5000);
again 5000 vs 0 is not an issue for me. It is a good idea to see the
problems (it helps me in debug, my server FREEZES)
| // Maybe my transaction already expired?
| if (mi.getTransaction() != null && mi.getTransaction().getStatus() ==
|Status.STATUS_MARKED_ROLLBACK)
| throw new RuntimeException("Transaction marked for rollback,
|possibly a
|timeout");
we do that test when a thread wakes up, we set the ctx to null and continue
so we go through that test again. Is it something else?
|at the beginning of your while(ctx==null) loop. Actually, it
|would probably
|be best to have a wait(Tx.timeout) instead of just 5000.
wait(2xTx.timeout) is more like it, just to make sure we actually timeout :)
|Anyways, FREEZING, is very very bad in itself. Yeah sure, JBoss might not
|have any deadlocks, but you really want to be able to recover from
|Application deadlock.
you don't get it I am doing it to find the deadlocks in JBoss :) that is how
I worked on the cache, I would wait() and know immediately that I have a
problem.
For example, some bugs where that the ctx values are not correctly set and
if I have a wait(4000) it would keep on cycling and I would never know if it
was because threads were working or not, this is very clear cut, very useful
for debugging.
|- I'll probably open up a different email thread on this, but maybe all the
|locks should be pulled out of EnterpriseContext and be managed by some
|LockManager. If multiple instances for option B and C is ever
|implemented(I'd like to tackle this), they'll have to use a different
|locking mechanism because you wouldn't be able to synchronize on ctx and
|ctx.getTxLock().
yes, that is XP, I am not going to complicate the code until we really need
to. You will notice that I left the lock stuff commented but not removed
for that purpose, it is a very possible we use this in the future, but the
first thing I wanted to get going is the bare bones version.
I wanted to minimize complexity and make the code simpler.
It is still fairly complex as you see :)
|Well, I hope I'm making sense here. It's the 4th and I have a
|belly full of
|beer and BBQ.
hey thanks, I feel I am in a fighter plane and I am working on oxygen
supply, not much to breeze in that code so I am always happy to see a fellow
bomber plane fly by, feel less alone.
Actually good work very useful and I am impressed and there is much more we
can do.
marcf
_______________________________________________
Jboss-development mailing list
[EMAIL PROTECTED]
http://lists.sourceforge.net/lists/listinfo/jboss-development
