Improve buffer handling #890

stevenh · 2018-11-09T12:17:44Z

Standardise on cap(..), which is the expected standard for a slice resizing, for all buffer expansion checks.
Eliminate redundant size test in takeBuffer.
Change buffer takeXXX functions to return an error to make it explicit that they can fail.
Add missing error check in handleAuthResult.
Add buffer.store(..) method, used by external buffer consumers to update the raw buffer if they expand it.

Also:

Fix some typo's and unnecessary UTF-8 characters in comments.
Improve buffer function docs.
Add comments to explain some non-obvious behaviour around buffer handling.
Add myself / company to AUTHORS.

Checklist

Code compiles correctly
Created tests which fail without the change (if possible)
All tests passing
Extended the README / documentation, if necessary
Added myself / the copyright holder to the AUTHORS file

methane · 2018-11-12T10:14:32Z

I will not review PRs until 1.4.1 is released, but...

Fix a number of uses of len(..) instead of cap(..) eliminating unnecessary buffer reallocation and invalid assignment of buffers smaller than defaultBufSize, which could cause a panic.

Could you elaborate about "unnecessary buffer reallocation and invalid assignment"?
I don't know any issue for now.

stevenh · 2018-11-12T12:03:05Z

Could you elaborate about "unnecessary buffer reallocation and invalid assignment"?
I don't know any issue for now.

In a number of areas the use of len(buf) instead of cap(buf) results in the code creating a new buffer instead of using the existing one, which is already big enough; this is what I meant by "unnecessary buffer reallocation".

With regards to the "invalid assignment" in the case of writeExecutePacket(...) this actually results in a new slice being created externally which is smaller than defaultBufSize but its also assigned back to buffer. If this is then followed by a call to takeBuffer(...) it can cause a panic for the "cheap" case as it indexes off the end of byte slice. I'm guessing the test was actually meant to be if length <= defaultBufSize && length <= cap(b.buf) { to avoid this but the double test isn't needed if the buffer size is consistently maintained. I tripped over this while playing with a prototype to reuse buffers.

Does that answer your question @methane ?

methane · 2018-11-12T12:17:22Z

In a number of areas the use of len(buf) instead of cap(buf) results in the code creating a new buffer instead of using the existing one, which is already big enough; this is what I meant by "unnecessary buffer reallocation".

Have you really checked allocation? We profiled allocations and confirmed minimum allocation
in typical cases.

With regards to the "invalid assignment" in the case of writeExecutePacket(...) this actually results in a new slice being created externally which is smaller than defaultBufSize but its also assigned back to buffer. If this is then followed by a call to takeBuffer(...) it can cause a panic for the "cheap" case as it indexes off the end of byte slice. I'm guessing the test was actually meant to be if length <= defaultBufSize && length <= cap(b.buf) { to avoid this but the double test isn't needed if the buffer size is consistently maintained. I tripped over this while playing with a prototype to reuse buffers.

Could you demonstrate the panic by code?

This pull request doesn't have test code demonstrates issues fixed by it.

stevenh · 2018-11-12T12:41:53Z

In a number of areas the use of len(buf) instead of cap(buf) results in the code creating a new buffer instead of using the existing one, which is already big enough; this is what I meant by "unnecessary buffer reallocation".

Have you really checked allocation? We profiled allocations and confirmed minimum allocation
in typical cases.

Sorry I don't understand your question there?

With regards to the "invalid assignment" in the case of writeExecutePacket(...) this actually results in a new slice being created externally which is smaller than defaultBufSize but its also assigned back to buffer. If this is then followed by a call to takeBuffer(...) it can cause a panic for the "cheap" case as it indexes off the end of byte slice. I'm guessing the test was actually meant to be if length <= defaultBufSize && length <= cap(b.buf) { to avoid this but the double test isn't needed if the buffer size is consistently maintained. I tripped over this while playing with a prototype to reuse buffers.

Could you demonstrate the panic by code?

This pull request doesn't have test code demonstrates issues fixed by it.

Not going to be the easiest thing to do in a real scenario, but I will give it a shot purely internally which should be easier.

methane · 2018-11-12T12:50:48Z

Sorry I don't understand your question there?

You didn't point out where unnecessary reallocation happens actually.
You can demonstrate reducing allocation by go tool -bench by writing demonstration code.

methane · 2018-11-12T13:00:51Z

In other words, when len(b.buf) < cap(b.buf)?
See this example: https://play.golang.org/p/XWY5pIkhsnm

I think this PR introduces unnecessary complexity.

stevenh · 2018-11-12T14:10:33Z

In other words, when len(b.buf) < cap(b.buf)?
See this example: https://play.golang.org/p/XWY5pIkhsnm

