Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes all statement leaks in the driver. #455

Merged
merged 6 commits into from
Aug 28, 2017

Conversation

peterbae
Copy link
Contributor

Close all statements in the driver that are initialized locally but are never / sometimes not closed.

Fixes issue #308.

@msftclas
Copy link

@peterbae,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

}
finally {
if (null != orgCat) {
if (null != rs)
rs.close();
Copy link
Contributor

@xiangyushawn xiangyushawn Aug 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this method returns the resultset, so we should not close this resultset here

@@ -1035,6 +1036,10 @@ else if (resultSet != null) {
SQLServerException.makeFromDriverError(connection, this, form.format(msgArgs), null, true);
}
}
finally {
if (null != stmt)
stmt.close();
Copy link
Contributor

@xiangyushawn xiangyushawn Aug 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this statement creates resultset which will be returned, closing this statement also closes the returned resultset, which is not what we want

@codecov-io
Copy link

codecov-io commented Aug 22, 2017

Codecov Report

❗ No coverage uploaded for pull request base (dev@93e087c). Click here to learn what that means.
The diff coverage is 38.09%.

Impacted file tree graph

@@          Coverage Diff           @@
##             dev     #455   +/-   ##
======================================
  Coverage       ?   46.36%           
  Complexity     ?     2202           
======================================
  Files          ?      108           
  Lines          ?    25258           
  Branches       ?     4175           
======================================
  Hits           ?    11710           
  Misses         ?    11529           
  Partials       ?     2019
Flag Coverage Δ Complexity Δ
#JDBC41 46.23% <38.09%> (?) 2193 <0> (?)
#JDBC42 46.16% <38.09%> (?) 2194 <0> (?)
Impacted Files Coverage Δ Complexity Δ
...oft/sqlserver/jdbc/SQLServerPreparedStatement.java 50.91% <ø> (ø) 161 <0> (?)
.../microsoft/sqlserver/jdbc/SQLServerXAResource.java 0% <0%> (ø) 0 <0> (?)
...soft/sqlserver/jdbc/SQLServerDatabaseMetaData.java 29.7% <0%> (ø) 49 <0> (?)
...oft/sqlserver/jdbc/SQLServerParameterMetaData.java 24.46% <62.5%> (ø) 32 <0> (?)
...oft/sqlserver/jdbc/SQLServerCallableStatement.java 29.19% <75%> (ø) 90 <0> (?)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 93e087c...195719e. Read the comment docs.

@codecov-io
Copy link

codecov-io commented Aug 22, 2017

Codecov Report

Merging #455 into dev will increase coverage by 0.03%.
The diff coverage is 32%.

Impacted file tree graph

@@             Coverage Diff              @@
##                dev     #455      +/-   ##
============================================
+ Coverage     46.33%   46.36%   +0.03%     
+ Complexity     2202     2198       -4     
============================================
  Files           108      108              
  Lines         25248    25264      +16     
  Branches       4170     4176       +6     
============================================
+ Hits          11698    11714      +16     
+ Misses        11531    11523       -8     
- Partials       2019     2027       +8
Flag Coverage Δ Complexity Δ
#JDBC41 46.26% <32%> (+0.08%) 2192 <0> (-2) ⬇️
#JDBC42 46.14% <32%> (+0.03%) 2191 <0> (+2) ⬆️
Impacted Files Coverage Δ Complexity Δ
...oft/sqlserver/jdbc/SQLServerPreparedStatement.java 50.82% <0%> (+0.16%) 161 <0> (ø) ⬇️
.../microsoft/sqlserver/jdbc/SQLServerXAResource.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...soft/sqlserver/jdbc/SQLServerDatabaseMetaData.java 29.7% <0%> (ø) 49 <0> (ø) ⬇️
...oft/sqlserver/jdbc/SQLServerParameterMetaData.java 24.35% <71.42%> (+0.37%) 31 <0> (ø) ⬇️
...oft/sqlserver/jdbc/SQLServerCallableStatement.java 29.19% <75%> (+0.03%) 90 <0> (ø) ⬇️
...om/microsoft/sqlserver/jdbc/SimpleInputStream.java 51.11% <0%> (-1.49%) 11% <0%> (-1%)
...m/microsoft/sqlserver/jdbc/SQLServerResultSet.java 32.49% <0%> (-0.37%) 239% <0%> (-3%)
...ncurrentlinkedhashmap/ConcurrentLinkedHashMap.java 42.2% <0%> (-0.22%) 46% <0%> (ø)
.../microsoft/sqlserver/jdbc/SQLServerConnection.java 45.84% <0%> (-0.08%) 274% <0%> (-2%)
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 93e087c...94f8829. Read the comment docs.

…nd added a statement field that can be closed after the prepared statement is closed.
}
finally {
if (null != orgCat) {
if (null != orgCat)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we always omit the bracket when we can, so i did the same here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually we started adding back the brackets even with one line of code, so lets keep the brackets here 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

@@ -563,6 +563,8 @@ private void checkClosed() throws SQLServerException {
assert null != st;
stmtParent = st;
con = st.connection;
SQLServerStatement s = null;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i guess s doesnt need to be instantiated to null but i did it for consistency

@@ -98,6 +98,9 @@

/** The prepared statement handle returned by the server */
private int prepStmtHandle = 0;

/** Statement used for getMetadata(). Declared as a field to facilitate closing the statement. */
private SQLServerStatement internalStmt = null;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

having internalStmt as a field is the only way to close it after the user is done working with the ResultsetMetadata.

@AfsanehR-zz AfsanehR-zz self-assigned this Aug 22, 2017
@cheenamalhotra cheenamalhotra added this to the 6.3.2 milestone Aug 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants