Fix bug: mysql statement leak by z7658329 · Pull Request #1627 · apache/hugegraph

z7658329 · 2021-11-02T03:33:31Z

Fix bug: mysql statement leak

fix #1626

Fix bug: mysql statement leak fix apache#1626

javeme

Looks good, thank you for your contribution.

javeme · 2021-11-02T03:51:19Z

        try {
-            while (!this.results.isClosed() && this.results.next()) {
-                MysqlBackendEntry entry = this.row2Entry(this.results);
+            while (!this.resultSetWrapper.getResultSet().isClosed() && this.resultSetWrapper.getResultSet().next()) {


prefer to add hasNext method to ResultSetWrapper to wrap isClosed() and next()

javeme · 2021-11-02T03:51:52Z

-            while (!this.results.isClosed() && this.results.next()) {
-                MysqlBackendEntry entry = this.row2Entry(this.results);
+            while (!this.resultSetWrapper.getResultSet().isClosed() && this.resultSetWrapper.getResultSet().next()) {
+                MysqlBackendEntry entry = this.row2Entry(this.resultSetWrapper.getResultSet());


rename to next()

javeme · 2021-11-02T03:52:43Z

    protected Iterator<BackendEntry> results2Entries(Query query,
-                                                     ResultSet results) {
-        return new MysqlEntryIterator(results, query, this::mergeEntries);
+                                                     ResultSetWrapper resultSetWrapper) {


keep results name

javeme · 2021-11-02T03:56:58Z

            try {
-                ResultSet resultSet = session.select(select);
+                resultSetWrapper = session.select(select);
+                ResultSet resultSet = resultSetWrapper.getResultSet();


rename to ResultSet rs?

javeme · 2021-11-02T03:57:43Z

        private PaloLoadInfo getLoadInfoByLabel(String label) {
            String sql = String.format("SHOW LOAD WHERE LABEL = '%s'", label);
-            ResultSet result;
+            ResultSetWrapper resultSetWrapper = null;


javeme · 2021-11-02T03:57:49Z

-                if (result.next()) {
-                    return new PaloLoadInfo(result);
+                resultSetWrapper = this.select(sql);
+                ResultSet resultSet = resultSetWrapper.getResultSet();


javeme · 2021-11-02T03:58:38Z

            for (StringBuilder selection : selections) {
-                ResultSet results = session.select(selection.toString());
-                rs.extend(parser.apply(query, results));
+                ResultSetWrapper resultSetWrapper = session.select(selection.toString());


keep results name

javeme · 2021-11-02T03:59:22Z

            }
-            return this.conn.createStatement().execute(sql);
+            Statement statement = null;
+            try {


can use try (Statement statement = this.conn.createStatement())

javeme · 2021-11-02T04:05:05Z

+            } finally {
+                if (resultSetWrapper != null) {
+                    resultSetWrapper.close();
+                }


seems lack of ResultSetWrapper file

Fix bug: mysql statement leak fix apache#1626

codecov · 2021-11-02T07:10:46Z

Codecov Report

Merging #1627 (f2b0c8d) into master (0b80d77) will increase coverage by 0.36%.
The diff coverage is 0.00%.

@@             Coverage Diff              @@
##             master    #1627      +/-   ##
============================================
+ Coverage     62.81%   63.18%   +0.36%     
- Complexity     6691     6715      +24     
============================================
  Files           418      419       +1     
  Lines         34504    34527      +23     
  Branches       4778     4779       +1     
============================================
+ Hits          21674    21816     +142     
+ Misses        10581    10456     -125     
- Partials       2249     2255       +6

Impacted Files	Coverage Δ
...egraph/backend/store/mysql/MysqlEntryIterator.java	`0.00% <0.00%> (ø)`
...u/hugegraph/backend/store/mysql/MysqlSessions.java	`0.00% <0.00%> (ø)`
...aidu/hugegraph/backend/store/mysql/MysqlTable.java	`0.00% <0.00%> (ø)`
...idu/hugegraph/backend/store/mysql/MysqlTables.java	`0.00% <0.00%> (ø)`
...ugegraph/backend/store/mysql/ResultSetWrapper.java	`0.00% <0.00%> (ø)`
.../baidu/hugegraph/backend/query/ConditionQuery.java	`86.38% <0.00%> (+0.27%)`	⬆️
...egraph/backend/store/cassandra/CassandraStore.java	`72.64% <0.00%> (+0.85%)`	⬆️
.../backend/store/cassandra/CassandraSessionPool.java	`57.14% <0.00%> (+1.02%)`	⬆️
...egraph/backend/store/cassandra/CassandraShard.java	`52.77% <0.00%> (+1.85%)`	⬆️
.../backend/store/scylladb/ScyllaDBStoreProvider.java	`94.59% <0.00%> (+94.59%)`	⬆️
... and 2 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 0b80d77...f2b0c8d. Read the comment docs.

javeme · 2021-11-02T07:05:38Z

        try {
-            while (!this.resultSetWrapper.getResultSet().isClosed() && this.resultSetWrapper.getResultSet().next()) {
-                MysqlBackendEntry entry = this.row2Entry(this.resultSetWrapper.getResultSet());
+            while (!this.results.isClosed() && this.results.next()) {


move !this.results.isClosed() into next() method

javeme · 2021-11-02T07:06:19Z

+        return resultSet.next();
+    }
+
+    public boolean isClosed() throws SQLException {


merge into next()

javeme · 2021-11-02T07:06:46Z

+        }
+    }
+
+    public ResultSet getResultSet() {


keep resultSet() style

javeme · 2021-11-02T07:06:57Z

+        return resultSet;
+    }
+
+    public void setResultSet(ResultSet resultSet) {


javeme · 2021-11-02T07:07:08Z

+    public Statement getStatement() {
+        return statement;
+    }
+
+    public void setStatement(Statement statement) {
+        this.statement = statement;
+    }


remove them

javeme · 2021-11-02T07:07:15Z

+    private ResultSet resultSet;
+    private Statement statement;


javeme · 2021-11-02T07:08:30Z

+                Statement statement = this.conn.createStatement();
+                ResultSet rs = statement.executeQuery(sql);
+                return new ResultSetWrapper(rs, statement);
+            } catch (SQLException sqlException) {


SQLException e

javeme · 2021-11-02T07:09:10Z

            assert this.conn.getAutoCommit();
-            return this.conn.createStatement().executeQuery(sql);
+            try {
+                Statement statement = this.conn.createStatement();


move out of try-catch

javeme · 2021-11-02T07:10:09Z

+                ResultSet rs = statement.executeQuery(sql);
+                return new ResultSetWrapper(rs, statement);
+            } catch (SQLException sqlException) {
+                throw sqlException;


close statement if exception occurred when executeQuery()

javeme · 2021-11-02T07:11:07Z

            try {
-                ResultSet resultSet = session.select(select);
+                results = session.select(select);
+                ResultSet resultSet = results.getResultSet();


ResultSet rs

Fix bug: mysql statement leak fix apache#1626

zhoney · 2021-11-02T08:05:01Z

+    public ResultSet resultSet() {
+        return resultSet;
+    }
+


delete empty line

zhoney · 2021-11-02T08:05:49Z

+            }
+        } catch (SQLException e) {
+            throw new BackendException(
+                    "Failed to close resultSet and statement", e);


Failed to close resultSet or Statement

also prefer align " with BackendException in prior line

javeme · 2021-11-02T08:01:47Z

@@ -66,7 +66,7 @@ protected final boolean fetch() {

        try {
            while (!this.results.isClosed() && this.results.next()) {


can remove !this.results.isClosed() && since done in results.next()

javeme · 2021-11-02T08:04:20Z

+            }
+        } catch (SQLException e) {
+            throw new BackendException(
+                    "Failed to close resultSet and statement", e);


align with "new" and keep "ResultSet and Statement"

javeme · 2021-11-02T08:05:52Z

+            if (this.statement != null) {
+                this.statement.close();
+            }


move to finally-block

javeme · 2021-11-02T08:06:11Z

-                if (result.next()) {
-                    return new PaloLoadInfo(result);
+            try (ResultSetWrapper results = this.select(sql)){
+                ResultSet resultSet = results.resultSet();


also keep ResultSet rs

Fix bug: mysql statement leak fix apache#1626

z7658329 added 5 commits November 2, 2021 11:19

Update PaloSessions.java

d470e70

Fix bug: mysql statement leak fix apache#1626

Update MysqlEntryIterator.java

c9bae02

Fix bug: mysql statement leak fix apache#1626

Update MysqlSessions.java

b5c8f5d

Fix bug: mysql statement leak fix apache#1626

Update MysqlTable.java

63a4079

Fix bug: mysql statement leak fix apache#1626

Update MysqlTables.java

a442797

Fix bug: mysql statement leak fix apache#1626

javeme changed the title ~~Bugfix branch~~ Fix bug: mysql statement leak Nov 2, 2021

javeme reviewed Nov 2, 2021

View reviewed changes

z7658329 added 6 commits November 2, 2021 14:52

Update MysqlEntryIterator.java

5e70051

Fix bug: mysql statement leak fix apache#1626

Update MysqlSessions.java

028ab2c

Fix bug: mysql statement leak fix apache#1626

Update MysqlTable.java

27f20fd

Fix bug: mysql statement leak fix apache#1626

Update MysqlTables.java

844869e

Fix bug: mysql statement leak fix apache#1626

Update PaloSessions.java

8e16d87

Fix bug: mysql statement leak fix apache#1626

Create ResultSetWrapper.java

fc330b8

Fix bug: mysql statement leak fix apache#1626

javeme reviewed Nov 2, 2021

View reviewed changes

z7658329 added 6 commits November 2, 2021 15:52

Update ResultSetWrapper.java

323a889

Fix bug: mysql statement leak fix apache#1626

Update PaloSessions.java

8af9fd6

Fix bug: mysql statement leak fix apache#1626

Update MysqlTables.java

635fdc7

Fix bug: mysql statement leak fix apache#1626

Update MysqlEntryIterator.java

7504ec8

Fix bug: mysql statement leak fix apache#1626

Update MysqlSessions.java

c5b17c1

Fix bug: mysql statement leak fix apache#1626

Update MysqlTable.java

8c1c821

Fix bug: mysql statement leak fix apache#1626

zhoney requested changes Nov 2, 2021

View reviewed changes

javeme reviewed Nov 2, 2021

View reviewed changes

z7658329 added 3 commits November 2, 2021 16:22

Update PaloSessions.java

73c06d6

Fix bug: mysql statement leak fix apache#1626

Update MysqlEntryIterator.java

ed82d46

Fix bug: mysql statement leak fix apache#1626

Update ResultSetWrapper.java

f2b0c8d

Fix bug: mysql statement leak fix apache#1626

javeme approved these changes Nov 2, 2021

View reviewed changes

z7658329 requested review from javeme and zhoney November 2, 2021 11:34

zhoney approved these changes Nov 3, 2021

View reviewed changes

imbajin approved these changes Nov 3, 2021

View reviewed changes

javeme merged commit ec81e76 into apache:master Nov 3, 2021

dosubot Bot mentioned this pull request Jul 21, 2026

[Bug] 1.5.0 mysql backend内存泄漏 #3101

Open

1 task

		@@ -66,7 +66,7 @@ protected final boolean fetch() {

		try {
		while (!this.results.isClosed() && this.results.next()) {

Uh oh!

Conversation

z7658329 commented Nov 2, 2021

Uh oh!

javeme left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Nov 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov Bot commented Nov 2, 2021 •

edited

Loading