MySQL Bugs: #87429: repeated close of ServerPreparedStatement causes memory leak

Bug #87429	repeated close of ServerPreparedStatement causes memory leak
Submitted:	15 Aug 2017 21:11	Modified:	28 Aug 2017 23:00
Reporter:	Eduard Gurskiy (OCA)	Email Updates:
Status:	Closed	Impact on me:	None
Category:	Connector / J	Severity:	S2 (Serious)
Version:	5.1.43	OS:	Any
Assigned to:		CPU Architecture:	Any

Description:
Actually 2 problems are identified:

- calling close() on ServerPreparedStatement behaves:

   if (this.isCached && isPoolable() && !this.isClosed) {
                clearParameters();
                this.isClosed = true;
                this.connection.recachePreparedStatement(this);
                return;
            }
   realClose(true, true);

So first time you call close() it will go in if {...}, second time - it will not go. Which violates JDBC comment: "Calling the method <code>close</code> on a <code>Statement</code> object that is already closed has no effect."

- more important, that inside if {...} statement gets flag isClosed=true, so second call of close() prevents it from being removed from openedStatements of ConnectionImpl which causes memory leak while it is removed from cache via decachePreparedStatement(). So we get statement which is referenced from openedStatements and absent in statements cache. It will be destroyed only when connection is destroyed which can be too late for pooled connections.

How to repeat:
1. Enable prepared statements caching.
2. Call close() twice on same Statement.
3. Observe that it is not in cache any more but still referenced from ConnectionImpl.openedStatements.

suggested fix

Attachment: Bug_87429.patch (application/octet-stream, text), 1.72 KiB.

This bug becomes really serious when something like StatementFinalized is used, which can close statement for second time and as a result: statement is leaked + statement cache is not working.

Sorry, i meant: org.apache.tomcat.jdbc.pool.interceptor.StatementFinalizer

Hello Eduard Gurskiy,

Thank you for the report and contribution.
In order to submit contributions you must first sign the Oracle Contribution Agreement (OCA). For additional information please check http://www.oracle.com/technetwork/community/oca-486395.html.
If you have any questions, please contact the MySQL community team.

Thanks,
Chiranjeevi.

Hi Chiranjeevi,

What does status 'Verified' mean?
Do you confirm bug or you applied my patch and confirm that it fixes problem?

Wouldn't it faster to apply this patch by you instead of waiting for OCA confirmation?

Thanks.

Looking forward to get it fixed. This issue is really important for us.
Thanks!

https://bugs.mysql.com/bug.php?id=87451 may be a duplicate of this.

manually built fix

Attachment: mysql-connector-java-5.1.43-PATCHED-BUG87429-bin.jar (application/octet-stream, text), 975.83 KiB.

Pull request created on GitHub.

Contribution submitted via Github - Bug 87429 (*) Contribution by Eduard Gurskiy (Github guredd, mysql-connector-j/pull/21#issuecomment-323532497): I confirm the code being submitted is offered under the terms of the OCA, and that I am authorized to contribute it.

Contribution: git_patch_136473027.txt (text/plain), 1.88 KiB.

Bug#87451 is a duplicate of this bug.

Hi Eduard,

Note that the caching of server side prepared statements feature is not a JDBC requirement, as such it is possible that it introduces behaviors that are not expected or described in the API, such as the closing of these statements.

In this case, the expected behavior is to effectively close the statement and de-cache it on the second call to #close(). This is so by design.

This bug should be fixed having that in mind so please don't expect any changes on this regard.

Thank you,

No logic in your words, Filipe. Sorry.
Implementation of JDBC method close() of java.sql.Statement should follow javadoc of its interface and can not violate it.

You can't override requirements of interface in its implementation. Nobody will never understand it.

If you need some logic which will de-cache and close - you should introduce explicit method for this and document it. Adding this logic to repeated execution of existing method which is part of documented JDBC interface - really bad practice.

And again please check code of: "org.apache.tomcat.jdbc.pool.interceptor.StatementFinalizer"
which is widely used as a protection from leaks of statements. It is part of Tomcat.

It closes all intercepted statements.
Taking into account your logic all people who use this interceptor must strictly avoid closing statements in their code to avoid double closed statements and cache misses as a result. Doesn't it sound stupid?

Hi Edward,

I don't disagree with with you but unfortunately I can't change this behavior at the moment, at least not in Connector/J 5.1 series.

This was implemented like so a long time ago under some feature request so that it could work with a specific set of requirements and I can't change it now.

I'd say that the typical use case isn't fully compatible with some external libraries and pool managers but, in this case, the worst that can happen is the client application not being able to take any advantage from the server-side prepared statements caching. In this case it mat be even preferable not to turn it on at all.

I'm really sorry if this doesn't help you much but my main focus ATM must be the memory leak which is causing a different kind of problems.

Thank you,

Posted by developer:
 
Added the following entry to the Connector/J 5.1.44 changelog:

"When using cached server-side prepared statements, a memory leak occurred as references to opened statements were being kept while the statements were being decached; it happened when either the close() method has been called twice on a statement, or when there were conflicting cache entries for a statement and the older entry had not been closed and removed from the opened statement list. This fix makes sure the statements are properly closed in both cases.  Thanks to Chiranjeevi Battula for contributing to the fix."

Posted by developer:
 
Added the changelog entry to the Connector/J 8.0.8 changelog.

Posted by developer:
 
Corrected the changelog entry to the following:

"When using cached server-side prepared statements, a memory leak occurred as
references to opened statements were being kept while the statements were
being decached; it happened when either the close() method has been called
twice on a statement, or when there were conflicting cache entries for a
statement and the older entry had not been closed and removed from the opened
statement list. This fix makes sure the statements are properly closed in
both cases.  Thanks to Eduard Gurskiy for contributing to the fix."