I think this PR introduces unnecessary complexity.

Nope it was more the case that the buffers []byte is replaced with a smaller one externally, so the assumptions made by the buffer didn't hold true.

This was caused by writeExecutePacket append.

This was identified by an internal prototype so it's possible this is actually impossible to trigger, so I'm trying to create an reproduction example as requested.

methane · 2018-11-12T16:40:36Z

This was caused by writeExecutePacket append.

Ah, nice catch.
But I think mc.buf.buf = data[:cap(data)] is enough. Why should we manage both of cap and len?

stevenh · 2018-11-12T17:13:06Z

There are a number reasons which spring to mind:

Consistency currently the code use a mix of len and cap.
Correctness len(buf) is irrelevant when it comes to needing to resize a slice.
Eliminate fragile behaviour, by which I mean assuming len and cap are always equal is far from obvious, as demonstrated by this issue, so it's something which should always be avoided as one small change can cause hard to track down issues such as this.

As a final point, which this currently doesn't fix, I would even go so far as writeExecutePacket shouldn't be modifying the internal slice of buffer. If needed this should be a method on buffer to explicitly grow the buffer.

Just noticed I didn't mention this also fixes the use of uninitialised values within nullMask too.

julienschmidt

Looks really good in general! Thanks!

julienschmidt · 2018-11-12T18:55:16Z

buffer.go

 		// Round up to the next multiple of the default size
 		newBuf := make([]byte, ((need/defaultBufSize)+1)*defaultBufSize)
 		copy(newBuf, b.buf)
 		b.buf = newBuf
