Re: JTA JMS Spec question, connection leakage
Just noticed in JmsConnectionFactoryBuilder this is present already with the attribute "transactionSupport". I need to tie that into my patch before it's merged On Tue, Aug 27, 2019 at 4:30 PM Jonathan S. Fisher wrote: > I just checked Wildfly, they do the same thing as Liberty. I agree with > your statement for the "completely correct" fix, ideally that's the place > to do it, but might take awhile to get a release out. > > On another note: I know the spec says, "Ignore all arguments to > connection.create*(int mode)" methods. Yet I can think of a lot of > scenarios where having a non-JTA connection pool is very handy (for > instance, logging over JMS). We have the option to have non-JTA Database > connections, I feel though we should be able to declare whether or not a > jms connection pool participates in JTA. > > I'm thinking maybe we should have an `xa=true/false` parameter in the > connection pool declaration. Would that be ok? > > > On Tue, Aug 27, 2019 at 3:43 PM David Jencks > wrote: > >> I checked the Open Liberty TransactionSynchronizationRegistry, and it >> interprets “active transaction” to mean “any transaction on the thread, no >> matter it’s state”. So I think that it would be best to decide to do the >> same in the Geronimo TM, deciding that the java doc is ambiguous as to the >> meaning of “active” and the most useful meaning can be picked rather than >> the most literal. >> >> Whether this is practical for the next TomEE, I don’t know. >> >> David Jencks >> >> > On Aug 27, 2019, at 8:25 AM, David Jencks >> wrote: >> > >> > I think the java doc for getResource might have been written >> thoughtlessly, and more appropriate behavior would be an ISE only for >> STATUS_NO_TRANSACTION; literally the geronimo implementation is too lax, as >> “marked rollback” is not status “active”. Is there anyone who’s opinion we >> might ask? >> > >> > I rather thought the “ignore session type” logic was supposed to be put >> into the RA, but I don’t recall if or how I dealt with this in Geronimo. >> > >> > So I’d prefer these issues be dealt with elsewhere but don’t see much >> practical alternative to your implementation. >> > >> > Nice to see someone working on XA:-) >> > >> > thanks! >> > David Jencks >> > >> >> On Aug 26, 2019, at 1:45 PM, Jonathan S. Fisher >> wrote: >> >> >> >> I've narrowed down the problem to AutoConnectionTracker. It's not >> >> completing, which isn't allowing the connections to be returned to the >> pool. >> >> >> https://github.com/apache/tomee/blob/master/container/openejb-core/src/main/java/org/apache/openejb/resource/AutoConnectionTracker.java#L174 >> >> >> >> getResource() is throwing an IllegalStateException. The JavaDoc ( >> >> >> https://docs.oracle.com/javaee/7/api/javax/transaction/TransactionSynchronizationRegistry.html#getResource-java.lang.Object- >> ) >> >> states it should throw an ISE if a current transaction is not Active. >> The >> >> transaction is in the state ROLLED_BACK when AutoConnectionTracker >> tries to >> >> call getResource(). >> >> >> >> I think the Geronimo implementation ( >> >> >> https://github.com/apache/geronimo-txmanager/blame/trunk/geronimo-transaction/src/main/java/org/apache/geronimo/transaction/manager/TransactionManagerImpl.java#L203 >> ) >> >> maybe be a little too strict. The JTA Spec pdf doesn't offer exact >> hints of >> >> which statuses ( >> >> https://docs.oracle.com/javaee/7/api/javax/transaction/Status.html) >> should >> >> be have getResource _not_ throw an ISE unfortunately. I was thinking of >> >> changing Geronimo's implementation to check for anything >> >> but STATUS_UNKNOWN, and STATUS_NO_TRANSACTION. >> >> >> >> The other way is to cast Transaction to the Geronimo implementation >> and use >> >> Geronimo specific APIs to get call getResource(). Do you guys have any >> >> preference which route I should take to fix? >> >> >> >> >> >> On Mon, Aug 26, 2019 at 9:15 AM Jonathan S. Fisher > > >> >> wrote: >> >> >> >>> https://github.com/exabrial/tomee-jms2-bug/tree/connection-pool-leak >> >>> >> >>> Here's a project that reproduces the bug. This project intentionally >> >>> exceeds the transaction timeout (of 1s). Each invocation, the >> connection is >> >>> not returned to the pool and eventually you run out, causing your >> >>> application to freeze. >> >>> >> >>> >> >>> >> >>> On Fri, Aug 23, 2019 at 2:37 PM Jonathan S. Fisher < >> exabr...@gmail.com> >> >>> wrote: >> >>> >> Hello Apache friends :) I have a question about the JTA and JMS/RA >> specs: >> >> If you borrow something from a RA, like a JMS Connection, and you're >> in >> XA Transaction, is it necessary to call connection.close()? It would >> seem >> JTA should be smart enough to know the connection is enrolled for 2 >> phase >> commit and should be smart enough to close it, but I'm not sure if >> that's >> part of the spec. >> >> In TomEE 7.0.6 we're noticing that if you borrow a JMS Connection >> with >>
Re: JTA JMS Spec question, connection leakage
I just checked Wildfly, they do the same thing as Liberty. I agree with your statement for the "completely correct" fix, ideally that's the place to do it, but might take awhile to get a release out. On another note: I know the spec says, "Ignore all arguments to connection.create*(int mode)" methods. Yet I can think of a lot of scenarios where having a non-JTA connection pool is very handy (for instance, logging over JMS). We have the option to have non-JTA Database connections, I feel though we should be able to declare whether or not a jms connection pool participates in JTA. I'm thinking maybe we should have an `xa=true/false` parameter in the connection pool declaration. Would that be ok? On Tue, Aug 27, 2019 at 3:43 PM David Jencks wrote: > I checked the Open Liberty TransactionSynchronizationRegistry, and it > interprets “active transaction” to mean “any transaction on the thread, no > matter it’s state”. So I think that it would be best to decide to do the > same in the Geronimo TM, deciding that the java doc is ambiguous as to the > meaning of “active” and the most useful meaning can be picked rather than > the most literal. > > Whether this is practical for the next TomEE, I don’t know. > > David Jencks > > > On Aug 27, 2019, at 8:25 AM, David Jencks > wrote: > > > > I think the java doc for getResource might have been written > thoughtlessly, and more appropriate behavior would be an ISE only for > STATUS_NO_TRANSACTION; literally the geronimo implementation is too lax, as > “marked rollback” is not status “active”. Is there anyone who’s opinion we > might ask? > > > > I rather thought the “ignore session type” logic was supposed to be put > into the RA, but I don’t recall if or how I dealt with this in Geronimo. > > > > So I’d prefer these issues be dealt with elsewhere but don’t see much > practical alternative to your implementation. > > > > Nice to see someone working on XA:-) > > > > thanks! > > David Jencks > > > >> On Aug 26, 2019, at 1:45 PM, Jonathan S. Fisher > wrote: > >> > >> I've narrowed down the problem to AutoConnectionTracker. It's not > >> completing, which isn't allowing the connections to be returned to the > pool. > >> > https://github.com/apache/tomee/blob/master/container/openejb-core/src/main/java/org/apache/openejb/resource/AutoConnectionTracker.java#L174 > >> > >> getResource() is throwing an IllegalStateException. The JavaDoc ( > >> > https://docs.oracle.com/javaee/7/api/javax/transaction/TransactionSynchronizationRegistry.html#getResource-java.lang.Object- > ) > >> states it should throw an ISE if a current transaction is not Active. > The > >> transaction is in the state ROLLED_BACK when AutoConnectionTracker > tries to > >> call getResource(). > >> > >> I think the Geronimo implementation ( > >> > https://github.com/apache/geronimo-txmanager/blame/trunk/geronimo-transaction/src/main/java/org/apache/geronimo/transaction/manager/TransactionManagerImpl.java#L203 > ) > >> maybe be a little too strict. The JTA Spec pdf doesn't offer exact > hints of > >> which statuses ( > >> https://docs.oracle.com/javaee/7/api/javax/transaction/Status.html) > should > >> be have getResource _not_ throw an ISE unfortunately. I was thinking of > >> changing Geronimo's implementation to check for anything > >> but STATUS_UNKNOWN, and STATUS_NO_TRANSACTION. > >> > >> The other way is to cast Transaction to the Geronimo implementation and > use > >> Geronimo specific APIs to get call getResource(). Do you guys have any > >> preference which route I should take to fix? > >> > >> > >> On Mon, Aug 26, 2019 at 9:15 AM Jonathan S. Fisher > >> wrote: > >> > >>> https://github.com/exabrial/tomee-jms2-bug/tree/connection-pool-leak > >>> > >>> Here's a project that reproduces the bug. This project intentionally > >>> exceeds the transaction timeout (of 1s). Each invocation, the > connection is > >>> not returned to the pool and eventually you run out, causing your > >>> application to freeze. > >>> > >>> > >>> > >>> On Fri, Aug 23, 2019 at 2:37 PM Jonathan S. Fisher > > >>> wrote: > >>> > Hello Apache friends :) I have a question about the JTA and JMS/RA > specs: > > If you borrow something from a RA, like a JMS Connection, and you're > in > XA Transaction, is it necessary to call connection.close()? It would > seem > JTA should be smart enough to know the connection is enrolled for 2 > phase > commit and should be smart enough to close it, but I'm not sure if > that's > part of the spec. > > In TomEE 7.0.6 we're noticing that if you borrow a JMS Connection with > connectionFactory.createConnection(), and your code fails to call > close() > before the transaction completion, the connection leaks. (And > unfortunately, calling close() after the transaction completes doesn't > mitigate the problem). It took awhile for us to track this down. > > This becomes a huge problem if you're calling external services in >
Re: JTA JMS Spec question, connection leakage
I checked the Open Liberty TransactionSynchronizationRegistry, and it interprets “active transaction” to mean “any transaction on the thread, no matter it’s state”. So I think that it would be best to decide to do the same in the Geronimo TM, deciding that the java doc is ambiguous as to the meaning of “active” and the most useful meaning can be picked rather than the most literal. Whether this is practical for the next TomEE, I don’t know. David Jencks > On Aug 27, 2019, at 8:25 AM, David Jencks wrote: > > I think the java doc for getResource might have been written thoughtlessly, > and more appropriate behavior would be an ISE only for STATUS_NO_TRANSACTION; > literally the geronimo implementation is too lax, as “marked rollback” is not > status “active”. Is there anyone who’s opinion we might ask? > > I rather thought the “ignore session type” logic was supposed to be put into > the RA, but I don’t recall if or how I dealt with this in Geronimo. > > So I’d prefer these issues be dealt with elsewhere but don’t see much > practical alternative to your implementation. > > Nice to see someone working on XA:-) > > thanks! > David Jencks > >> On Aug 26, 2019, at 1:45 PM, Jonathan S. Fisher wrote: >> >> I've narrowed down the problem to AutoConnectionTracker. It's not >> completing, which isn't allowing the connections to be returned to the pool. >> https://github.com/apache/tomee/blob/master/container/openejb-core/src/main/java/org/apache/openejb/resource/AutoConnectionTracker.java#L174 >> >> getResource() is throwing an IllegalStateException. The JavaDoc ( >> https://docs.oracle.com/javaee/7/api/javax/transaction/TransactionSynchronizationRegistry.html#getResource-java.lang.Object-) >> states it should throw an ISE if a current transaction is not Active. The >> transaction is in the state ROLLED_BACK when AutoConnectionTracker tries to >> call getResource(). >> >> I think the Geronimo implementation ( >> https://github.com/apache/geronimo-txmanager/blame/trunk/geronimo-transaction/src/main/java/org/apache/geronimo/transaction/manager/TransactionManagerImpl.java#L203) >> maybe be a little too strict. The JTA Spec pdf doesn't offer exact hints of >> which statuses ( >> https://docs.oracle.com/javaee/7/api/javax/transaction/Status.html) should >> be have getResource _not_ throw an ISE unfortunately. I was thinking of >> changing Geronimo's implementation to check for anything >> but STATUS_UNKNOWN, and STATUS_NO_TRANSACTION. >> >> The other way is to cast Transaction to the Geronimo implementation and use >> Geronimo specific APIs to get call getResource(). Do you guys have any >> preference which route I should take to fix? >> >> >> On Mon, Aug 26, 2019 at 9:15 AM Jonathan S. Fisher >> wrote: >> >>> https://github.com/exabrial/tomee-jms2-bug/tree/connection-pool-leak >>> >>> Here's a project that reproduces the bug. This project intentionally >>> exceeds the transaction timeout (of 1s). Each invocation, the connection is >>> not returned to the pool and eventually you run out, causing your >>> application to freeze. >>> >>> >>> >>> On Fri, Aug 23, 2019 at 2:37 PM Jonathan S. Fisher >>> wrote: >>> Hello Apache friends :) I have a question about the JTA and JMS/RA specs: If you borrow something from a RA, like a JMS Connection, and you're in XA Transaction, is it necessary to call connection.close()? It would seem JTA should be smart enough to know the connection is enrolled for 2 phase commit and should be smart enough to close it, but I'm not sure if that's part of the spec. In TomEE 7.0.6 we're noticing that if you borrow a JMS Connection with connectionFactory.createConnection(), and your code fails to call close() before the transaction completion, the connection leaks. (And unfortunately, calling close() after the transaction completes doesn't mitigate the problem). It took awhile for us to track this down. This becomes a huge problem if you're calling external services in your transaction. Let's say you have a reasonable transaction timeout of 30s set. You call three services, and they end up taking 15s a piece. Even if you're doing the right thing and you have connection.close() in a finally block, because your transaction isn't active when you call close, it leaks and it just gets "stuck" as an active connection, which eventually you hit the pool limit and your app freezes. On a separate note, we noticed if you open a connection outside of the scope of a transaction, then start a transaction, then create a session with session_transacted option, the session does not participate in the JTA (which seems out of spec). One most open the connection inside the transaction, AND open the session in the transaction, and close the connection in the transaction for everything to work. I'll get a reproducing project created,
Re: JTA JMS Spec question, connection leakage
Hi Jonathan Thanks for this. I think I had run into some of the things you fixed I little while back. I'll take a look at your change later today. Happy for you to commit and I can review after, too. :) Cheers The other Jonathan On Tue, Aug 27, 2019 at 2:59 AM Jonathan S. Fisher wrote: > > https://github.com/exabrial/tomee/commit/08dd4c744818702f3be5edfd8a1c4cf2b69d524d > > With these patches the JMS2.0 API works pretty well now :) If anyone wants > to review, please go ahead, otherwise I'll commit them tomorrow. > > cheers, > - [The other] Jonathan > > On Mon, Aug 26, 2019 at 3:45 PM Jonathan S. Fisher > wrote: > > > I've narrowed down the problem to AutoConnectionTracker. It's not > > completing, which isn't allowing the connections to be returned to the > pool. > > > > > https://github.com/apache/tomee/blob/master/container/openejb-core/src/main/java/org/apache/openejb/resource/AutoConnectionTracker.java#L174 > > > > getResource() is throwing an IllegalStateException. The JavaDoc ( > > > https://docs.oracle.com/javaee/7/api/javax/transaction/TransactionSynchronizationRegistry.html#getResource-java.lang.Object- > ) > > states it should throw an ISE if a current transaction is not Active. The > > transaction is in the state ROLLED_BACK when AutoConnectionTracker tries > to > > call getResource(). > > > > I think the Geronimo implementation ( > > > https://github.com/apache/geronimo-txmanager/blame/trunk/geronimo-transaction/src/main/java/org/apache/geronimo/transaction/manager/TransactionManagerImpl.java#L203 > ) > > maybe be a little too strict. The JTA Spec pdf doesn't offer exact hints > of > > which statuses ( > > https://docs.oracle.com/javaee/7/api/javax/transaction/Status.html) > > should be have getResource _not_ throw an ISE unfortunately. I was > thinking > > of changing Geronimo's implementation to check for anything > > but STATUS_UNKNOWN, and STATUS_NO_TRANSACTION. > > > > The other way is to cast Transaction to the Geronimo implementation and > > use Geronimo specific APIs to get call getResource(). Do you guys have > any > > preference which route I should take to fix? > > > > > > On Mon, Aug 26, 2019 at 9:15 AM Jonathan S. Fisher > > wrote: > > > >> https://github.com/exabrial/tomee-jms2-bug/tree/connection-pool-leak > >> > >> Here's a project that reproduces the bug. This project intentionally > >> exceeds the transaction timeout (of 1s). Each invocation, the > connection is > >> not returned to the pool and eventually you run out, causing your > >> application to freeze. > >> > >> > >> > >> On Fri, Aug 23, 2019 at 2:37 PM Jonathan S. Fisher > >> wrote: > >> > >>> Hello Apache friends :) I have a question about the JTA and JMS/RA > specs: > >>> > >>> If you borrow something from a RA, like a JMS Connection, and you're in > >>> XA Transaction, is it necessary to call connection.close()? It would > seem > >>> JTA should be smart enough to know the connection is enrolled for 2 > phase > >>> commit and should be smart enough to close it, but I'm not sure if > that's > >>> part of the spec. > >>> > >>> In TomEE 7.0.6 we're noticing that if you borrow a JMS Connection with > >>> connectionFactory.createConnection(), and your code fails to call > close() > >>> before the transaction completion, the connection leaks. (And > >>> unfortunately, calling close() after the transaction completes doesn't > >>> mitigate the problem). It took awhile for us to track this down. > >>> > >>> This becomes a huge problem if you're calling external services in your > >>> transaction. Let's say you have a reasonable transaction timeout of 30s > >>> set. You call three services, and they end up taking 15s a piece. Even > if > >>> you're doing the right thing and you have connection.close() in a > finally > >>> block, because your transaction isn't active when you call close, it > leaks > >>> and it just gets "stuck" as an active connection, which eventually you > hit > >>> the pool limit and your app freezes. > >>> > >>> On a separate note, we noticed if you open a connection outside of the > >>> scope of a transaction, then start a transaction, then create a session > >>> with session_transacted option, the session does not participate in > the JTA > >>> (which seems out of spec). One most open the connection inside the > >>> transaction, AND open the session in the transaction, and close the > >>> connection in the transaction for everything to work. > >>> > >>> I'll get a reproducing project created, but I was curious if anyone > knew > >>> offhand what the spec says. > >>> > >>> cheers, and thanks, > >>> -[the other] Jonathan > >>> > >>> -- > >>> Jonathan | exabr...@gmail.com > >>> Pessimists, see a jar as half empty. Optimists, in contrast, see it as > >>> half full. > >>> Engineers, of course, understand the glass is twice as big as it needs > >>> to be. > >>> > >> > >> > >> -- > >> Jonathan | exabr...@gmail.com > >> Pessimists, see a jar as half empty. Optimists, in contrast, see it as > >> half
Re: JTA JMS Spec question, connection leakage
I think the java doc for getResource might have been written thoughtlessly, and more appropriate behavior would be an ISE only for STATUS_NO_TRANSACTION; literally the geronimo implementation is too lax, as “marked rollback” is not status “active”. Is there anyone who’s opinion we might ask? I rather thought the “ignore session type” logic was supposed to be put into the RA, but I don’t recall if or how I dealt with this in Geronimo. So I’d prefer these issues be dealt with elsewhere but don’t see much practical alternative to your implementation. Nice to see someone working on XA:-) thanks! David Jencks > On Aug 26, 2019, at 1:45 PM, Jonathan S. Fisher wrote: > > I've narrowed down the problem to AutoConnectionTracker. It's not > completing, which isn't allowing the connections to be returned to the pool. > https://github.com/apache/tomee/blob/master/container/openejb-core/src/main/java/org/apache/openejb/resource/AutoConnectionTracker.java#L174 > > getResource() is throwing an IllegalStateException. The JavaDoc ( > https://docs.oracle.com/javaee/7/api/javax/transaction/TransactionSynchronizationRegistry.html#getResource-java.lang.Object-) > states it should throw an ISE if a current transaction is not Active. The > transaction is in the state ROLLED_BACK when AutoConnectionTracker tries to > call getResource(). > > I think the Geronimo implementation ( > https://github.com/apache/geronimo-txmanager/blame/trunk/geronimo-transaction/src/main/java/org/apache/geronimo/transaction/manager/TransactionManagerImpl.java#L203) > maybe be a little too strict. The JTA Spec pdf doesn't offer exact hints of > which statuses ( > https://docs.oracle.com/javaee/7/api/javax/transaction/Status.html) should > be have getResource _not_ throw an ISE unfortunately. I was thinking of > changing Geronimo's implementation to check for anything > but STATUS_UNKNOWN, and STATUS_NO_TRANSACTION. > > The other way is to cast Transaction to the Geronimo implementation and use > Geronimo specific APIs to get call getResource(). Do you guys have any > preference which route I should take to fix? > > > On Mon, Aug 26, 2019 at 9:15 AM Jonathan S. Fisher > wrote: > >> https://github.com/exabrial/tomee-jms2-bug/tree/connection-pool-leak >> >> Here's a project that reproduces the bug. This project intentionally >> exceeds the transaction timeout (of 1s). Each invocation, the connection is >> not returned to the pool and eventually you run out, causing your >> application to freeze. >> >> >> >> On Fri, Aug 23, 2019 at 2:37 PM Jonathan S. Fisher >> wrote: >> >>> Hello Apache friends :) I have a question about the JTA and JMS/RA specs: >>> >>> If you borrow something from a RA, like a JMS Connection, and you're in >>> XA Transaction, is it necessary to call connection.close()? It would seem >>> JTA should be smart enough to know the connection is enrolled for 2 phase >>> commit and should be smart enough to close it, but I'm not sure if that's >>> part of the spec. >>> >>> In TomEE 7.0.6 we're noticing that if you borrow a JMS Connection with >>> connectionFactory.createConnection(), and your code fails to call close() >>> before the transaction completion, the connection leaks. (And >>> unfortunately, calling close() after the transaction completes doesn't >>> mitigate the problem). It took awhile for us to track this down. >>> >>> This becomes a huge problem if you're calling external services in your >>> transaction. Let's say you have a reasonable transaction timeout of 30s >>> set. You call three services, and they end up taking 15s a piece. Even if >>> you're doing the right thing and you have connection.close() in a finally >>> block, because your transaction isn't active when you call close, it leaks >>> and it just gets "stuck" as an active connection, which eventually you hit >>> the pool limit and your app freezes. >>> >>> On a separate note, we noticed if you open a connection outside of the >>> scope of a transaction, then start a transaction, then create a session >>> with session_transacted option, the session does not participate in the JTA >>> (which seems out of spec). One most open the connection inside the >>> transaction, AND open the session in the transaction, and close the >>> connection in the transaction for everything to work. >>> >>> I'll get a reproducing project created, but I was curious if anyone knew >>> offhand what the spec says. >>> >>> cheers, and thanks, >>> -[the other] Jonathan >>> >>> -- >>> Jonathan | exabr...@gmail.com >>> Pessimists, see a jar as half empty. Optimists, in contrast, see it as >>> half full. >>> Engineers, of course, understand the glass is twice as big as it needs to >>> be. >>> >> >> >> -- >> Jonathan | exabr...@gmail.com >> Pessimists, see a jar as half empty. Optimists, in contrast, see it as >> half full. >> Engineers, of course, understand the glass is twice as big as it needs to >> be. >> > > > -- > Jonathan | exabr...@gmail.com >
