From: "Carlo de Wolf" <cdewolf(a)redhat.com&gt; To: jpai(a)redhat.com Cc: jboss-as7-dev(a)lists.jboss.org Sent: Tuesday, October 18, 2011 10:51:24 AM Subject: Re: [jboss-as7-dev] Better handling of transaction leaks?

Yes, I would even say that we need it in the thread pools themselves. Note that it does not have anything to do with cached connections.

Carlo

On 10/18/2011 05:58 PM, Jaikiran Pai wrote: > While looking into a test failure I noticed that currently user > applications can end up leaking transactions (associated with the > invocation thread): > > Servlet { > > doGet() { > UserTransaction.begin(); > doSomeOp(); > fail(); > // no tx rollback/commit > } > } > > Subsequent transactions on that thread can lead to failures and > other > sorts of issues. Looking back at previous versions of AS, we had a > valve > http://anonsvn.jboss.org/repos/jbossas/trunk/tomcat/src/main/java/org/jbo... > which used to cleanup (and log an error) about the leaking > transactions. > > Do we want something similar for AS7? Also, are there other "entry > points" (like servlets for web requests) where such leaks can > happen and > needs to be taken care off? > > -Jaikiran > > _______________________________________________ > jboss-as7-dev mailing list > jboss-as7-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/jboss-as7-dev

_______________________________________________ jboss-as7-dev mailing list jboss-as7-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/jboss-as7-dev

From: "Remy Maucherat" <rmaucher(a)redhat.com&gt; To: jboss-as7-dev(a)lists.jboss.org Sent: Tuesday, October 18, 2011 11:31:04 AM Subject: Re: [jboss-as7-dev] Better handling of transaction leaks?

On Tue, 2011-10-18 at 21:28 +0530, Jaikiran Pai wrote: > While looking into a test failure I noticed that currently user > applications can end up leaking transactions (associated with the > invocation thread): > > Servlet { > > doGet() { > UserTransaction.begin(); > doSomeOp(); > fail(); > // no tx rollback/commit > } > } > > Subsequent transactions on that thread can lead to failures and > other > sorts of issues. Looking back at previous versions of AS, we had a > valve > http://anonsvn.jboss.org/repos/jbossas/trunk/tomcat/src/main/java/org/jbo... > which used to cleanup (and log an error) about the leaking > transactions. > > Do we want something similar for AS7? Also, are there other "entry > points" (like servlets for web requests) where such leaks can > happen and > needs to be taken care off?

JPA has automatic tracking of this sort of things using a valve (and added only when needed).

For the insignificant amount of people that will do manual transactions and can't be bothered to code properly, everyone gets to pay the cost of transaction tracking for all requests to the web container, so I am against this feature.

I remember being flamed pretty badly for daring disabling this "nice" valve by default in AS 6 :) Hopefully it will go better this time.

-- Remy Maucherat <rmaucher(a)redhat.com&gt; Red Hat Inc

_______________________________________________ jboss-as7-dev mailing list jboss-as7-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/jboss-as7-dev

On 10/18/11 12:31 PM, Remy Maucherat wrote: > On Tue, 2011-10-18 at 21:28 +0530, Jaikiran Pai wrote: >> While looking into a test failure I noticed that currently user >> applications can end up leaking transactions (associated with the >> invocation thread): >> >> Servlet { >> >> doGet() { >> UserTransaction.begin(); >> doSomeOp(); >> fail(); >> // no tx rollback/commit >> } >> } >> >> Subsequent transactions on that thread can lead to failures and other >> sorts of issues. Looking back at previous versions of AS, we had a valve >> http://anonsvn.jboss.org/repos/jbossas/trunk/tomcat/src/main/java/org/jbo... >> which used to cleanup (and log an error) about the leaking transactions. >> >> Do we want something similar for AS7? Also, are there other "entry >> points" (like servlets for web requests) where such leaks can happen and >> needs to be taken care off? > > JPA has automatic tracking of this sort of things using a valve (and > added only when needed). > > For the insignificant amount of people that will do manual transactions > and can't be bothered to code properly, everyone gets to pay the cost of > transaction tracking for all requests to the web container, so I am > against this feature. > > I remember being flamed pretty badly for daring disabling this "nice" > valve by default in AS 6 :) Hopefully it will go better this time. > I'll support you (I'd rather have the perf by default). It seems like it should be possible to come up with a fast solution though, instead of using the slow CCV. For example its pretty cheap to check the UT threadlocal and see if the status is still active (hence a leak).

I want to make sure that whatever is done with this stuff, is that it is configurable in such a way that it can be removed for our benchmarking purposes, and performance tuning in general. Even a lightweight check will add overhead that we cannot afford if we are to be competitive with IBM and Oracle on SPECjEnterprise2010. Besides, this is a debugging exercise, not something that should ever really be on in a production environment, in my opinion. Andy ------------------------------------------------------------------------ *From: *"Jaikiran Pai" <jpai(a)redhat.com&gt; *To: *jboss-as7-dev(a)lists.jboss.org *Sent: *Tuesday, October 18, 2011 11:42:30 PM *Subject: *Re: [jboss-as7-dev] Better handling of transaction leaks? Just to be clear, I wasn't in favour of introducing that entire CachedConnectionValve (which definitely is quite heavy). That link was more of a reference to show how it was handled in previous versions. Like you say, it should be easy to check the tx status with a simple check. -Jaikiran On Wednesday 19 October 2011 12:09 AM, Jason T. Greene wrote: > On 10/18/11 12:31 PM, Remy Maucherat wrote: >> On Tue, 2011-10-18 at 21:28 +0530, Jaikiran Pai wrote: >>> While looking into a test failure I noticed that currently user >>> applications can end up leaking transactions (associated with the >>> invocation thread): >>> >>> Servlet { >>> >>> doGet() { >>> UserTransaction.begin(); >>> doSomeOp(); >>> fail(); >>> // no tx rollback/commit >>> } >>> } >>> >>> Subsequent transactions on that thread can lead to failures and other >>> sorts of issues. Looking back at previous versions of AS, we had a valve >>> http://anonsvn.jboss.org/repos/jbossas/trunk/tomcat/src/main/java/org/jbo... >>> which used to cleanup (and log an error) about the leaking transactions. >>> >>> Do we want something similar for AS7? Also, are there other "entry >>> points" (like servlets for web requests) where such leaks can happen and >>> needs to be taken care off? >> >> JPA has automatic tracking of this sort of things using a valve (and >> added only when needed). >> >> For the insignificant amount of people that will do manual transactions >> and can't be bothered to code properly, everyone gets to pay the cost of >> transaction tracking for all requests to the web container, so I am >> against this feature. >> >> I remember being flamed pretty badly for daring disabling this "nice" >> valve by default in AS 6 :) Hopefully it will go better this time. >> > > I'll support you (I'd rather have the perf by default). It seems like it > should be possible to come up with a fast solution though, instead of > using the slow CCV. For example its pretty cheap to check the UT > threadlocal and see if the status is still active (hence a leak). > _______________________________________________ jboss-as7-dev mailing list jboss-as7-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/jboss-as7-dev

On Wed, 2011-10-19 at 19:25 +0530, Jaikiran Pai wrote: > I agree that there should be a way to easily disable this. The only real > reason why I am in favour of this simple check/cleanup is to ensure that > some rogue application (which doesn't handle transactions correctly) > doesn't end up breaking/affecting some other (well-written) application > deployed on the same instance. A rogue application will be able to trivially do something that would affect other applications in the same instance, I believe.

On Wed, 2011-10-19 at 19:29 +0200, Carlo de Wolf wrote: > On 10/19/2011 04:27 PM, Remy Maucherat wrote: >> On Wed, 2011-10-19 at 19:25 +0530, Jaikiran Pai wrote: >>> I agree that there should be a way to easily disable this. The only real >>> reason why I am in favour of this simple check/cleanup is to ensure that >>> some rogue application (which doesn't handle transactions correctly) >>> doesn't end up breaking/affecting some other (well-written) application >>> deployed on the same instance. >> A rogue application will be able to trivially do something that would >> affect other applications in the same instance, I believe. >> > EE.4.2.2 Transaction in Web Component Life Cycles > Returning from the service method to the network client with an active > transaction > context is an error. The web container is required to detect this error > and abort the > transaction. > > How do you propose we implement this? I didn't know it was a spec requirement :) Old legacy item, probably. So you win, congratulations, let's make our product worse.

I agree that there should be a way to easily disable this. The only real reason why I am in favour of this simple check/cleanup is to ensure that some rogue application (which doesn't handle transactions correctly) doesn't end up breaking/affecting some other (well-written) application deployed on the same instance. -Jaikiran On Wednesday 19 October 2011 07:22 PM, Andrig Miller wrote: > I want to make sure that whatever is done with this stuff, is that it > is configurable in such a way that it can be removed for our > benchmarking purposes, and performance tuning in general. > > Even a lightweight check will add overhead that we cannot afford if we > are to be competitive with IBM and Oracle on SPECjEnterprise2010. > Besides, this is a debugging exercise, not something that should ever > really be on in a production environment, in my opinion. > > Andy > > ------------------------------------------------------------------------ > > *From: *"Jaikiran Pai&quot;&lt;jpai(a)redhat.com&gt; > *To: *jboss-as7-dev(a)lists.jboss.org > *Sent: *Tuesday, October 18, 2011 11:42:30 PM > *Subject: *Re: [jboss-as7-dev] Better handling of transaction leaks? > > Just to be clear, I wasn't in favour of introducing that entire > CachedConnectionValve (which definitely is quite heavy). That link > was > more of a reference to show how it was handled in previous versions. > > Like you say, it should be easy to check the tx status with a > simple check. > > -Jaikiran > > On Wednesday 19 October 2011 12:09 AM, Jason T. Greene wrote: > > On 10/18/11 12:31 PM, Remy Maucherat wrote: > >> On Tue, 2011-10-18 at 21:28 +0530, Jaikiran Pai wrote: > >>> While looking into a test failure I noticed that currently user > >>> applications can end up leaking transactions (associated with the > >>> invocation thread): > >>> > >>> Servlet { > >>> > >>> doGet() { > >>> UserTransaction.begin(); > >>> doSomeOp(); > >>> fail(); > >>> // no tx rollback/commit > >>> } > >>> } > >>> > >>> Subsequent transactions on that thread can lead to failures > and other > >>> sorts of issues. Looking back at previous versions of AS, we > had a valve > >>> > http://anonsvn.jboss.org/repos/jbossas/trunk/tomcat/src/main/java/org/jbo... > >>> which used to cleanup (and log an error) about the leaking > transactions. > >>> > >>> Do we want something similar for AS7? Also, are there other "entry > >>> points" (like servlets for web requests) where such leaks can > happen and > >>> needs to be taken care off? > >> > >> JPA has automatic tracking of this sort of things using a valve > (and > >> added only when needed). > >> > >> For the insignificant amount of people that will do manual > transactions > >> and can't be bothered to code properly, everyone gets to pay > the cost of > >> transaction tracking for all requests to the web container, so I am > >> against this feature. > >> > >> I remember being flamed pretty badly for daring disabling this > "nice" > >> valve by default in AS 6 :) Hopefully it will go better this time. > >> > > > > I'll support you (I'd rather have the perf by default). It seems > like it > > should be possible to come up with a fast solution though, > instead of > > using the slow CCV. For example its pretty cheap to check the UT > > threadlocal and see if the status is still active (hence a leak). > > > > _______________________________________________ > jboss-as7-dev mailing list > jboss-as7-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/jboss-as7-dev > > _______________________________________________ jboss-as7-dev mailing list jboss-as7-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/jboss-as7-dev