+	} else if need > len(b.buf) {


This could be nested instead. need > cap(b.buf) can only be true if also need > len(b.buf) is true.

If I follow where you're going, it will result in an unneeded comparison in the common case len == cap

Is this what you're thinking of?

if need > len(b.buf) { if need > cap(b.buf) { // create larger buffer } else { // expand existing buffer } }

I personally find less nesting is clearer and simpler to reason about.

Yes, except for that it makes an even more common case cheaper: need <= len(b.buf)

julienschmidt · 2018-11-12T18:57:55Z

packets.go

-		if maskLen, typesLen := (len(args)+7)/8, 1+2*len(args); pos+maskLen+typesLen >= len(data) {
+		maskLen, typesLen := (len(args)+7)/8, 1+2*len(args)
+		l := pos + maskLen + typesLen
+		if l > cap(data) {


julienschmidt · 2018-11-12T18:59:55Z

If you can demonstrate an issue (e.g. the aforementioned panic) with the current code, we should consider to include this in v1.4.1, otherwise this should be merged only after the bugfix release.

stevenh · 2018-11-12T22:03:49Z

Closing until I can find a reproduction case.

julienschmidt · 2018-11-12T23:10:23Z

No need to close. This is a great PR and we will merge it sooner or later

methane · 2018-11-13T07:43:24Z

Consistency currently the code use a mix of len and cap.

Correctness len(buf) is irrelevant when it comes to needing to resize a slice.

Only writeExecutePacket is special. All other code uses only len consistently and it is simple and obvious.
So I don't like this PR.

stevenh · 2018-11-13T08:21:28Z

Consistency currently the code use a mix of len and cap.

Correctness len(buf) is irrelevant when it comes to needing to resize a slice.

Only writeExecutePacket is special. All other code uses only len consistently and it is simple and obvious.
So I don't like this PR.

Unfortunately that's not the case even in buffer functions the two are used interchangeably e.g.

methane · 2018-11-13T09:18:37Z

Unfortunately that's not the case even in buffer functions the two are used interchangeably e.g.

And you make it worse.

Please split this PR. You changed too many things at once.

methane · 2018-11-13T09:25:35Z

See #892 which keeps len=cap instead of managing both of len and cap separately.

methane · 2018-11-13T09:35:19Z

@julienschmidt

mysql/packets.go

Lines 1075 to 1080 in 369b5d6

    
           // Check if param values exceeded the available buffer 
        
           // In that case we must build the data packet with the new values buffer 
        
           if valuesCap != cap(paramValues) { 
        
           	data = append(data[:pos], paramValues...) 
        
           	mc.buf.buf = data 
        
           }

valuesCap != cap(paramValues) is true only when len(paramValues) exceededs original capacity.
So buffer smaller than defaultBufSize must not be set here. panic must not happen.

If you want to check "smaller buffer must not be set" explicitly, I think #892 is small enough
to merge before v1.4.1.

stevenh · 2018-11-14T00:28:54Z

Looks like we both had very similar ideas around the direction @methane

I've updated this PR, taking into account everyone's feedback.

Hope this is moving in the right direction.

I've currently kept it all in one PR, as all the changes are buffer related, but I can split it up if people think that's required?

methane · 2018-11-14T12:47:27Z

Two added benchmarks are not make sense to me.

stevenh · 2018-11-14T13:12:08Z

Two added benchmarks are not make sense to me.

Removed

methane · 2018-11-14T13:53:22Z

I'm not happy with errLog.Print(err).
It made sense because caller knew the error is ErrBusyBuffer explicitly.

Now callers doesn't know what is the err.
errLog.Print(ErrBusyBuffer) must be in buffer.go.

methane · 2018-11-14T13:54:56Z

buffer.go

@@ -49,7 +49,7 @@ func (b *buffer) fill(need int) error {
 	// grow buffer if necessary
 	// TODO: let the buffer shrink again at some point
 	//       Maybe keep the org buf slice and swap back?
-	if need > len(b.buf) {
+	if need > cap(b.buf) {


Keep using len here. Read() uses len, not cap.

But the decision to scale the buffer should always cap based not len based, which is one of the reasons I explicitly called out the fact that for buf len == cap

I did have the code which expanded len as a second case but this would never hit due to the above.

I know it. But no reason to use cap here.
Len is consistent with Read.

stevenh · 2018-11-14T15:12:44Z

I'm not happy with errLog.Print(err).
It made sense because caller knew the error is ErrBusyBuffer explicitly.

Now callers doesn't know what is the err.
errLog.Print(ErrBusyBuffer) must be in buffer.go.

I'm missing some context here, it looked like it was logging the error as this is an implementation bug and the original error was being lost hence needed to be logged vs returned. Is that not the case?

methane · 2018-11-15T05:47:23Z

I'm missing some context here, it looked like it was logging the error as this is an implementation bug and the original error was being lost hence needed to be logged vs returned. Is that not the case?

I didn't mean don't log anywhere, I just meant move logging code.
You moved error means busy buffer and context from caller side to takeBuffer() functions. After your change, logging in caller side looks ugly to me.
It violates SSOT.

But I noticed error log prints filename and lineno. Where takeBuffer() called from is useful information. So no need to move logging code.

Now I prefer not adding error return value. Caller must handle error case anyway. Additional return value doesn't abstract anything. It just add more code.

stevenh · 2018-11-16T08:22:00Z

I'm missing some context here, it looked like it was logging the error as this is an implementation bug and the original error was being lost hence needed to be logged vs returned. Is that not the case?

I didn't mean don't log anywhere, I just meant move logging code.
You moved error means busy buffer and context from caller side to takeBuffer() functions. After your change, logging in caller side looks ugly to me.

Logging at all seems questionable, could you explain why its there / what its achieving?

It violates SSOT.
Again without the context of why there is logging in these specific cases vs just returning errors and having the consumer deal with it, it's hard to comment.

But I noticed error log prints filename and lineno. Where takeBuffer() called from is useful information. So no need to move logging code.

Now I prefer not adding error return value. Caller must handle error case anyway. Additional return value doesn't abstract anything. It just add more code.

It adds is visibility and the ability to verify code correctness. The current behaviour is totally unexpected, by which I mean the caller needs to know that the function can return nil and has to deal with that and its not mentioned anywhere.

Adding an error return makes very clear to any consumer that the call can fail. In addition it can be used by linters such errcheck to improve code quality by identifying missing validation, which would have ensured the other missing check for nil never happened.

methane · 2018-11-16T08:53:35Z

It adds is visibility and the ability to verify code correctness. The current behaviour is totally unexpected, by which I mean the caller needs to know that the function can return nil and has to deal with that

I know them at first. But takeBuffer is special. The caller should know they should log the error.
So adding error value can't make it typical function.

and its not mentioned anywhere.

Existing callers had code comment. I agree it may not enough for new contributors. But just adding comment is enough.

Adding an error return makes very clear to any consumer that the call can fail. In addition it can be used by linters such errcheck to improve code quality by identifying missing validation, which would have ensured the other missing check for nil never happened.

Again, caller should log the error, and errcheck can't find missing logging for it.
buffer is lowest level, hacker's only special function. Not public API.

That's why I suggested logging to buffer.go at first. If logging code is moved to buffer.go, takeBuffer can be typical function. But you didn't and keep takeBuffer special function caller should know how it works.

Logging at all seems questionable, could you explain why its there / what its achieving?

takeBuffer fails only when very unexpected cases:

There is race or memory breakage (from C or asm, or hardware error) and all unexpected behavior can happen.
People use connection without closing previous Rows. (Return useful error when attempting to reuse a blocked connection #526)
There is an unknown bug in database/sql or this driver.
Database server sends unknown/unexpected packet (it can be not MySQL, but some server talking MySQL client/server protocol)
Other any unexpected cases.

The logging code is a gentle version of panic() or assertions in other languages.
In many case, we can continue running application safely. Only need to close this connection. That's why we don't use panic here.

I have idea to improve this checking and that's why I prefer not adding error return.

methane · 2018-11-16T09:20:17Z

I'm not good at English. Discussion about high level design with you is much hard and
it's not enjoyable. No need to change more about error value and logging. I will change
it again when I want to do it. Writing better Go code is much easier and joyful.

Change the cap -> len I pointed before. It's only thing you should do before merge.

* Eliminate redundant size test in takeBuffer. * Change buffer takeXXX functions to return an error to make it explicit that they can fail. * Add missing error check in handleAuthResult. * Add buffer.store(..) method which can be used by external buffer consumers to update the raw buffer. Also: * Fix some typo's and unnecessary UTF-8 characters in comments. * Improve buffer function docs. * Add comments to explain some non-obvious behaviour around buffer handling. * Add myself / company to AUTHORS.

stevenh · 2018-11-16T09:27:28Z

I'm not good at English. Discussion about high level design with you is much hard and
it's not enjoyable. No need to change more about error value and logging. I will change
it again when I want to do it. Writing better Go code is much easier and joyful.

Your English is great! I very much appreciate you taking the time to talk about this and would gladly help with changes around logging if you would like?

Change the cap -> len I pointed before. It's only thing you should do before merge.

Done

julienschmidt requested changes Nov 12, 2018

View reviewed changes

stevenh closed this Nov 12, 2018

julienschmidt reopened this Nov 12, 2018

stevenh force-pushed the fix-buffer branch from 6e8c06c to c37e4d7 Compare November 14, 2018 00:14

stevenh changed the title ~~Fix buffer allocation issues~~ Improve buffer handling Nov 14, 2018

julienschmidt approved these changes Nov 14, 2018

View reviewed changes

stevenh force-pushed the fix-buffer branch from c37e4d7 to 10eaa49 Compare November 14, 2018 13:12

methane reviewed Nov 14, 2018

View reviewed changes

stevenh force-pushed the fix-buffer branch from 10eaa49 to d3e790a Compare November 16, 2018 09:25

methane merged commit 6be42e0 into go-sql-driver:master Nov 16, 2018

stevenh deleted the fix-buffer branch November 16, 2018 14:55

julienschmidt added the performance label Apr 24, 2019

julienschmidt added this to the v1.5.0 milestone Apr 24, 2019

zjj mentioned this pull request Aug 13, 2021

enhancement for mysqlConn handleAuthResult #1250

Merged

5 tasks

Improve buffer handling #890

Improve buffer handling #890

Conversation

stevenh commented Nov 9, 2018 • edited

Checklist

methane commented Nov 12, 2018

stevenh commented Nov 12, 2018

methane commented Nov 12, 2018

stevenh commented Nov 12, 2018

methane commented Nov 12, 2018

methane commented Nov 12, 2018

stevenh commented Nov 12, 2018 • edited

methane commented Nov 12, 2018

stevenh commented Nov 12, 2018

julienschmidt left a comment

Choose a reason for hiding this comment

julienschmidt Nov 12, 2018

Choose a reason for hiding this comment

stevenh Nov 12, 2018 • edited

Choose a reason for hiding this comment

julienschmidt Nov 14, 2018

Choose a reason for hiding this comment

julienschmidt Nov 12, 2018

Choose a reason for hiding this comment

julienschmidt commented Nov 12, 2018

stevenh commented Nov 12, 2018

julienschmidt commented Nov 12, 2018

methane commented Nov 13, 2018

stevenh commented Nov 13, 2018 • edited

methane commented Nov 13, 2018

methane commented Nov 13, 2018

methane commented Nov 13, 2018 • edited

stevenh commented Nov 14, 2018 • edited

methane commented Nov 14, 2018

stevenh commented Nov 14, 2018

methane commented Nov 14, 2018

methane Nov 14, 2018

Choose a reason for hiding this comment

stevenh Nov 14, 2018

Choose a reason for hiding this comment

methane Nov 14, 2018

Choose a reason for hiding this comment

stevenh commented Nov 14, 2018

methane commented Nov 15, 2018 • edited

stevenh commented Nov 16, 2018

methane commented Nov 16, 2018

methane commented Nov 16, 2018

stevenh commented Nov 16, 2018

stevenh commented Nov 9, 2018 •

edited

stevenh commented Nov 12, 2018 •

edited

stevenh Nov 12, 2018 •

edited

stevenh commented Nov 13, 2018 •

edited

methane commented Nov 13, 2018 •

edited

stevenh commented Nov 14, 2018 •

edited

methane commented Nov 15, 2018 •

edited