DecodeMessage: skip bytes Buffer, reducing alloc overhead

[Dieterbe]

see #174 (comment)
for more discussion and before and after

[mreiferson]

restarted that one test failure, just looks like a flakey test

[mreiferson]

should we define a constant? I had to sit and think about why you chose 11 for too long 😁

[Dieterbe]

i would say no. because the lines below make it clear. this function has hardcoded slice ranges from 0-7, 8 to 9, etc. pulling out the number 11 (or 11+MsgIDLength) and defining it elsewhere feels weird because than you have a bunch of magic numbers except for one that is defined elsewhere. if we go this route, than we should also constant-ize all the other magic numbers IMHO. (which i wouldn't do, seems overkill. what we could do, is document the message format somewhere - if not already exists - and link to it)

[twmb]

It's only confusing being 11 because nsqd rejects length 0 bodies. I'd expect in the code for this to be 10, the len(int64) and len(uint16).

FWIW, NewMessage accepts length 0 bodies; it'd be more consistent for this to accept length 0 bodies. Also might make sense for nsqd to do the same. IIRC there was an issue/discussion about that somewhere.

[mreiferson]

https://github.com/nsqio/nsq/blob/73cb4384556d670446da8d9a3fe0117d19b41f17/nsqd/message.go#L14

We're already doing something similar on the nsqd side so let's keep it consistent (which, btw, I'm realizing is also eligible for this same optimization).

Feel free to also copy the message format into the function comment:

http://nsq.io/clients/tcp_protocol_spec.html#data-format

[x][x][x][x][x][x][x][x][x][x][x][x][x][x][x][x][x][x][x][x][x][x][x][x][x][x][x][x][x][x]...
|       (int64)        ||    ||      (hex string encoded in ASCII)           || (binary)
|       8-byte         ||    ||                 16-byte                      || N-byte
------------------------------------------------------------------------------------------...
  nanosecond timestamp    ^^                   message ID                       message body
                       (uint16)
                        2-byte
                       attempts

[Dieterbe]

so you're referring to https://github.com/nsqio/nsq/blob/fa56e2fda80c08f1428b9139f516b9aadac117c3/nsqd/message.go#L69-L92

1. i guess we could argue for days about this, but i'm personally not a big fan of having part of the numbers hardcoded in the function and part defined elsewhere. if minValidMsgLength ever changes then i can guarantee you this function will change as well. so in my mind it makes more sense to keep all these related numbers together in one place.

2. that function allows empty bodies. whether nsqd allows empty bodies or not, i actually don't care much (though we should obviously follow the same rules everywhere). i did this check cause i thought b[10+16:] would have an index out of bounds if len was 26, but turns out i'm wrong about that so i'm ok with changing it

[mreiferson]

green light, thanks @Dieterbe

If you don't mind addressing that one comment and squashing in, this LGTM!

[twmb]

package nsq

import (
    "bytes"
    "math/rand"
    "reflect"
    "sync"
    "testing"
    "testing/quick"
    "time"
)

var quickID = func() func() MessageID {
    t := reflect.TypeOf(MessageID{})
    r := rand.New(rand.NewSource(time.Now().UnixNano()))
    var m sync.Mutex
    return func() MessageID {
        m.Lock()
        v, _ := quick.Value(t, r)
        m.Unlock()
        return v.Interface().(MessageID)
    }
}()

func TestDecodeMessage(t *testing.T) {
    // Create our message: nsqd rejects length 0 messages.
    message := NewMessage(quickID(), make([]byte, 1))
    // Encode the message.
    buf := new(bytes.Buffer)
    message.WriteTo(buf)
    encoded := buf.Bytes()
    // We expect a valid, min length message to decode idempotently.
    decodeRight, err := DecodeMessage(encoded)
    if err != nil {
        t.Errorf("expected idempotent encode/decode, got err: %v", err)
    }
    if !reflect.DeepEqual(message, decodeRight) {
        t.Error("expected idempotent encode/decode, not")
    }
    // We expect a message with no body to not decode.
    encoded = encoded[:len(encoded)-1] // trim the 1 byte body
    _, err = DecodeMessage(encoded)
    if err == nil {
        t.Error("expected invalid encode to not decode")
    }
}

TIL that nsqd rejects length 0 messages.

[jehiah]

Docstring nitpick: message ID is binary w/r/t the protocol. Practically we chose to use ascii printable characters for now.

[mreiferson]

(if we want to reword that, that's in the official docs)