Avoid buf copy and fix buf leak #101

jilen · 2019-03-08T02:04:40Z

No description provided.

codecov · 2019-03-08T02:10:14Z

Codecov Report

Merging #101 into master will increase coverage by 0.03%.
The diff coverage is 85.71%.

@@             Coverage Diff              @@
##             master     #101      +/-   ##
============================================
+ Coverage      78.9%   78.94%   +0.03%     
  Complexity     1011     1011              
============================================
  Files           258      258              
  Lines          3655     3661       +6     
  Branches        487      487              
============================================
+ Hits           2884     2890       +6     
  Misses          531      531              
  Partials        240      240

Impacted Files	Coverage Δ	Complexity Δ
...jasync/sql/db/mysql/decoder/ResultSetRowDecoder.kt	`100% <100%> (ø)`	`4 <0> (ø)`	⬇️
...hub/jasync/sql/db/mysql/codec/MySQLFrameDecoder.kt	`91.4% <100%> (ø)`	`37 <0> (ø)`	⬇️
.../jasync/sql/db/postgresql/parsers/DataRowParser.kt	`100% <100%> (ø)`	`4 <0> (ø)`	⬇️
...async/sql/db/mysql/codec/MySQLConnectionHandler.kt	`78.37% <81.81%> (+0.72%)`	`57 <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 ecf5edd...35972d9. Read the comment docs.

andy-yx-chen · 2019-03-08T08:19:19Z

mysql-async/src/main/java/com/github/jasync/sql/db/mysql/codec/MySQLConnectionHandler.kt

@@ -164,7 +164,9 @@ class MySQLConnectionHandler(
                    }
                    ServerMessage.BinaryRow -> {
                        val m = message as BinaryRowMessage
-                        this.currentQuery!!.addRow(this.binaryRowDecoder.decode(m.buffer, this.currentColumns))
+                        val decodedRow = this.binaryRowDecoder.decode(m.buffer, this.currentColumns)
+                        m.buffer.release()


my understanding is you don't need to release the buffer from BytesToMessageDecoder

andy-yx-chen · 2019-03-08T08:24:02Z

mysql-async/src/main/java/com/github/jasync/sql/db/mysql/codec/MySQLFrameDecoder.kt

@@ -222,7 +222,8 @@ class MySQLFrameDecoder(val charset: Charset, private val connectionId: String)

        return if (this.totalColumns == this.processedColumns) {
            if (this.isPreparedStatementExecute) {
-                val row = slice.readBytes(slice.readableBytes())
+                val row = slice.retainedSlice()
+                slice.readerIndex(slice.readerIndex() + slice.readableBytes())


Okay, according to https://netty.io/4.1/api/io/netty/handler/codec/ByteToMessageDecoder.html

Some methods such as ByteBuf.readBytes(int) will cause a memory leak if the returned buffer is not released or added to the out List. Use derived buffers like ByteBuf.readSlice(int) to avoid leaking memory.

we should have this fix, but why not just readSlice? then you don't need to release it in the file above

If not retained, the buf will be released while current handler finished, and cannot be accessed in another handler.

@andy-yx-chen Replaced with readRetainedSlice.

@jilen you are right, please help to check if there are other places that have the same issue here

@oshai , we need you to take a look at this one, good finding actually

oshai · 2019-03-08T19:06:18Z

I will review it as soon as possible. I just wonder why this leak wasn't noticed until now. Will also compare it to mauricio.

andy-yx-chen · 2019-03-08T21:33:28Z

I will review it as soon as possible. I just wonder why this leak wasn't noticed until now. Will also compare it to mauricio.

thanks, I think the fix is valid, I also read the source code for netty, the readBytes will return a buffer from MessageDecoder's ctx.allocator, which is pooled, and need to be explicitly release

oshai · 2019-03-09T21:45:58Z

@jilen Thanks for raising the issue!
I want to be sure that I understand the change: readBytes is not efficient because it is copying the buffer (BTW I found 3 places like that). Instead, you would like to use readRetainedSlice which is a view on the same buffer with it's own ref count thus more efficient.

In addition, the copied buffer was probably leaking. I tried using -Dio.netty.leakDetection.level=paranoid but there wasn't any message. On the other hand checking m.buffer.refCnt() after a while was == 1. If you can bring some more details or ideas how to make sure we can obeserve the leak it will be great.

What I did find is the following:

DataRowMessage was leaking in the past but it was fixed: Fixed ByteBuf leaks mauricio/postgresql-async#181
ResultSetRowMessage.buffer is probably also leaking.

Would be glad to get more input before merging it.

jilen · 2019-03-11T02:10:41Z

@oshai Yes, those bufs could be replaced with readRetainedSlice, and be released later. And netty's leak detection relies on gc. So it might not always be reliable. For me, sometimes it will report leak, while other times it will not.

andy-yx-chen · 2019-03-11T03:56:02Z

again, whether detector reports the leak or not, the fact is readBytes in MessageDecoder will cause leak, as it will allocate a buffer in handler context.alloc, which needs to be release explicitly as the document says

oshai · 2019-03-11T08:02:50Z

@jilen Thanks again for the fix!

andy-yx-chen reviewed Mar 8, 2019

View reviewed changes

jilen force-pushed the fix_buf_leak branch 2 times, most recently from 8c1a8f2 to cefde94 Compare March 11, 2019 02:22

avoid buf copy and fix buf leaks

35972d9

jilen force-pushed the fix_buf_leak branch from cefde94 to 35972d9 Compare March 11, 2019 02:23

oshai merged commit fff9c64 into jasync-sql:master Mar 11, 2019

jilen deleted the fix_buf_leak branch March 11, 2019 09:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid buf copy and fix buf leak #101

Avoid buf copy and fix buf leak #101

jilen commented Mar 8, 2019

codecov bot commented Mar 8, 2019 •

edited

Loading

andy-yx-chen Mar 8, 2019

andy-yx-chen Mar 8, 2019 •

edited

Loading

jilen Mar 8, 2019 •

edited

Loading

jilen Mar 8, 2019

andy-yx-chen Mar 8, 2019

andy-yx-chen Mar 8, 2019

oshai commented Mar 8, 2019

andy-yx-chen commented Mar 8, 2019

oshai commented Mar 9, 2019

jilen commented Mar 11, 2019 •

edited

Loading

andy-yx-chen commented Mar 11, 2019

oshai commented Mar 11, 2019

Avoid buf copy and fix buf leak #101

Avoid buf copy and fix buf leak #101

Conversation

jilen commented Mar 8, 2019

codecov bot commented Mar 8, 2019 • edited Loading

Codecov Report

andy-yx-chen Mar 8, 2019

Choose a reason for hiding this comment

andy-yx-chen Mar 8, 2019 • edited Loading

Choose a reason for hiding this comment

jilen Mar 8, 2019 • edited Loading

Choose a reason for hiding this comment

jilen Mar 8, 2019

Choose a reason for hiding this comment

andy-yx-chen Mar 8, 2019

Choose a reason for hiding this comment

andy-yx-chen Mar 8, 2019

Choose a reason for hiding this comment

oshai commented Mar 8, 2019

andy-yx-chen commented Mar 8, 2019

oshai commented Mar 9, 2019

jilen commented Mar 11, 2019 • edited Loading

andy-yx-chen commented Mar 11, 2019

oshai commented Mar 11, 2019

codecov bot commented Mar 8, 2019 •

edited

Loading

andy-yx-chen Mar 8, 2019 •

edited

Loading

jilen Mar 8, 2019 •

edited

Loading

jilen commented Mar 11, 2019 •

edited

Loading