Derby
  1. Derby
  2. DERBY-2532

Client does not return SQLException on XAConnection.getXAResource() on a closed connection, Embedded does

    Details

    • Bug behavior facts:
      Embedded/Client difference

      Description

      In the following scenario from converted test DataSourceTest:
      (non-tested code based on the test code)
      ----------------
      ClientXADataSource dsx = new ClientXADataSource();
      dsx.setDatabaseName("tstdb");
      XAConnection xac = dsx.getXAConnection();
      XAConnection xac2 = dsx.getXAConnection();
      XAResource xar2 = xac2.getXAResource();
      xac2.close();

      // allow close on already closed XAConnection
      xac2.close();
      try

      { xac2.getXAResource(); // Network Server does not think this is worth an exception. }

      catch (SQLException sqle)

      { System.out.println("expect a 08003 as with Embedded"); }

      ------------------
      With DerbyNetClient, the xac2.getXAResource() does not return an SQLException.

      This ought to be documented if expected, or fixed.

      1. ClientXAConnection.diff
        0.2 kB
        Sabari S Kumar
      2. Simple.java
        1 kB
        Sabari S Kumar
      3. derby-2532-1a-getxares_on_closed_conn.diff
        2 kB
        Kristian Waagan

        Issue Links

          Activity

          Hide
          Kristian Waagan added a comment -

          Backported to 10.5 with revision 954426 and to 10.6 with revision 954425.

          Show
          Kristian Waagan added a comment - Backported to 10.5 with revision 954426 and to 10.6 with revision 954425.
          Hide
          Kristian Waagan added a comment -

          Committed patch 1a to trunk with revision 951346.

          If I don't get any pushback, I'll backport this simple fix.

          Show
          Kristian Waagan added a comment - Committed patch 1a to trunk with revision 951346. If I don't get any pushback, I'll backport this simple fix.
          Hide
          Kristian Waagan added a comment -

          Wrapped up Sabari's work in 'derby-2532-1a-getxares_on_closed_conn.diff'.
          suites.All passed.

          Patch ready for review, will commit shortly.

          Show
          Kristian Waagan added a comment - Wrapped up Sabari's work in 'derby-2532-1a-getxares_on_closed_conn.diff'. suites.All passed. Patch ready for review, will commit shortly.
          Hide
          Sabari S Kumar added a comment -

          Will submit the new patch in a few days.

          Show
          Sabari S Kumar added a comment - Will submit the new patch in a few days.
          Hide
          Kristian Waagan added a comment -

          Reset the patch available flag, a new patch is expected.

          Show
          Kristian Waagan added a comment - Reset the patch available flag, a new patch is expected.
          Hide
          Sabari S Kumar added a comment -

          Sure, I will do that.
          Thanks a lot for the comment.

          Show
          Sabari S Kumar added a comment - Sure, I will do that. Thanks a lot for the comment.
          Hide
          Kristian Waagan added a comment -

          Thanks for the patch and the repro, Sabari.

          I had a look at the patch and have the following comments;
          a) There is no SQLState associated with the exception being thrown. It would be wise to use the existing framework to throw an exception with the SQLState specified by SQLState.NO_CURRENT_CONNECTION (08003). This would also match the embedded driver. You should find examples of this in the client code, otherwise just ask.

          b) Can you regenerate the patch using 'svn diff'? To make it easier for everyone to apply the patch, this should be done from the root directory of the source tree (i.e. /my/home/trunk).

          c) The community has agreed to use spaces (4) instead of tabs for indentation.

          d) (Tiny nit) If you want to use the full name for physicalConnection, it makes more sense to me to use 'super' instead of 'this'. What do you think?

          Have you run the regression tests? BTW, they do take some hours to run.
          I think the patch will be ready for commit when the mentioned issues have been addressed.

          Regards,

          Show
          Kristian Waagan added a comment - Thanks for the patch and the repro, Sabari. I had a look at the patch and have the following comments; a) There is no SQLState associated with the exception being thrown. It would be wise to use the existing framework to throw an exception with the SQLState specified by SQLState.NO_CURRENT_CONNECTION (08003). This would also match the embedded driver. You should find examples of this in the client code, otherwise just ask. b) Can you regenerate the patch using 'svn diff'? To make it easier for everyone to apply the patch, this should be done from the root directory of the source tree (i.e. /my/home/trunk). c) The community has agreed to use spaces (4) instead of tabs for indentation. d) (Tiny nit) If you want to use the full name for physicalConnection, it makes more sense to me to use 'super' instead of 'this'. What do you think? Have you run the regression tests? BTW, they do take some hours to run. I think the patch will be ready for commit when the mentioned issues have been addressed. Regards,
          Hide
          Myrna van Lunteren added a comment -

          checking the 'patch available' property so committers will get reminders about this patch available for review/commit.

          Show
          Myrna van Lunteren added a comment - checking the 'patch available' property so committers will get reminders about this patch available for review/commit.
          Hide
          Sabari S Kumar added a comment - - edited

          This file[Simple.java] helps to test the issue

          Show
          Sabari S Kumar added a comment - - edited This file [Simple.java] helps to test the issue
          Hide
          Sabari S Kumar added a comment - - edited

          Please test this patch[ClientXAConnection.diff].It seems to resolve the issue.The change has to be made on ClientXAConnection.java
          I have also uploaded a java file to test the issue

          Show
          Sabari S Kumar added a comment - - edited Please test this patch [ClientXAConnection.diff] .It seems to resolve the issue.The change has to be made on ClientXAConnection.java I have also uploaded a java file to test the issue
          Hide
          Sabari S Kumar added a comment -

          Exactly..i just wanted to confirm that this connection closing is not an issue..i was confused with that comment in the problem statement ( // allow close on already closed XAConnection)...

          And now about the "XAConnection.getXAResource() doesn't thrown an exception if the XAConnection is closed" issue:

          ClientXADataSource dsx = new ClientXADataSource();
          XAConnection xac = dsx.getXAConnection();

          Here getXAConnection returns a ClientXAConnection.The getXAResource() method is implemented as:

          public XAResource JavaDoc getXAResource() throws SQLException JavaDoc {
          90 if (logWriter_ != null)

          { 91 logWriter_.traceExit(this, "getXAResource", xares_); 92 }

          93
          94 return xares_;
          95 }

          So will it be fine if we check whether the logical connection is still alive(can be done the very same way as we did in the previous case,ie the repeated connection closing issue),and if not alive we can force an SQLE .

          Show
          Sabari S Kumar added a comment - Exactly..i just wanted to confirm that this connection closing is not an issue..i was confused with that comment in the problem statement ( // allow close on already closed XAConnection)... And now about the "XAConnection.getXAResource() doesn't thrown an exception if the XAConnection is closed" issue: ClientXADataSource dsx = new ClientXADataSource(); XAConnection xac = dsx.getXAConnection(); Here getXAConnection returns a ClientXAConnection.The getXAResource() method is implemented as: public XAResource JavaDoc getXAResource() throws SQLException JavaDoc { 90 if (logWriter_ != null) { 91 logWriter_.traceExit(this, "getXAResource", xares_); 92 } 93 94 return xares_; 95 } So will it be fine if we check whether the logical connection is still alive(can be done the very same way as we did in the previous case,ie the repeated connection closing issue),and if not alive we can force an SQLE .
          Hide
          Kristian Waagan added a comment -

          I'm confused.

          I thought the problem is that XAConnection.getXAResource() doesn't thrown an exception if the XAConnection is closed?
          As far as I understand, it should be allowed to call XAConnection.close() several times, and doing this should not result in an exception.

          So maybe adding a check to see if the connection has been closed in getXAResource would be a fix? This would match the behavior of the embedded driver.

          Show
          Kristian Waagan added a comment - I'm confused. I thought the problem is that XAConnection.getXAResource() doesn't thrown an exception if the XAConnection is closed? As far as I understand, it should be allowed to call XAConnection.close() several times, and doing this should not result in an exception. So maybe adding a check to see if the connection has been closed in getXAResource would be a fix? This would match the behavior of the embedded driver.
          Hide
          Sabari S Kumar added a comment -

          And if u are interested i will attach the full source code...

          Show
          Sabari S Kumar added a comment - And if u are interested i will attach the full source code...
          Hide
          Sabari S Kumar added a comment -

          as per the sourcecode of ClientXAConnection,
          a close() call will call its super.close().
          ie,the close() of ClientPooledConnection class.
          now in CPC's close(),we check if the connection is
          already closed in the log and only if the
          connection is not closed already,it attempts to
          close the connection.Else it simply returns.

          So there is no scope of an SQL exception as
          stated in 2532.

          Now if its required that there shud be an SQLEx
          raised,we can simply do that by forcing an SQLEx
          using the 'throw' keyword, instead of returning normally.

          The current implementation of close function is
          as follows:

          public synchronized void close() throws SQLException JavaDoc {
          150 try
          151 {
          152 if (logWriter_ != null)

          { 153 logWriter_.traceEntry(this, "close"); 154 }
          155
          156 if (logicalConnection_ != null) { 157 logicalConnection_.nullPhysicalConnection(); 158 logicalConnection_ = null; 159 }
          160
          161 if (physicalConnection_ == null) { 162 return; 163 }
          164
          165 // Even if the physcial connection is marked closed (in the pool),
          166 // this will close its underlying resources.
          167 physicalConnection_.closeResources();
          168 }
          169 finally
          170 { 171 physicalConnection_ = null; 172 }
          173 }
          174





          So a possible fix is:


          add a snippet:
          if(logicalConnection_ == null) { throw new SQLException("Connection already closed"); }

          as shown:


          public synchronized void close() throws SQLException JavaDoc {
          150 try
          151 {
          152 if (logWriter_ != null) { 153 logWriter_.traceEntry(this, "close"); 154 }

          if(logicalConnection_ == null)

          { throw new SQLException("Connection already closed"); }

          155
          156 if (logicalConnection_ != null)

          { 157 logicalConnection_.nullPhysicalConnection(); 158 logicalConnection_ = null; 159 }

          160
          161 if (physicalConnection_ == null)

          { 162 return; 163 }

          164
          165 // Even if the physcial connection is marked closed (in the pool),
          166 // this will close its underlying resources.
          167 physicalConnection_.closeResources();
          168 }
          169 finally
          170

          { 171 physicalConnection_ = null; 172 }

          173 }
          174

          Show
          Sabari S Kumar added a comment - as per the sourcecode of ClientXAConnection, a close() call will call its super.close(). ie,the close() of ClientPooledConnection class. now in CPC's close(),we check if the connection is already closed in the log and only if the connection is not closed already,it attempts to close the connection.Else it simply returns. So there is no scope of an SQL exception as stated in 2532. Now if its required that there shud be an SQLEx raised,we can simply do that by forcing an SQLEx using the 'throw' keyword, instead of returning normally. The current implementation of close function is as follows: public synchronized void close() throws SQLException JavaDoc { 150 try 151 { 152 if (logWriter_ != null) { 153 logWriter_.traceEntry(this, "close"); 154 } 155 156 if (logicalConnection_ != null) { 157 logicalConnection_.nullPhysicalConnection(); 158 logicalConnection_ = null; 159 } 160 161 if (physicalConnection_ == null) { 162 return; 163 } 164 165 // Even if the physcial connection is marked closed (in the pool), 166 // this will close its underlying resources. 167 physicalConnection_.closeResources(); 168 } 169 finally 170 { 171 physicalConnection_ = null; 172 } 173 } 174 So a possible fix is: add a snippet: if(logicalConnection_ == null) { throw new SQLException("Connection already closed"); } as shown: public synchronized void close() throws SQLException JavaDoc { 150 try 151 { 152 if (logWriter_ != null) { 153 logWriter_.traceEntry(this, "close"); 154 } if(logicalConnection_ == null) { throw new SQLException("Connection already closed"); } 155 156 if (logicalConnection_ != null) { 157 logicalConnection_.nullPhysicalConnection(); 158 logicalConnection_ = null; 159 } 160 161 if (physicalConnection_ == null) { 162 return; 163 } 164 165 // Even if the physcial connection is marked closed (in the pool), 166 // this will close its underlying resources. 167 physicalConnection_.closeResources(); 168 } 169 finally 170 { 171 physicalConnection_ = null; 172 } 173 } 174
          Hide
          Kristian Waagan added a comment -

          Calling close on a connection that has already been closed is not a problem for java.sql.Connection, according to the Java SE 6 API JavaDoc:
          "Calling the method close on a Connection object that is already closed is a no-op."

          I couldn't find any other information in the JavaDoc for XAConnection or PooledConnection. I know XAConnection doesn't implement java.sql.Connection, but maybe it can be used as a guideline?

          Show
          Kristian Waagan added a comment - Calling close on a connection that has already been closed is not a problem for java.sql.Connection, according to the Java SE 6 API JavaDoc: "Calling the method close on a Connection object that is already closed is a no-op." I couldn't find any other information in the JavaDoc for XAConnection or PooledConnection. I know XAConnection doesn't implement java.sql.Connection, but maybe it can be used as a guideline?
          Hide
          Sabari S Kumar added a comment -

          do you think allowing a close on an already closed connection is also an issue?

          Show
          Sabari S Kumar added a comment - do you think allowing a close on an already closed connection is also an issue?
          Hide
          Kathey Marsden added a comment -

          accidentally closed wrong issue

          Show
          Kathey Marsden added a comment - accidentally closed wrong issue
          Hide
          Kathey Marsden added a comment -

          Duplicate of DERBY-1018

          Show
          Kathey Marsden added a comment - Duplicate of DERBY-1018

            People

            • Assignee:
              Sabari S Kumar
              Reporter:
              Myrna van Lunteren
            • Votes:
              1 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development