Do not null the actual connection when delaying close. That means whe… #1022

tomjenkinson · 2016-06-15T15:40:49Z

https://issues.jboss.org/browse/JBTM-2676

!BLACKTIE !XTS !PERF

nmcl · 2016-06-18T13:51:47Z

I've noticed a few commits like this which don't appear to have associated JIRAs. Am I missing something?

tomjenkinson · 2016-06-18T21:22:32Z

Generally they are likely to be speculative ones. Occasionally "CI" ones might not have a Jira (i.e. the ones that AMS2 runs for us).

This one in particular has not been cleared for release into the code as I am not 100% on the strategy yet. I am just running CI on it to verify it does not break anything.

nmcl · 2016-06-20T14:00:41Z

OK. Was looking for a JIRA which might explain what the issue was. What's the plan to address that aspect, i.e., speculative issues which don't (appear to) have descriptions on the (speculative) issue and hence can't solicit input from the wider team/community? Or would the plan be to escalate to a full JIRA if going beyond speculation and then asking for input?

tomjenkinson · 2016-06-20T14:47:23Z

Precisely. At the moment I am working out if the change that makes the other project work, breaks our CI. Then I will look more closely at the change to see if it makes sense to make the change or make the dependent project change (i.e. is a bug in Narayana, enhancement in Narayana or bug in the third party project). Finally I will either create a forum post or an issue (depending on whether its an enhancement, bug or whatever).

For now I have just added the "hold" label to show this incomplete state - I should have done that before.

jbosstm-bot · 2016-09-23T08:49:28Z

Started testing this pull request with QA_JTA profile: http://narayanaci1.eng.hst.ams2.redhat.com/job/btny-pulls-narayana-jdk8/PROFILE=QA_JTA,jdk=jdk8.latest,label=linux/6/

jbosstm-bot · 2016-09-23T08:49:41Z

Started testing this pull request with MAIN profile: http://narayanaci1.eng.hst.ams2.redhat.com/job/btny-pulls-narayana-jdk8/PROFILE=MAIN,jdk=jdk8.latest,label=linux/6/

jbosstm-bot · 2016-09-23T08:49:43Z

Started testing this pull request with QA_JTS_JDKORB profile: http://narayanaci1.eng.hst.ams2.redhat.com/job/btny-pulls-narayana-jdk8/PROFILE=QA_JTS_JDKORB,jdk=jdk8.latest,label=linux/6/

jbosstm-bot · 2016-09-23T09:08:27Z

http://narayanaci1.eng.hst.ams2.redhat.com/job/btny-pulls-narayana-jdk8/PROFILE=MAIN,jdk=jdk8.latest,label=linux/6/ using non-default AS_BRANCH:

jbosstm-bot · 2016-09-23T09:20:35Z

MAIN profile tests passed - Job complete http://narayanaci1.eng.hst.ams2.redhat.com/job/btny-pulls-narayana-jdk8/PROFILE=MAIN,jdk=jdk8.latest,label=linux/6/

jbosstm-bot · 2016-09-23T10:50:07Z

QA_JTA profile tests passed - Job complete http://narayanaci1.eng.hst.ams2.redhat.com/job/btny-pulls-narayana-jdk8/PROFILE=QA_JTA,jdk=jdk8.latest,label=linux/6/

jbosstm-bot · 2016-09-23T13:41:03Z

QA_JTS_JDKORB profile tests passed - Job complete http://narayanaci1.eng.hst.ams2.redhat.com/job/btny-pulls-narayana-jdk8/PROFILE=QA_JTS_JDKORB,jdk=jdk8.latest,label=linux/6/

gytis · 2016-09-23T15:30:24Z

ArjunaJTA/jdbc/tests/classes/com/hp/mwtests/ts/jdbc/basic/ConnectionImpleTest.java

