Skip to content

Replace recursive approach with iterative approach for readPacket() #170

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Nov 13, 2013
Merged

Replace recursive approach with iterative approach for readPacket() #170

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7e9e660
Minor changes, avoid recursive approach in readPacket()
cxmcc Nov 11, 2013
745998a
Remove the buffer copying, no longer necessary
cxmcc Nov 11, 2013
7847296
Avoid buffer copying for non-splitting packets
cxmcc Nov 13, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 34 additions & 35 deletions packets.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,54 +24,53 @@ import (

// Read packet to buffer 'data'
func (mc *mysqlConn) readPacket() ([]byte, error) {
// Read packet header
data, err := mc.buf.readNext(4)
if err != nil {
errLog.Print(err.Error())
mc.Close()
return nil, driver.ErrBadConn
}
var payload []byte
for {
// Read packet header
data, err := mc.buf.readNext(4)
if err != nil {
errLog.Print(err.Error())
mc.Close()
return nil, driver.ErrBadConn
}

// Packet Length [24 bit]
pktLen := int(uint32(data[0]) | uint32(data[1])<<8 | uint32(data[2])<<16)
// Packet Length [24 bit]
pktLen := int(uint32(data[0]) | uint32(data[1])<<8 | uint32(data[2])<<16)

if pktLen < 1 {
errLog.Print(errMalformPkt.Error())
mc.Close()
return nil, driver.ErrBadConn
}
if pktLen < 1 {
errLog.Print(errMalformPkt.Error())
mc.Close()
return nil, driver.ErrBadConn
}

// Check Packet Sync [8 bit]
if data[3] != mc.sequence {
if data[3] > mc.sequence {
return nil, errPktSyncMul
} else {
return nil, errPktSync
// Check Packet Sync [8 bit]
if data[3] != mc.sequence {
if data[3] > mc.sequence {
return nil, errPktSyncMul
} else {
return nil, errPktSync
}
}
}
mc.sequence++
mc.sequence++

// Read packet body [pktLen bytes]
if data, err = mc.buf.readNext(pktLen); err == nil {
if pktLen < maxPacketSize {
return data, nil
// Read packet body [pktLen bytes]
data, err = mc.buf.readNext(pktLen)
if err != nil {
errLog.Print(err.Error())
mc.Close()
return nil, driver.ErrBadConn
}

// Make a copy since data becomes invalid with the next read
buf := make([]byte, len(data))
copy(buf, data)
Copy link
Member

@arnehormann arnehormann Nov 11, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't need the copy without the recursion - data stays valid and appending copies it anyway

Copy link
Contributor Author

@cxmcc cxmcc Nov 11, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point! just added a commit to remove that.


// More data
data, err = mc.readPacket()
if err == nil {
return append(buf, data...), nil
payload = append(payload, buf...)

if pktLen < maxPacketSize {
Copy link
Member

@arnehormann arnehormann Nov 11, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@julienschmidt is you critique still valid if the append above is removed and this if becomes

if pktLen >= maxPacketSize {
  payload = append(payload, data...)
  continue
}
return data, nil

Copy link
Member

@arnehormann arnehormann Nov 11, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and (handwaving) the correct variable is returned every time... (not data if it should be payload)

Copy link
Contributor Author

@cxmcc cxmcc Nov 11, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@julienschmidt I understood what you mean.

It's doing payload = append(nil, data) for common cases. (I thought golang will do payload=data for append(nil...) but obviously not.)

We can still disable the buffer copying for common case by doing what @arnehormann suggested.

return payload, nil
}
}

// err case
mc.Close()
errLog.Print(err.Error())
return nil, driver.ErrBadConn
}

// Write packet buffer 'data'
Expand Down