RE: [JBoss-dev] comments on new instance interceptors and locking
-Original Message- From: [EMAIL PROTECTED] [mailto:[EMAIL PROTECTED]]On Behalf Of marc fleury Sent: Wednesday, July 04, 2001 9:09 PM To: [EMAIL PROTECTED] Subject: RE: [JBoss-dev] comments on new instance interceptors and locking [snip |- 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'll do this on Monday, but if somebody feels the itch, try out the classic deadlock scenario and see if it actually times out and rollsback the transaction. Trans 1: Trans 2: get A get B sleep sleep get B get A I'm very suspicious that it won't rollback and release with the wait() code in there. |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. When the transaction times out, who will wakeup threads in EntityInstanceInterceptor doing a wait()? Again, when a timeout happens, I think the TM only marks the transaction as STATUS_ROLLBACK, and doesn't do any interrupting or do a loop through the transaction's InstanceSynchronizations. I'll try out the classic deadlock scenario on Monday. |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); Oh, ok, so wait() is in there for debugging purposes. |Well, I hope I'm making sense here. It's the 4th and I have a |belly full of |beer and BBQ. Burp! 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. My motivations are purely selfish. This stuff is fun!(Yes, I'm a dork...). Now, I'm really off until Monday, Bill ___ Jboss-development mailing list [EMAIL PROTECTED] http://lists.sourceforge.net/lists/listinfo/jboss-development
RE: [JBoss-dev] comments on new instance interceptors and locking
Excellent! frankly I will try to read tonight, finishing up the stateful stuff (minor rewrite). read real fast: the wait() as opposed to wait(5000), I know :) I do it on purpose, if there is a lock I WANT TO KNOW ABOUT IT. In fact wait(5000) only makes things cosmetically better, a missed notify is bad and we would need to see it. That's all. marcf |-Original Message- |From: [EMAIL PROTECTED] |[mailto:[EMAIL PROTECTED]]On Behalf Of Bill |Burke |Sent: Wednesday, July 04, 2001 4:34 PM |To: Jboss-Development@Lists. Sourceforge. Net |Subject: [JBoss-dev] comments on new instance interceptors and locking | | |Hey Marc, | |I looked over the new Entity Interceptors and locking and have a few |comments | |- 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(...). I suggest maybe that you put the |synchronized(ctx) |block inside the synchronized(ctx.getTxLock()) block. I hope I'm making |sense here. | | |- 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. | |- 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. |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 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: | |// 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); | |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. | |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. | | |- 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(). | | |Well, I hope I'm making sense here. It's the 4th and I have a |belly full of |beer and BBQ. | |Regards, | |Bill | | | |___ |Jboss-development mailing list |[EMAIL PROTECTED] |http://lists.sourceforge.net/lists/listinfo/jboss-development ___ Jboss-development mailing list [EMAIL PROTECTED] http://lists.sourceforge.net/lists/listinfo/jboss-development
RE: [JBoss-dev] comments on new instance interceptors and locking
|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. done in cvs ||- 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 done in cvs ||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. commented last traces of locks in cache, feel free to use back in old version ctxLock instead of ctx as long as the logic is the same shoul be fine keep it coming, marcf ___ Jboss-development mailing list [EMAIL PROTECTED] http://lists.sourceforge.net/lists/listinfo/jboss-development
Re: [JBoss-dev] comments on new instance interceptors and locking
Hi Bill and Marc and all others, Bill: I looked over the new Entity Interceptors and locking and have a few comments My reverence to Bill for this excellent code review. And ditto to Marc for the excellent code to review. Most points you discuss I'm barely able to follow, not to mention suggestions. And the one point 'upsetting' me (not really) 'wait() vs. wait(some_time)' was cleared by Marc: in the testing phase he wants to see the hang, in production code there WILL be a timeout to protect against client introduced deadlock situations. Whats left to me? Just let you know, that I'm watching, what you are doing here, still learning; might be some big brother is watching you too, at last the whole world is looking at that, not literally at every code line, but at JBoss as a very good EJB server and focussing on usability. Marc to Bill: | 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. Hey, Bill actually impresses me as a wingman (?), accompanying you as near as possible having an eye at your back! And remember, if you ever make it back to your base, there are others to feed you! If you get lost in the skyes, may be Bill catches you up. Bill concluded: 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, let's have multiple instances! I'll try to be a usefull member of the groundcrew for that. best regards Georg ___ ___ | + | |__Georg Rehfeld Woltmanstr. 12 20097 Hamburg |_|_\ |___ [EMAIL PROTECTED] +49 (40) 23 53 27 10 ___ Jboss-development mailing list [EMAIL PROTECTED] http://lists.sourceforge.net/lists/listinfo/jboss-development