@@ -125,13 +124,14 @@ public void beforeCompletion() { }
            public void afterCompletion(int status) {
                try {
                    connectionToTest.isClosed();
+                    connectionToTest.close();


Not sure about that. This test is specifically for isClosed method. Close method is tested by closeConnectionInAfterCompletion.
I did that on purpose to make test units as small as possible.

Sure, but its part of spec that you have to call connection.close if you call getConnection. I can move it out of the verify I think

Yeah. I can. I will move it down so it doesn't look wrong for the test

But connection wasn't really opened. xaDataSource is a mock, so instead of creating a connection it returns a mock connection.
Also message in the catch block is a bit misleading since you've removed close statement.

I updated the message.
RE connection.close() I am not sure if you are saying it doesn't matter because we are using a mock in which case I would disagree because someone might change it from a mock to a real connection in the future and then it would break. OTOH aybe you are saying even with a real connection you would not need the .close in which case I can remove it.
I do see from the rest of your tests that you do have the .close()

OK, I will just add the comment then

Hmm, as you can see from the diff I have made the change you requested to not close connection however in doing so there is an issue that the close functionality is not executed:
https://github.com/tomjenkinson/narayana/blob/bcc7c01599e46fcd5e5412ce9277bda62146ddb0/ArjunaJTA/jdbc/classes/com/arjuna/ats/internal/jdbc/ConnectionImple.java#L388

Did you have change to review this yet? I wonder about if adding the .close would also be useful if we in the future added pooling to ConnectionImpl. By not calling .close we would leak the connection not just from ConnectionManager but also this hypothetical pool.

Thanks!

Sorry, I didn't get a notification for some reason...
We would leak in the real application. But this is a unit test which doesn't use real connections. There is nothing to leak.
I'm not sure what you mean by close not being executed. Why would isClose execute it? You should check for closeImpl execution in closeConnectionInAfterCompletion test.

Hi gytis, I don't think I am being clear here. The connection we would leak would be a ConnectionImpl.

But lets not talk hypothetical. If this test does not call connection.close() then the following line is not called: https://github.com/tomjenkinson/narayana/blob/bcc7c01599e46fcd5e5412ce9277bda62146ddb0/ArjunaJTA/jdbc/classes/com/arjuna/ats/internal/jdbc/ConnectionImple.java#L388

gytis · 2016-09-23T15:30:59Z

...sses/com/arjuna/ats/internal/jdbc/drivers/modifiers/SupportsMultipleConnectionsModifier.java

+ * own implementation!
+ */
+
+public class SupportsMultipleConnectionsModifier implements XAModifier, ConnectionModifier {


Here and in some other places tab is used to indent rather than a space.

Well, we don't normally worry about the code format but I will reformat with IntelliJ and see how that comes out

tomjenkinson · 2016-09-23T15:53:47Z

I just internationalized the log messages too

jbosstm-bot · 2016-09-23T16:00:38Z

Started testing this pull request with QA_JTS_JDKORB profile: http://narayanaci1.eng.hst.ams2.redhat.com/job/btny-pulls-narayana-jdk8/PROFILE=QA_JTS_JDKORB,jdk=jdk8.latest,label=linux/11/

jbosstm-bot · 2016-09-23T16:00:54Z

Started testing this pull request with MAIN profile: http://narayanaci1.eng.hst.ams2.redhat.com/job/btny-pulls-narayana-jdk8/PROFILE=MAIN,jdk=jdk8.latest,label=linux/11/

jbosstm-bot · 2016-09-23T16:00:57Z

Started testing this pull request with QA_JTA profile: http://narayanaci1.eng.hst.ams2.redhat.com/job/btny-pulls-narayana-jdk8/PROFILE=QA_JTA,jdk=jdk8.latest,label=linux/11/

jbosstm-bot · 2016-09-23T16:19:47Z

http://narayanaci1.eng.hst.ams2.redhat.com/job/btny-pulls-narayana-jdk8/PROFILE=MAIN,jdk=jdk8.latest,label=linux/11/ using non-default AS_BRANCH:

jbosstm-bot · 2016-09-23T16:32:12Z

MAIN profile tests passed - Job complete http://narayanaci1.eng.hst.ams2.redhat.com/job/btny-pulls-narayana-jdk8/PROFILE=MAIN,jdk=jdk8.latest,label=linux/11/

jbosstm-bot · 2016-09-23T17:59:44Z

QA_JTA profile tests passed - Job complete http://narayanaci1.eng.hst.ams2.redhat.com/job/btny-pulls-narayana-jdk8/PROFILE=QA_JTA,jdk=jdk8.latest,label=linux/11/

jbosstm-bot · 2016-09-23T20:51:28Z

QA_JTS_JDKORB profile tests passed - Job complete http://narayanaci1.eng.hst.ams2.redhat.com/job/btny-pulls-narayana-jdk8/PROFILE=QA_JTS_JDKORB,jdk=jdk8.latest,label=linux/11/

jbosstm-bot · 2016-09-25T20:46:27Z

Started testing this pull request with QA_JTA profile: http://narayanaci1.eng.hst.ams2.redhat.com/job/btny-pulls-narayana-jdk8/PROFILE=QA_JTA,jdk=jdk8.latest,label=linux/13/

jbosstm-bot · 2016-09-25T20:46:35Z

Started testing this pull request with QA_JTS_JDKORB profile: http://narayanaci1.eng.hst.ams2.redhat.com/job/btny-pulls-narayana-jdk8/PROFILE=QA_JTS_JDKORB,jdk=jdk8.latest,label=linux/13/

jbosstm-bot · 2016-09-25T20:46:40Z

Started testing this pull request with MAIN profile: http://narayanaci1.eng.hst.ams2.redhat.com/job/btny-pulls-narayana-jdk8/PROFILE=MAIN,jdk=jdk8.latest,label=linux/13/

jbosstm-bot · 2016-09-25T21:05:31Z

http://narayanaci1.eng.hst.ams2.redhat.com/job/btny-pulls-narayana-jdk8/PROFILE=MAIN,jdk=jdk8.latest,label=linux/13/ using non-default AS_BRANCH:

jbosstm-bot · 2016-09-25T21:17:20Z

MAIN profile tests passed - Job complete http://narayanaci1.eng.hst.ams2.redhat.com/job/btny-pulls-narayana-jdk8/PROFILE=MAIN,jdk=jdk8.latest,label=linux/13/

jbosstm-bot · 2016-09-25T22:35:32Z

QA_JTA profile tests passed - Job complete http://narayanaci1.eng.hst.ams2.redhat.com/job/btny-pulls-narayana-jdk8/PROFILE=QA_JTA,jdk=jdk8.latest,label=linux/13/

jbosstm-bot · 2016-09-26T01:36:36Z

QA_JTS_JDKORB profile tests passed - Job complete http://narayanaci1.eng.hst.ams2.redhat.com/job/btny-pulls-narayana-jdk8/PROFILE=QA_JTS_JDKORB,jdk=jdk8.latest,label=linux/13/

tomjenkinson · 2016-09-27T10:26:57Z

As agreed on HipChat I have made the change to use a ConnectionImple (rather than java.sql.Connection) and @gytis agreed I should merge it after that.

tomjenkinson force-pushed the dontnullconnection branch from 46a955c to cbfc39b Compare June 15, 2016 21:52

tomjenkinson added the Hold label Jun 18, 2016

tomjenkinson force-pushed the dontnullconnection branch from cbfc39b to 609231e Compare June 20, 2016 19:00

tomjenkinson self-assigned this Jul 12, 2016

tomjenkinson force-pushed the dontnullconnection branch from 609231e to 57ec140 Compare September 20, 2016 09:41

tomjenkinson removed the Hold label Sep 20, 2016

tomjenkinson force-pushed the dontnullconnection branch 6 times, most recently from 2912b58 to 075c7c8 Compare September 23, 2016 08:42

tomjenkinson added the Ready For Review label Sep 23, 2016

tomjenkinson assigned gytis and unassigned tomjenkinson Sep 23, 2016

gytis suggested changes Sep 23, 2016

View reviewed changes

gytis removed the Ready For Review label Sep 23, 2016

gytis assigned tomjenkinson and unassigned gytis Sep 23, 2016

tomjenkinson force-pushed the dontnullconnection branch from 2a7f56b to f541be8 Compare September 23, 2016 15:53

tomjenkinson assigned gytis and unassigned tomjenkinson Sep 23, 2016

tomjenkinson added the Ready For Review label Sep 23, 2016

tomjenkinson force-pushed the dontnullconnection branch 2 times, most recently from 38ddc13 to bcc7c01 Compare September 25, 2016 20:43

JBTM-2676 JDBC close handling

d2ac4aa

tomjenkinson force-pushed the dontnullconnection branch from bcc7c01 to d2ac4aa Compare September 27, 2016 10:23

tomjenkinson added Ready for Merge and removed Ready For Review labels Sep 27, 2016

tomjenkinson assigned mmusgrov and unassigned gytis Sep 27, 2016

tomjenkinson merged commit 2cf35fa into jbosstm:master Sep 27, 2016

tomjenkinson deleted the dontnullconnection branch November 8, 2016 14:43

Do not null the actual connection when delaying close. That means whe… #1022

Do not null the actual connection when delaying close. That means whe… #1022

Conversation

tomjenkinson commented Jun 15, 2016 • edited

nmcl commented Jun 18, 2016

tomjenkinson commented Jun 18, 2016

nmcl commented Jun 20, 2016

tomjenkinson commented Jun 20, 2016

jbosstm-bot commented Sep 23, 2016

jbosstm-bot commented Sep 23, 2016

jbosstm-bot commented Sep 23, 2016

jbosstm-bot commented Sep 23, 2016

jbosstm-bot commented Sep 23, 2016

jbosstm-bot commented Sep 23, 2016

jbosstm-bot commented Sep 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomjenkinson commented Sep 23, 2016

jbosstm-bot commented Sep 23, 2016

jbosstm-bot commented Sep 23, 2016

jbosstm-bot commented Sep 23, 2016

jbosstm-bot commented Sep 23, 2016

jbosstm-bot commented Sep 23, 2016

jbosstm-bot commented Sep 23, 2016

jbosstm-bot commented Sep 23, 2016

jbosstm-bot commented Sep 25, 2016

jbosstm-bot commented Sep 25, 2016

jbosstm-bot commented Sep 25, 2016

jbosstm-bot commented Sep 25, 2016

jbosstm-bot commented Sep 25, 2016

jbosstm-bot commented Sep 25, 2016

jbosstm-bot commented Sep 26, 2016

tomjenkinson commented Sep 27, 2016

tomjenkinson commented Jun 15, 2016 •

edited