Changing certain properties on client DataSource objects causes existing connections to reflect the new values

• Key: DERBY-3581
• Type: Bug
• Status: Closed
• Resolution: Fixed
• Priority: Major
• Assignee: Kristian Waagan
• Reporter: Kristian Waagan
• Component/s: JDBC, Network Client
• Affects Version/s: 10.3.2.1, 10.4.1.3, 10.5.1.1
• Fix Version/s: 10.4.2.0, 10.5.1.1

Description

The client driver has code propagating changes made to the DataSource to existing connections created by that DataSource.
There is some discussion of this in the thread http://www.nabble.com/ConnectionPoolDataSource-properties-td15740457.html, and there is also an example of what can happen due to this "feature".

Besides from being a bug with the potential to cause strange errors in deployment, the issue also makes the client driver code harder to read and understand.
As far as I can see, there is also some related code in other parts of the client driver, for instance for "fully" resetting statements. There is mention of dynamic versus static sections in the comments (this one from am.Statement):

    // If a dataSource is passed into resetClientConnection(), then we will assume
    // properties on the dataSource may have changed, and we will need to go through
    // the open-statement list on the connection and do a full reset on all statements,
    // including preparedStatement's and callableStatement's. This is because property
    // change may influence the section we allocate for the preparedStatement, and
    // also the cursor attributes, i.e. setCursorSensitivity().
    // If no dataSource is passed into resetClientConnection(), then we will do the
    // minimum reset required for preparedStatement's and callableStatement's.

Note that the reset code for statements is also invoked when ClientPooledConnection.getConnection() is invoked. I do not understand why we should reset statements when we get a new logical connection.

Further, I also suspect the concept of a deferred reset has been introduced because of the feature/bug described by this Jira issue. A deferred reset seems to be a mechanism to avoid a round-trip to validate the newly changed DataSource properties (typically user, password and security mechanism).
I will look into removing code related to deferred resets as well. If you have historic information about these parts of the driver, I would appreciate if you share it with the community if possible.

Just to be clear, it is my opinion that changed DataSource properties should take effect when one of the following methods is invoked:
 * DataSource.getConnection()
 * ConnectionPoolDataSource.getPooledConnection()
 * XADataSource.getXAConnection()

All of the methods above returns a physical connection. Properties like user name and password are associated with the physical connection, and thus requests to obtain a logical connection should not cause any of these properties to change.

Comments

[Kristian Waagan]

'derby-3581-1a-remove_user_password_iteration1.diff' removes the use of user and password variables when resetting a pooled connection. The only call to the top-level method changed is in ClientPooledConnection.getConnection().

I have run suites.All without failures, and will continue the work by checking if and for what the data source reference is needed when resetting a physical connection.
Note that there is no way to change the user name and/or password except for getting a new connection. One can either change the values held be the creating data source, or once can use getPooledConnection(user, password).

Patch ready for review.
I have started derbyall as well, and will report back if it fails.

[Kristian Waagan]

In the issue description, I wrote the following:
----- Kristian wrote:
Further, I also suspect the concept of a deferred reset has been introduced because of the feature/bug described by this Jira issue. A deferred reset seems to be a mechanism to avoid a round-trip to validate the newly changed DataSource properties (typically user, password and security mechanism).
I will look into removing code related to deferred resets as well. If you have historic information about these parts of the driver, I would appreciate if you share it with the community if possible.
-----

After reading some sections in the DRDA spec, I think that statement is wrong.
Sending what is called the deferred reset sequence in the driver (EXCSAT, ACCSEC, SECCHK and ACCRDB), is specified by the DRDA spec. It says the source server *can* send it if it wants to reuse the connection for another application or transaction, but I haven't found a place where it says it has to send it.
I thought about sending only SECCHK and ACCRDB to reduce the work, but this is not allowed by the DRDA spec (but I think it is allowed by the network server).

My current conclusion is that the deferred reset code still has to be used and cannot be removed. The code related to picking up changes in the data source can however still be removed.
Implementing an alternative "reset protocol" might be possible, but that would be 10.5 or maybe even 11. I'm sure a more efficient scheme can be implemented, but it is not clear to me what the solution space looks like bounded by the DRDA spec.

BTW; derbyall ran cleanly for the 1a patch.

[Kristian Waagan]

Committed 'derby-3581-1a-remove_user_password_iteration1.diff' to trunk with revision 648280.
More patches will follow.

[Kristian Waagan]

'derby-3581-2a-remove_datasource_iteration1.diff' removes the usage of the data source when the connection is reset. The reset still happens when a new logical connection is obtained, so this change would only affect pooled and xa connections.

Further, since the reset method is only ever called with the recomputeFromDataSource variable set to true, I plan to simplify the methods and remove that variable.

Patch ready for review.
J2EEDataSourceTest, XATest and XATransactionTest ran cleanly. I'm running full regression tests.

[Kristian Waagan]

I'm a bit torn on what to do regarding backporting. I know there are bugs in this area that might have to be fixed in the 10.4 branch. On the other hand, these changes are more like cleanup issues.

I'll wait for feedback, if I hear nothing I think I'll backport to 10.4.

[Kristian Waagan]

Committed patch 2a to trunk with revision 649473.
Regression tests ran cleanly (except for derbyrunjartest in derbyall, which also failed before this patch was applied).

[Kristian Waagan]

Backported patches 1a and 2a to 10.4 with revision 663893.
suites.All and derbyall ran successfully (Sun JDK 1.6.0, Solaris 10).

[Kristian Waagan]

'derby-3581-3a-remove_recomputeFromDataSource_flag.diff' removes a flag that is always true.
It serves no purpose any more, and the associated functionality will not be used/supported by the Derby client driver. Adding it again at a later time, for instance if the community wants to create an enterprise level client side pooling agent, should be pretty easy.
The removal also makes the code easier to read and understand.

Regression tests ran without failures (Sun JDK 1.6.0, Solaris 10).
Patch ready for review.

[Knut Anders Hatlen]

This comment in NetConnection.completeReset() also needs to be updated when recomputeFromDataSource is removed (is 3341 a typo, by the way?):

         // NB! Override the recomputFromDataSource flag.
         // This was done as a temporary, minimal intrusive fix to support
         // JDBC statement pooling.
         // See DERBY-3341 for details.

[Kristian Waagan]

'derby-3581-3b-remove_recomputeFromDataSource_flag.diff' also removes the comment (and an unused import).

Thanks for the correction Knut Anders!
I plan to commit patch 3b later today.

[Kristian Waagan]

Committed 'derby-3581-3b-remove_recomputeFromDataSource_flag.diff' to trunk with revision 667568.
I plan to backport it to 10.4 later, because an upcoming fix under DERBY-3723 will touch the same code.

[Kristian Waagan]

Backported patch 3b to the 10.4 branch with revision 672749. The commit also included the patches for DERBY-3723.

[Kristian Waagan]

Don't plan to do any more work on this.
No problems reported.