Fix bug: mysql statement leak #1627

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.

hugegraph-mysql/src/main/java/com/baidu/hugegraph/backend/store/mysql/MysqlEntryIterator.java

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

hugegraph-mysql/src/main/java/com/baidu/hugegraph/backend/store/mysql/MysqlEntryIterator.java

@@ -65,8 +65,8 @@ protected final boolean fetch() {
        }

        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

hugegraph-mysql/src/main/java/com/baidu/hugegraph/backend/store/mysql/MysqlEntryIterator.java

-            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

hugegraph-mysql/src/main/java/com/baidu/hugegraph/backend/store/mysql/MysqlTable.java

@@ -672,8 +672,8 @@ protected void wrapOffset(StringBuilder select, Query query) {
    }

    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

hugegraph-mysql/src/main/java/com/baidu/hugegraph/backend/store/mysql/MysqlTables.java

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


rename to ResultSet rs?

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

hugegraph-palo/src/main/java/com/baidu/hugegraph/backend/store/palo/PaloSessions.java

@@ -196,21 +197,22 @@ private PaloFile getOrCreate(String table) {
        @SuppressWarnings("unused")
        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

hugegraph-palo/src/main/java/com/baidu/hugegraph/backend/store/palo/PaloSessions.java

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


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

hugegraph-mysql/src/main/java/com/baidu/hugegraph/backend/store/mysql/MysqlTable.java

@@ -364,8 +364,8 @@ public Number queryNumber(Session session, Query query) {
        List<StringBuilder> selections = this.query2Select(this.table(), query);
        try {
            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

hugegraph-mysql/src/main/java/com/baidu/hugegraph/backend/store/mysql/MysqlSessions.java

@@ -486,7 +493,17 @@ public boolean execute(String sql) throws SQLException {
            if (!this.conn.getAutoCommit()) {
                this.end();
            }
-            return this.conn.createStatement().execute(sql);
+            Statement statement = null;
+            try {


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

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

hugegraph-palo/src/main/java/com/baidu/hugegraph/backend/store/palo/PaloSessions.java

+            } 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

hugegraph-mysql/src/main/java/com/baidu/hugegraph/backend/store/mysql/MysqlEntryIterator.java

@@ -65,8 +65,8 @@ protected final boolean fetch() {
        }

        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

hugegraph-mysql/src/main/java/com/baidu/hugegraph/backend/store/mysql/ResultSetWrapper.java

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


merge into next()

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

hugegraph-mysql/src/main/java/com/baidu/hugegraph/backend/store/mysql/ResultSetWrapper.java

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


keep resultSet() style

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

hugegraph-mysql/src/main/java/com/baidu/hugegraph/backend/store/mysql/ResultSetWrapper.java

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


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

hugegraph-mysql/src/main/java/com/baidu/hugegraph/backend/store/mysql/ResultSetWrapper.java

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


remove them

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

hugegraph-mysql/src/main/java/com/baidu/hugegraph/backend/store/mysql/ResultSetWrapper.java

+    private ResultSet resultSet;
+    private Statement statement;


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

hugegraph-mysql/src/main/java/com/baidu/hugegraph/backend/store/mysql/MysqlSessions.java

+                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

hugegraph-mysql/src/main/java/com/baidu/hugegraph/backend/store/mysql/MysqlSessions.java

            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

hugegraph-mysql/src/main/java/com/baidu/hugegraph/backend/store/mysql/MysqlSessions.java

+                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

hugegraph-mysql/src/main/java/com/baidu/hugegraph/backend/store/mysql/MysqlTables.java

            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

hugegraph-mysql/src/main/java/com/baidu/hugegraph/backend/store/mysql/ResultSetWrapper.java

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


delete empty line

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

hugegraph-mysql/src/main/java/com/baidu/hugegraph/backend/store/mysql/ResultSetWrapper.java

+            }
+        } 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

hugegraph-mysql/src/main/java/com/baidu/hugegraph/backend/store/mysql/MysqlEntryIterator.java

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

hugegraph-mysql/src/main/java/com/baidu/hugegraph/backend/store/mysql/ResultSetWrapper.java

+            }
+        } 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

hugegraph-mysql/src/main/java/com/baidu/hugegraph/backend/store/mysql/ResultSetWrapper.java

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


move to finally-block

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

hugegraph-palo/src/main/java/com/baidu/hugegraph/backend/store/palo/PaloSessions.java

-                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 zhoney and javeme 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bug: mysql statement leak #1627

Fix bug: mysql statement leak #1627

z7658329 commented Nov 2, 2021

javeme left a comment

javeme Nov 2, 2021

javeme Nov 2, 2021

javeme Nov 2, 2021

javeme Nov 2, 2021

javeme Nov 2, 2021

javeme Nov 2, 2021

javeme Nov 2, 2021

javeme Nov 2, 2021

javeme Nov 2, 2021

codecov bot commented Nov 2, 2021 •

edited

javeme Nov 2, 2021

javeme Nov 2, 2021

javeme Nov 2, 2021

javeme Nov 2, 2021

javeme Nov 2, 2021

javeme Nov 2, 2021

javeme Nov 2, 2021

javeme Nov 2, 2021

javeme Nov 2, 2021

javeme Nov 2, 2021

zhoney Nov 2, 2021

zhoney Nov 2, 2021

javeme Nov 2, 2021

javeme Nov 2, 2021

javeme Nov 2, 2021

javeme Nov 2, 2021

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

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

Fix bug: mysql statement leak #1627

Fix bug: mysql statement leak #1627

Conversation

z7658329 commented Nov 2, 2021

javeme left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 2, 2021 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 2, 2021 •

edited