Skip to content

Fixes all statement leaks in the driver.#455

Merged
peterbae merged 6 commits intomicrosoft:devfrom
peterbae:statementLeak
Aug 28, 2017
Merged

Fixes all statement leaks in the driver.#455
peterbae merged 6 commits intomicrosoft:devfrom
peterbae:statementLeak

Conversation

@peterbae
Copy link
Copy Markdown
Contributor

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

Fixes issue #308.

@msftclas
Copy link
Copy Markdown

@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
Copy Markdown
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

}
finally {
if (null != stmt)
stmt.close();
Copy link
Copy Markdown
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
Copy Markdown

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
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done.

assert null != st;
stmtParent = st;
con = st.connection;
SQLServerStatement s = null;
Copy link
Copy Markdown
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

private int prepStmtHandle = 0;

/** Statement used for getMetadata(). Declared as a field to facilitate closing the statement. */
private SQLServerStatement internalStmt = null;
Copy link
Copy Markdown
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.

7 participants